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 2022/05/19 17:06:39 UTC
[GitHub] [tvm] Lunderberg opened a new pull request, #11373: [TIR] Additional Stmt/Expr simplication rules
Lunderberg opened a new pull request, #11373:
URL: https://github.com/apache/tvm/pull/11373
These came about after investigating some simplifications that may be useful for handling of padding introduced during padded layout transformations.
- Enabled simplification of `A[i] = A[i] + 0` into no-op. This was a bug introduced in https://github.com/apache/tvm/pull/9727, which applied this rewrite only to `A[i] = A[i]`, and not to statements which simplify to `A[i] = A[i]`. Regression test added to prevent reoccurrence of this bug.
- Enabled simplification of `x - x` to zero for floating point types. Previously, this simplification was applied only for data types that could be used as buffer indices.
--
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
[GitHub] [tvm] tkonolige commented on pull request #11373: [TIR] Additional Stmt/Expr simplication rules
Posted by GitBox <gi...@apache.org>.
tkonolige commented on PR #11373:
URL: https://github.com/apache/tvm/pull/11373#issuecomment-1132043565
`x - x = 0` isn't valid for all IEEE floating point numbers. Specifically `Inf - Inf = Nan` and `NaN - NaN = NaN`. Not sure if we care about these edge cases though.
--
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
[GitHub] [tvm] Lunderberg commented on pull request #11373: [TIR] Additional Stmt/Expr simplication rules
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #11373:
URL: https://github.com/apache/tvm/pull/11373#issuecomment-1137753898
Retriggering CI following #11456.
--
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
[GitHub] [tvm] vinx13 commented on a diff in pull request #11373: [TIR] Additional Stmt/Expr simplication rules
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #11373:
URL: https://github.com/apache/tvm/pull/11373#discussion_r880710604
##########
tests/python/unittest/test_tir_transform_simplify.py:
##########
@@ -133,9 +134,41 @@ def sls(n, d):
assert "if" not in str(stmt)
+def test_load_store_noop():
+ """Store of a value that was just read from the same location is a no-op."""
+
+ @T.prim_func
+ def before(A: T.Buffer[(1,), "float32"]):
+ A[0] = A[0]
+
+ @T.prim_func
+ def expected(A: T.Buffer[(1,), "float32"]):
+ T.evaluate(0)
+
+ after = tvm.tir.transform.Simplify()(tvm.IRModule.from_expr(before))["main"]
+ tvm.ir.assert_structural_equal(after, expected)
+
+
+def test_load_store_noop_after_simplify():
+ """As test_load_store_noop, but requiring simplification to identify.
+
+ Previously, a bug caused the self-assignment of a buffer to
+ checked based on the pre-simplification assignment, not the
+ post-simplification. This test is to identify any similar
+ regression.
+ """
+
+ @T.prim_func
+ def before(A: T.Buffer[(1,), "float32"]):
+ A[0] = A[0] + (5.0 - 5.0)
+
+ @T.prim_func
+ def expected(A: T.Buffer[(1,), "float32"]):
+ T.evaluate(0)
+
+ after = tvm.tir.transform.Simplify()(tvm.IRModule.from_expr(before))["main"]
+ tvm.ir.assert_structural_equal(after, expected)
+
+
if __name__ == "__main__":
- test_stmt_simplify()
- test_thread_extent_simplify()
- test_if_likely()
- test_basic_likely_elimination()
- test_complex_likely_elimination()
+ sys.exit(pytest.main(sys.argv))
Review Comment:
As suggested in https://github.com/apache/tvm/issues/11318, we can use `tvm.testing.main()` instead
--
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
[GitHub] [tvm] Lunderberg commented on pull request #11373: [TIR] Additional Stmt/Expr simplication rules
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #11373:
URL: https://github.com/apache/tvm/pull/11373#issuecomment-1132096985
Good point, and I hadn't considered Nan. Looking through other operators, it looks like there are existing simplifications that would also change `NaN` propagation (e.g. [Simplifying `x==y || x!=y`](https://github.com/apache/tvm/blob/main/src/arith/rewrite_simplify.cc#L1553) to true, or [simplifying `!(x <= y)` to `y < x`](https://github.com/apache/tvm/blob/main/src/arith/rewrite_simplify.cc#L1485)), so it wouldn't break any existing guarantees.
--
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
[GitHub] [tvm] Lunderberg commented on a diff in pull request #11373: [TIR] Additional Stmt/Expr simplication rules
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #11373:
URL: https://github.com/apache/tvm/pull/11373#discussion_r881773847
##########
tests/python/unittest/test_tir_transform_simplify.py:
##########
@@ -133,9 +134,41 @@ def sls(n, d):
assert "if" not in str(stmt)
+def test_load_store_noop():
+ """Store of a value that was just read from the same location is a no-op."""
+
+ @T.prim_func
+ def before(A: T.Buffer[(1,), "float32"]):
+ A[0] = A[0]
+
+ @T.prim_func
+ def expected(A: T.Buffer[(1,), "float32"]):
+ T.evaluate(0)
+
+ after = tvm.tir.transform.Simplify()(tvm.IRModule.from_expr(before))["main"]
+ tvm.ir.assert_structural_equal(after, expected)
+
+
+def test_load_store_noop_after_simplify():
+ """As test_load_store_noop, but requiring simplification to identify.
+
+ Previously, a bug caused the self-assignment of a buffer to
+ checked based on the pre-simplification assignment, not the
+ post-simplification. This test is to identify any similar
+ regression.
+ """
+
+ @T.prim_func
+ def before(A: T.Buffer[(1,), "float32"]):
+ A[0] = A[0] + (5.0 - 5.0)
+
+ @T.prim_func
+ def expected(A: T.Buffer[(1,), "float32"]):
+ T.evaluate(0)
+
+ after = tvm.tir.transform.Simplify()(tvm.IRModule.from_expr(before))["main"]
+ tvm.ir.assert_structural_equal(after, expected)
+
+
if __name__ == "__main__":
- test_stmt_simplify()
- test_thread_extent_simplify()
- test_if_likely()
- test_basic_likely_elimination()
- test_complex_likely_elimination()
+ sys.exit(pytest.main(sys.argv))
Review Comment:
Ooh, I like the change. Rebased to include #11393, and updated to use `tvm.testing.main()`. Looks like this file was missed in #11393, which is why it didn't appear as a conflict.
##########
tests/python/unittest/test_tir_transform_simplify.py:
##########
@@ -133,9 +134,41 @@ def sls(n, d):
assert "if" not in str(stmt)
+def test_load_store_noop():
+ """Store of a value that was just read from the same location is a no-op."""
+
+ @T.prim_func
+ def before(A: T.Buffer[(1,), "float32"]):
+ A[0] = A[0]
+
+ @T.prim_func
+ def expected(A: T.Buffer[(1,), "float32"]):
+ T.evaluate(0)
+
+ after = tvm.tir.transform.Simplify()(tvm.IRModule.from_expr(before))["main"]
+ tvm.ir.assert_structural_equal(after, expected)
+
+
+def test_load_store_noop_after_simplify():
+ """As test_load_store_noop, but requiring simplification to identify.
+
+ Previously, a bug caused the self-assignment of a buffer to
+ checked based on the pre-simplification assignment, not the
+ post-simplification. This test is to identify any similar
+ regression.
+ """
+
+ @T.prim_func
+ def before(A: T.Buffer[(1,), "float32"]):
+ A[0] = A[0] + (5.0 - 5.0)
+
+ @T.prim_func
+ def expected(A: T.Buffer[(1,), "float32"]):
+ T.evaluate(0)
+
+ after = tvm.tir.transform.Simplify()(tvm.IRModule.from_expr(before))["main"]
+ tvm.ir.assert_structural_equal(after, expected)
+
+
if __name__ == "__main__":
- test_stmt_simplify()
- test_thread_extent_simplify()
- test_if_likely()
- test_basic_likely_elimination()
- test_complex_likely_elimination()
+ sys.exit(pytest.main(sys.argv))
Review Comment:
Ooh, I like the new utility. Rebased to include #11393, and updated to use `tvm.testing.main()`. Looks like this file was missed in #11393, which is why it didn't appear as a conflict.
--
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
[GitHub] [tvm] vinx13 commented on a diff in pull request #11373: [TIR] Additional Stmt/Expr simplication rules
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #11373:
URL: https://github.com/apache/tvm/pull/11373#discussion_r882216891
##########
src/arith/rewrite_simplify.cc:
##########
@@ -256,6 +256,14 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const SubNode* op) {
TVM_TRY_REWRITE(broadcast(x, lanes) - broadcast(y, lanes), broadcast(x - y, lanes));
}
+ // cancelation rules
+ TVM_TRY_REWRITE_IF(x - x, ZeroWithTypeLike(x),
+ SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF((x + y) - y, x, SideEffect(y.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF((x + y) - x, y, SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF(x - (y + x), 0 - y, SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF(x - (x + y), 0 - y, SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+
Review Comment:
should this be removed since you've added the else branch?
--
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
[GitHub] [tvm] Lunderberg commented on a diff in pull request #11373: [TIR] Additional Stmt/Expr simplication rules
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #11373:
URL: https://github.com/apache/tvm/pull/11373#discussion_r882600432
##########
src/arith/rewrite_simplify.cc:
##########
@@ -256,6 +256,14 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const SubNode* op) {
TVM_TRY_REWRITE(broadcast(x, lanes) - broadcast(y, lanes), broadcast(x - y, lanes));
}
+ // cancelation rules
+ TVM_TRY_REWRITE_IF(x - x, ZeroWithTypeLike(x),
+ SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF((x + y) - y, x, SideEffect(y.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF((x + y) - x, y, SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF(x - (y + x), 0 - y, SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF(x - (x + y), 0 - y, SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+
Review Comment:
Thank you for the catch, and they are now removed.
--
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
[GitHub] [tvm] vinx13 commented on a diff in pull request #11373: [TIR] Additional Stmt/Expr simplication rules
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #11373:
URL: https://github.com/apache/tvm/pull/11373#discussion_r879870628
##########
src/arith/rewrite_simplify.cc:
##########
@@ -256,13 +256,16 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const SubNode* op) {
TVM_TRY_REWRITE(broadcast(x, lanes) - broadcast(y, lanes), broadcast(x - y, lanes));
}
+ // cancelation rules
+ TVM_TRY_REWRITE_IF(x - x, ZeroWithTypeLike(x),
+ SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF((x + y) - y, x, SideEffect(y.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF((x + y) - x, y, SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF(x - (y + x), 0 - y, SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF(x - (x + y), 0 - y, SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+
if (IsIndexType(op->dtype)) {
// Index rules
- // cancelation rules
Review Comment:
The integer simplifcation should be fast path (and clean, hopefully without Nans), it would be better to put new rules in else branch even if it means some duplications, since (recursively) checking side effects is expensive.
Doing floating point simplification is probably fine, but given the possibility of introducing additional types, it might be safer to say something like
```
if (IsIndexType(op->dtype)) {
old rules
} else if (op->dtype.is_float())
new rules
}
```
--
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
[GitHub] [tvm] Lunderberg commented on a diff in pull request #11373: [TIR] Additional Stmt/Expr simplication rules
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #11373:
URL: https://github.com/apache/tvm/pull/11373#discussion_r880556275
##########
src/arith/rewrite_simplify.cc:
##########
@@ -256,13 +256,16 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const SubNode* op) {
TVM_TRY_REWRITE(broadcast(x, lanes) - broadcast(y, lanes), broadcast(x - y, lanes));
}
+ // cancelation rules
+ TVM_TRY_REWRITE_IF(x - x, ZeroWithTypeLike(x),
+ SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF((x + y) - y, x, SideEffect(y.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF((x + y) - x, y, SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF(x - (y + x), 0 - y, SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+ TVM_TRY_REWRITE_IF(x - (x + y), 0 - y, SideEffect(x.Eval()) <= CallEffectKind::kReadState);
+
if (IsIndexType(op->dtype)) {
// Index rules
- // cancelation rules
Review Comment:
Sounds reasonable, and updated as requested.
--
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
[GitHub] [tvm] vinx13 merged pull request #11373: [TIR] Additional Stmt/Expr simplication rules
Posted by GitBox <gi...@apache.org>.
vinx13 merged PR #11373:
URL: https://github.com/apache/tvm/pull/11373
--
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