[GitHub] [tvm] ANSHUMAN87 commented on a change in pull request #7807: [Relay][Pass] Update SimplifyTranspose to correctly simplify rank changing layout transforms

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

ANSHUMAN87 commented on a change in pull request #7807:
URL: https://github.com/apache/tvm/pull/7807#discussion_r609786864



##########
File path: src/relay/transforms/simplify_expr.cc
##########
@@ -163,6 +141,69 @@ class SimplifyTranspose : public DFPatternRewrite {
     return x;
   }
 
+  String PermuteLayout(const String& layout, std::vector<int> axes) const {
+    std::string new_layout{};

Review comment:
       nit: Can we use String instead of std::string ?
   Just to maintain uniformity.

##########
File path: src/relay/transforms/simplify_expr.cc
##########
@@ -163,6 +141,69 @@ class SimplifyTranspose : public DFPatternRewrite {
     return x;
   }
 
+  String PermuteLayout(const String& layout, std::vector<int> axes) const {

Review comment:
       I find this is prone for errors.
   So can we add list of possible values allowed in layout ? If not we can add some validation before accessing layout.
   Also i think it would be good to add debug logs here as the layout has changed from x --> y.
   It will help detect errors in future developments. 

##########
File path: tests/python/relay/test_pass_simplify_expr.py
##########
@@ -106,10 +106,28 @@ def expected3():
         y = relay.transpose(y, axes=[0, 2, 3, 1])
         return relay.Function([x], y)
 
+    # Test a series of transpose and rank changing layout_transform

Review comment:
       This test case looks great. Pretty clear to understand what is happening.
   I think it would be great we can add one more fusion scenario as well, like some of the test cases before and the new one.
   Please let me know in case i am not clear.




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