You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "Lunderberg (via GitHub)" <gi...@apache.org> on 2024/07/15 21:02:38 UTC
[PR] [TIR][Analyzer] Simplify `x==x` expressions for all dtypes [tvm]
Lunderberg opened a new pull request, #17158:
URL: https://github.com/apache/tvm/pull/17158
Prior to this commit, there was no rule to simplify `x == x` into `True`. In some cases, despite not having an explicit rewrite rule in `RewriteSimplifier`, the `RewriteSimplifier::CanProve` function would check if `x-x` simplifies to zero, relying on the rewrite rules used for `tir::Sub`. However, the rule to rewrite `x-x` into zero was only enabled for `int32`, `int64`, and floating-point types, so relying on this behavior was inconsistent.
This commit updates the rewrite rules for both `tir::EQ` and `tir::Sub` to check for simplification of `x-x` or `x==x`, regardless of the datatype. This change preserves the fast-path for index data-types, in which `int32` and `int64` expressions may be simplified without checking for side effects. For all other dtypes, the cancellation only applies when evaluating `x` has no side effects.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TIR][Analyzer] Simplify `x==x` expressions for all dtypes [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on code in PR #17158:
URL: https://github.com/apache/tvm/pull/17158#discussion_r1679736063
##########
src/arith/rewrite_simplify.cc:
##########
@@ -697,7 +698,7 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const SubNode* op) {
TVM_TRY_RECURSIVE_REWRITE(x - (y + c1), (x - y) + (0 - c1));
TVM_TRY_RECURSIVE_REWRITE(x - (y - z), (x + z) - y);
TVM_TRY_RECURSIVE_REWRITE(x - y * c1, x + y * (0 - c1));
- } else if (op->dtype.is_float()) {
+ } else {
Review Comment:
Some of the rewrite rules in the `IsIndexDType` branch assume that overflow won't occur. While this isn't guaranteed to be true, we're more likely to get overflow for small integer types than for the 32/64-bit signed integers used for index dtypes.
For the uint16/uint8 types seen in https://github.com/apache/tvm/pull/17149, it would be a lot easier to get accidental overflow.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TIR][Analyzer] Simplify `x==x` expressions for all dtypes [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen merged PR #17158:
URL: https://github.com/apache/tvm/pull/17158
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TIR][Analyzer] Simplify `x==x` expressions for all dtypes [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on code in PR #17158:
URL: https://github.com/apache/tvm/pull/17158#discussion_r1683501013
##########
src/arith/rewrite_simplify.cc:
##########
@@ -1698,6 +1700,11 @@ PrimExpr RewriteSimplifier::Impl::ApplyRewriteRules(EQ ret) {
TVM_TRY_REWRITE(c1 - x == c2, x == c1 - c2);
TVM_TRY_REWRITE(x + c1 == c2, x == c2 - c1);
TVM_TRY_RECURSIVE_REWRITE(x * y == 0, x == 0 || y == 0);
+ TVM_TRY_REWRITE(x == x, ctrue);
+ } else {
Review Comment:
Good point, and I'll add a comment on it. In general, the numeric simplifications assume that the values are neither `NaN` nor `Inf`, and that expressions do not rely on integer overflow for correctness. That is, integer types and floating-point types are simplified as if they are mathematical integer/real numbers, rather than IEEE floats.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TIR][Analyzer] Simplify `x==x` expressions for all dtypes [tvm]
Posted by "wrongtest-intellif (via GitHub)" <gi...@apache.org>.
wrongtest-intellif commented on code in PR #17158:
URL: https://github.com/apache/tvm/pull/17158#discussion_r1682038424
##########
src/arith/rewrite_simplify.cc:
##########
@@ -1698,6 +1700,11 @@ PrimExpr RewriteSimplifier::Impl::ApplyRewriteRules(EQ ret) {
TVM_TRY_REWRITE(c1 - x == c2, x == c1 - c2);
TVM_TRY_REWRITE(x + c1 == c2, x == c2 - c1);
TVM_TRY_RECURSIVE_REWRITE(x * y == 0, x == 0 || y == 0);
+ TVM_TRY_REWRITE(x == x, ctrue);
+ } else {
Review Comment:
Hi, I have one question on that generally `NaN == NaN` evaluates to False. I think it would be great to leave some explaination on the case. For example, could we just define it as some "undefined" behaviour?
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TIR][Analyzer] Simplify `x==x` expressions for all dtypes [tvm]
Posted by "sunggg (via GitHub)" <gi...@apache.org>.
sunggg commented on code in PR #17158:
URL: https://github.com/apache/tvm/pull/17158#discussion_r1679655697
##########
src/arith/rewrite_simplify.cc:
##########
@@ -697,7 +698,7 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const SubNode* op) {
TVM_TRY_RECURSIVE_REWRITE(x - (y + c1), (x - y) + (0 - c1));
TVM_TRY_RECURSIVE_REWRITE(x - (y - z), (x + z) - y);
TVM_TRY_RECURSIVE_REWRITE(x - y * c1, x + y * (0 - c1));
- } else if (op->dtype.is_float()) {
+ } else {
Review Comment:
For currently unsupported integer types, is there any reason to take this path rather than path for the index types? They are also integers and this path has more rules defined. But, I'm not currently sure if it supports negative index but if it does not, it could be effectively same with unsigned integer?
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org