[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6008: [Relay][Dyn] Dynamic TopK Op

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

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



##########
File path: src/relay/transforms/dynamic_to_static.cc
##########
@@ -55,6 +58,20 @@ class DynamicToStaticMutator : public MixedModeMutator {
         static const Op& op = Op::Get("tile");
         return Call(op, {call_node->args[0]}, Attrs(attrs), {});
       }
+    } else if (call_node->op == dyn_topk_op_) {
+      if (const ConstantNode* k = call_node->args[1].as<ConstantNode>()) {

Review comment:
       Maybe we should have `MakeTopK`, `MakeTile`, etc?

##########
File path: src/relay/transforms/dynamic_to_static.cc
##########
@@ -33,7 +34,9 @@ namespace relay {
 class DynamicToStaticMutator : public MixedModeMutator {
  public:
   DynamicToStaticMutator()
-      : dyn_reshape_op_(Op::Get("dyn.reshape")), dyn_tile_op_(Op::Get("dyn.tile")) {}
+      : dyn_reshape_op_(Op::Get("dyn.reshape")),

Review comment:
       In this case, I think it is probably okay to  not cache them. Otherwise, we may have a long list here and a long list of members. This would have some lookup overhead, but it increases the readability. How do you think?




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