[GitHub] [tvm] csullivan commented on a change in pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.

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

csullivan commented on a change in pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#discussion_r623399795



##########
File path: src/relay/backend/graph_executor_codegen.cc
##########
@@ -380,6 +380,19 @@ class GraphExecutorCodegen : public backend::MemoizedExprTranslator<std::vector<
       return GraphAddCallNode(op, ext_func->func_name, ext_func->func_name);
     }
 
+    // In the current flat memory allocation scenario
+    // the flat memory allocator can always allocate input
+    // and output of the reshape to the same memory, we can turn reshape only
+    // function to a nop.
+    //
+    // NOTE that for non-flat memory this is not necessarily true.
+    //
+    // TODO(tvm-team) Update checks of flat memory enablement when we support
+    // opaque-nd memory planning to skip this path.
+    if (func->HasNonzeroAttr(attr::kReshapeOnly)) {
+      return GraphAddCallNode(op, "reshape_nop", "__nop");
+    }

Review comment:
       This remains problematic once we have memory planning for nd allocations (like #7690). 
   
   The criteria for eliding a reshape (that it is logical only) should be that it's both a reshape _and_ its input and output are aliased. My feeling is that the conditional should reflect this, with a check on both the `attr::kReshapeOnly` attribute as well as whether the outputs storage_ids are equivalent. If the latter criteria isn't met we shouldn't emit a nop.
   
   Can you add a helper in the conditional that checks this with the `storage_device_map_`?




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