[GitHub] [incubator-tvm] comaniac commented on a change in pull request #5914: [clflush] Enable x86 cpu cache flush

[GitBox <gi...@apache.org>]

comaniac commented on a change in pull request #5914:
URL: https://github.com/apache/incubator-tvm/pull/5914#discussion_r453779891



##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -309,7 +313,8 @@ class LocalRunner(RPCRunner):
         Whether check correctness after measurement. This will use llvm cpu target to
         call your template and get the reference output.
         This can work for TOPI templates, but may not work for your custom template.
-
+    enable_cpu_cache_flush: bool
+        Whether to enable cpu cache flush

Review comment:
       Maybe explicitly mention that this has no effect on GPU tasks?

##########
File path: python/tvm/runtime/module.py
##########
@@ -163,7 +163,7 @@ def save(self, file_name, fmt=""):
         """
         _ffi_api.ModuleSaveToFile(self, file_name, fmt)
 
-    def time_evaluator(self, func_name, ctx, number=10, repeat=1, min_repeat_ms=0):
+    def time_evaluator(self, func_name, ctx, number=10, repeat=1, min_repeat_ms=0, f_prepare=''):

Review comment:
       Would `f_preproc` be more clear?

##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -454,6 +461,8 @@ def run_through_rpc(measure_input, build_result,
         The reference input used for checking correctness
     ref_output: List of np.ndarray
         The reference output used for checking correctness
+    enable_cpu_cache_flush: bool
+        Whether to enable cpu cache flush

Review comment:
       ditto

##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -473,8 +482,10 @@ def run_through_rpc(measure_input, build_result,
         remote.upload(build_result.filename)
         func = remote.load_module(os.path.split(build_result.filename)[1])
         ctx = remote.context(str(measure_input.target), 0)
+        f_prepare = 'cache_flush_cpu_non_first_arg' if enable_cpu_cache_flush else ''

Review comment:
       The processing flow of this function seems a bit long:
   1. setting `enable_cpu_cache_flush` to True
   2. setting `f_prepare` to "cache_flush_cpu_non_first_arg"
   3. setting `pf_prepare` to the packed function
   
   To me, it's hard for people to realize "cache_flush_cpu_non_first_arg" is the global function symbol unless tracing back to `rpc_module.cc`. Is that possible to get the packed function here directly so that we can set the type of `f_prepare` to `PackedFunc`? It's also more general for its description (the prepared function) IMHO.
   

##########
File path: tutorials/autotvm/tune_relay_x86.py
##########
@@ -122,8 +122,9 @@ def get_network(name, batch_size):
 
     'measure_option': autotvm.measure_option(
         builder=autotvm.LocalBuilder(),
-        runner=autotvm.LocalRunner(number=10, repeat=1,
-                                   min_repeat_ms=1000),
+        runner=autotvm.LocalRunner(number=1, repeat=10,
+                                   min_repeat_ms=0,
+                                   enable_cpu_cache_flush=True),

Review comment:
       Might be better to discuss `enable_cpu_cache_flush` in this tutorial to explain 1) this is only for CPU, and 2) how we set number/repeat/min_repeat_ms when this option is enabled.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org