You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/03/18 09:06:27 UTC

[GitHub] [tvm] leeexyz opened a new pull request #7692: [TE] Bugfix for reduction that involves multi-outs with where cond

leeexyz opened a new pull request #7692:
URL: https://github.com/apache/tvm/pull/7692


   Hi All,
   There is a bug if you specified the where condition in comm_reducer that involves multiple outputs because the cond for each output has a different address.
   Fix:
   1. Using deep equal to compare conditions when verify compute operation.
   2. Invoking test cases that do not be executed before.


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

[GitHub] [tvm] comaniac merged pull request #7692: [TE] Bugfix for reduction that involves multi-outs with where cond

Posted by GitBox <gi...@apache.org>.
comaniac merged pull request #7692:
URL: https://github.com/apache/tvm/pull/7692


   


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

[GitHub] [tvm] comaniac commented on pull request #7692: [TE] Bugfix for reduction that involves multi-outs with where cond

Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #7692:
URL: https://github.com/apache/tvm/pull/7692#issuecomment-811620141


   Thanks @leeexyz @huajsj 


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

[GitHub] [tvm] leeexyz commented on pull request #7692: [TE] Bugfix for reduction that involves multi-outs with where cond

Posted by GitBox <gi...@apache.org>.
leeexyz commented on pull request #7692:
URL: https://github.com/apache/tvm/pull/7692#issuecomment-811589240


   @comaniac Thanks for your remind. I confirmed again. Parameters like combiner, source, axis, and init do not have this issue. :)


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

[GitHub] [tvm] comaniac commented on pull request #7692: [TE] Bugfix for reduction that involves multi-outs with where cond

Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #7692:
URL: https://github.com/apache/tvm/pull/7692#issuecomment-811345413


   Just to make sure. Will other parameters (i.e., combiner, axis) have this issue?


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

[GitHub] [tvm] leeexyz commented on a change in pull request #7692: [TE] Bugfix for reduction that involves multi-outs with where cond

Posted by GitBox <gi...@apache.org>.
leeexyz commented on a change in pull request #7692:
URL: https://github.com/apache/tvm/pull/7692#discussion_r598213293



##########
File path: src/te/operation/compute_op.cc
##########
@@ -57,8 +57,10 @@ TVM_REGISTER_NODE_TYPE(ComputeOpNode);
 static void VerifyComputeOp(const ComputeOpNode* op);
 
 inline bool ReduceEqual(const tir::ReduceNode* a, const tir::ReduceNode* b) {
+  StructuralEqual expr_equal;
   return (a->combiner.same_as(b->combiner)) && (a->source.same_as(b->source)) &&
-         (a->axis.same_as(b->axis)) && (a->condition.same_as(b->condition)) &&
+         (a->axis.same_as(b->axis)) &&
+         (a->condition.same_as(b->condition) || expr_equal(a->condition, b->condition)) &&

Review comment:
       Thanks, it's more elegant.




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

[GitHub] [tvm] leeexyz commented on pull request #7692: [TE] Bugfix for reduction that involves multi-outs with where cond

Posted by GitBox <gi...@apache.org>.
leeexyz commented on pull request #7692:
URL: https://github.com/apache/tvm/pull/7692#issuecomment-810864261


   Hi @huajsj @comaniac, can you guys help me review it?


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

[GitHub] [tvm] huajsj commented on a change in pull request #7692: [TE] Bugfix for reduction that involves multi-outs with where cond

Posted by GitBox <gi...@apache.org>.
huajsj commented on a change in pull request #7692:
URL: https://github.com/apache/tvm/pull/7692#discussion_r597946154



##########
File path: src/te/operation/compute_op.cc
##########
@@ -57,8 +57,10 @@ TVM_REGISTER_NODE_TYPE(ComputeOpNode);
 static void VerifyComputeOp(const ComputeOpNode* op);
 
 inline bool ReduceEqual(const tir::ReduceNode* a, const tir::ReduceNode* b) {
+  StructuralEqual expr_equal;
   return (a->combiner.same_as(b->combiner)) && (a->source.same_as(b->source)) &&
-         (a->axis.same_as(b->axis)) && (a->condition.same_as(b->condition)) &&
+         (a->axis.same_as(b->axis)) &&
+         (a->condition.same_as(b->condition) || expr_equal(a->condition, b->condition)) &&

Review comment:
       the additional variable  'expr_equal' is not necessary, how about  "StructuralEqual()(a->condition, b->condition)"

##########
File path: src/te/operation/compute_op.cc
##########
@@ -57,8 +57,10 @@ TVM_REGISTER_NODE_TYPE(ComputeOpNode);
 static void VerifyComputeOp(const ComputeOpNode* op);
 
 inline bool ReduceEqual(const tir::ReduceNode* a, const tir::ReduceNode* b) {
+  StructuralEqual expr_equal;
   return (a->combiner.same_as(b->combiner)) && (a->source.same_as(b->source)) &&
-         (a->axis.same_as(b->axis)) && (a->condition.same_as(b->condition)) &&
+         (a->axis.same_as(b->axis)) &&
+         (a->condition.same_as(b->condition) || expr_equal(a->condition, b->condition)) &&

Review comment:
       expr_equal(a->condition, b->condition) should can cover a->condition.same_as(b->condition), condition.same_as seems like not necessary.




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