Lunderberg commented on code in PR #13933:
URL: https://github.com/apache/tvm/pull/13933#discussion_r1109009516
##########
src/arith/rewrite_simplify.cc:
##########
@@ -344,69 +355,47 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const
SubNode* op) {
if (IsIndexType(op->dtype)) {
// Index rules
// cancelation rules
- TVM_TRY_REWRITE((x + y) - y, x);
- TVM_TRY_REWRITE((x + y) - x, y);
- TVM_TRY_REWRITE(x - (y + x), 0 - y);
- TVM_TRY_REWRITE(x - (x + y), 0 - y);
+ TVM_TRY_REWRITE(matches_one_of((x + y) - y, (y + x) - y), x);
+ TVM_TRY_REWRITE(matches_one_of(x - (y + x), x - (x + y)), 0 - y);
- TVM_TRY_REWRITE(min(x, y) - x, min(0, y - x));
- TVM_TRY_REWRITE(min(x, y) - y, min(x - y, 0));
- TVM_TRY_REWRITE(max(x, y) - x, max(0, y - x));
- TVM_TRY_REWRITE(max(x, y) - y, max(x - y, 0));
-
- TVM_TRY_REWRITE(x - max(x, y), min(0, x - y));
- TVM_TRY_REWRITE(y - max(x, y), min(y - x, 0));
- TVM_TRY_REWRITE(x - min(x, y), max(0, x - y));
- TVM_TRY_REWRITE(y - min(x, y), max(y - x, 0));
+ TVM_TRY_REWRITE(matches_one_of(min(x, y) - y, x - max(y, x)), min(x - y,
0));
Review Comment:
At the moment, yes, this is intentional. The min/max simplification rules
do not normalize constants to the RHS the way most of the commutative binary
operators do, and so some of the unit tests expect the `min(0, x - y)` result
with the zero on the LHS. This PR intentionally kept all rewrites identical,
so the two cases are handled separately.
That said, I plan to have a follow-up PR that moves any constant to the RHS,
at which point these two rules can be replaced with a single rule that results
on `min(x - y, 0)`.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
