[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6351: Dynamic ONNX Importer

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

zhiics commented on a change in pull request #6351:
URL: https://github.com/apache/incubator-tvm/pull/6351#discussion_r488056333



##########
File path: src/relay/transforms/dynamic_to_static.cc
##########
@@ -236,13 +236,13 @@ Expr DynamicToStatic(Function f, IRModule m) {
     expr = mutator.Mutate(m->functions[gv]);
     m->Update(gv, Downcast<BaseFunc>(expr));
     i += 1;
-  } while (pre != expr && i < 1000);
+  } while (!StructuralEqual()(pre, expr) && i < 1000);

Review comment:
       Maybe define 1000 somewhere and add a comment for it?

##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -2293,7 +2247,18 @@ def from_onnx(self, graph, opset):
         # now return the outputs
         outputs = [self._nodes[self._parse_value_proto(i)] for i in graph.output]
         outputs = outputs[0] if len(outputs) == 1 else _expr.Tuple(outputs)
-        func = _function.Function(analysis.free_vars(outputs), outputs)
+        ## Maintain the order of inputs and parametersfrom the ONNX graph, but only include

Review comment:
       ```suggestion
           ## Maintain the order of inputs and parameters from the ONNX graph, but only include
   ```

##########
File path: python/tvm/relay/op/strategy/x86.py
##########
@@ -347,12 +348,20 @@ def dense_strategy_cpu(attrs, inputs, out_type, target):
 def batch_matmul_strategy_cpu(attrs, inputs, out_type, target):
     """batch_matmul x86 strategy"""
     strategy = _op.OpStrategy()
-    strategy.add_implementation(
-        wrap_compute_batch_matmul(topi.x86.batch_matmul),
-        wrap_topi_schedule(topi.x86.schedule_batch_matmul),
-        name="batch_matmul.x86",
-        plevel=10,
-    )
+    if is_dynamic(out_type):
+        strategy.add_implementation(

Review comment:
       shouldn't this one be in generic.py?

##########
File path: tests/python/relay/dyn/test_dynamic_op_level10.py
##########
@@ -27,34 +27,62 @@
 import random
 import tvm.testing
 
-# TODO(mbrookhart): Enable when VM supports heterogenus execution
+# TODO(mbrookhart): Enable when the VM supports heterogenus execution

Review comment:
       Should we enable this since VM supports heterogeneous execution now? or you plan to enable them all in a followup PR?




----------------------------------------------------------------
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