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/10/19 02:43:05 UTC
[GitHub] [tvm] Lunderberg opened a new pull request, #13129: [TIR][Arith] Implement kApplyConstraintsToBooleanBranches extension
Lunderberg opened a new pull request, #13129:
URL: https://github.com/apache/tvm/pull/13129
When simplifying a branch of a boolean AND or a boolean OR, the other branch may be assumed not to dominate the result of the operator. For example, when simplifying `(A and B)`, `A` may be simplified on the assumption that `B` is true. Similarly, when simplifying `(A or B)`, `A` may be simplified on the assumption that `B` is false.
Prior to this commit, these constraints were not used for simplifications. This commit introduced an optional extension, `kApplyConstraintsToBooleanBranches`, which exposes these constraints for simplification. This isn't enabled by default, as some cases require a second visit to each branch of a boolean operator. (e.g. Simplifying the LHS of an operator, using a constraint provided by the RHS, where the sub-analyzer that uses the constraint expects it to already be simplified.)
--
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 #13129: [TIR][Arith] Implement kApplyConstraintsToBooleanBranches extension
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13129:
URL: https://github.com/apache/tvm/pull/13129#discussion_r1007139255
##########
src/arith/rewrite_simplify.cc:
##########
@@ -1557,7 +1591,44 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const NotNode* op) {
}
PrimExpr RewriteSimplifier::Impl::VisitExpr_(const AndNode* op) {
- PrimExpr ret = IRMutatorWithAnalyzer::VisitExpr_(op);
+ PrimExpr ret = [&]() -> PrimExpr {
+ // If this extension isn't enabled, just delegate out.
+ if (!(enabled_extensions_ & kApplyConstraintsToBooleanBranches)) {
+ return IRMutatorWithAnalyzer::VisitExpr_(op);
+ }
+
+ PrimExpr a = op->a;
+ PrimExpr b = op->b;
+
+ // Alternate which branch is used as the constraint, and which
+ // is being simplified. Because some sub-analyzers expect their
+ // constraints to already be simplified, each branch may require
+ // more than update. The loop condition allows each branch to be
+ // visited up to twice, but only if performs the second visit if
+ // necessary.
Review Comment:
Thank you, and updated!
--
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] csullivan commented on a diff in pull request #13129: [TIR][Arith] Implement kApplyConstraintsToBooleanBranches extension
Posted by GitBox <gi...@apache.org>.
csullivan commented on code in PR #13129:
URL: https://github.com/apache/tvm/pull/13129#discussion_r1007080484
##########
src/arith/rewrite_simplify.cc:
##########
@@ -1557,7 +1591,44 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const NotNode* op) {
}
PrimExpr RewriteSimplifier::Impl::VisitExpr_(const AndNode* op) {
- PrimExpr ret = IRMutatorWithAnalyzer::VisitExpr_(op);
+ PrimExpr ret = [&]() -> PrimExpr {
+ // If this extension isn't enabled, just delegate out.
+ if (!(enabled_extensions_ & kApplyConstraintsToBooleanBranches)) {
+ return IRMutatorWithAnalyzer::VisitExpr_(op);
+ }
+
+ PrimExpr a = op->a;
+ PrimExpr b = op->b;
+
+ // Alternate which branch is used as the constraint, and which
+ // is being simplified. Because some sub-analyzers expect their
+ // constraints to already be simplified, each branch may require
+ // more than update. The loop condition allows each branch to be
+ // visited up to twice, but only if performs the second visit if
+ // necessary.
+ for (size_t i = 0, last_updated = 0; i < last_updated + 2 && i < 4; i++) {
Review Comment:
nit: Is there a way to make this for-loop more understandable? The multiple iterator makes it difficult to understand, possibly error prone, without detailed study.
##########
src/arith/rewrite_simplify.cc:
##########
@@ -1557,7 +1591,44 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const NotNode* op) {
}
PrimExpr RewriteSimplifier::Impl::VisitExpr_(const AndNode* op) {
- PrimExpr ret = IRMutatorWithAnalyzer::VisitExpr_(op);
+ PrimExpr ret = [&]() -> PrimExpr {
+ // If this extension isn't enabled, just delegate out.
+ if (!(enabled_extensions_ & kApplyConstraintsToBooleanBranches)) {
+ return IRMutatorWithAnalyzer::VisitExpr_(op);
+ }
+
+ PrimExpr a = op->a;
+ PrimExpr b = op->b;
+
+ // Alternate which branch is used as the constraint, and which
+ // is being simplified. Because some sub-analyzers expect their
+ // constraints to already be simplified, each branch may require
+ // more than update. The loop condition allows each branch to be
+ // visited up to twice, but only if performs the second visit if
+ // necessary.
Review Comment:
nits:
"more than one update".
"but only performs the second visit if 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.
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 #13129: [TIR][Arith] Implement kApplyConstraintsToBooleanBranches extension
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13129:
URL: https://github.com/apache/tvm/pull/13129#discussion_r1008105757
##########
src/arith/rewrite_simplify.cc:
##########
@@ -1557,7 +1591,44 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const NotNode* op) {
}
PrimExpr RewriteSimplifier::Impl::VisitExpr_(const AndNode* op) {
- PrimExpr ret = IRMutatorWithAnalyzer::VisitExpr_(op);
+ PrimExpr ret = [&]() -> PrimExpr {
+ // If this extension isn't enabled, just delegate out.
+ if (!(enabled_extensions_ & kApplyConstraintsToBooleanBranches)) {
+ return IRMutatorWithAnalyzer::VisitExpr_(op);
+ }
+
+ PrimExpr a = op->a;
+ PrimExpr b = op->b;
+
+ // Alternate which branch is used as the constraint, and which
+ // is being simplified. Because some sub-analyzers expect their
+ // constraints to already be simplified, each branch may require
+ // more than update. The loop condition allows each branch to be
+ // visited up to twice, but only if performs the second visit if
+ // necessary.
+ for (size_t i = 0, last_updated = 0; i < last_updated + 2 && i < 4; i++) {
Review Comment:
And updated in the OrNode handler as well.
--
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 merged pull request #13129: [TIR][Arith] Implement kApplyConstraintsToBooleanBranches extension
Posted by GitBox <gi...@apache.org>.
Lunderberg merged PR #13129:
URL: https://github.com/apache/tvm/pull/13129
--
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 #13129: [TIR][Arith] Implement kApplyConstraintsToBooleanBranches extension
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13129:
URL: https://github.com/apache/tvm/pull/13129#discussion_r1007176609
##########
src/arith/rewrite_simplify.cc:
##########
@@ -1557,7 +1591,44 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const NotNode* op) {
}
PrimExpr RewriteSimplifier::Impl::VisitExpr_(const AndNode* op) {
- PrimExpr ret = IRMutatorWithAnalyzer::VisitExpr_(op);
+ PrimExpr ret = [&]() -> PrimExpr {
+ // If this extension isn't enabled, just delegate out.
+ if (!(enabled_extensions_ & kApplyConstraintsToBooleanBranches)) {
+ return IRMutatorWithAnalyzer::VisitExpr_(op);
+ }
+
+ PrimExpr a = op->a;
+ PrimExpr b = op->b;
+
+ // Alternate which branch is used as the constraint, and which
+ // is being simplified. Because some sub-analyzers expect their
+ // constraints to already be simplified, each branch may require
+ // more than update. The loop condition allows each branch to be
+ // visited up to twice, but only if performs the second visit if
+ // necessary.
+ for (size_t i = 0, last_updated = 0; i < last_updated + 2 && i < 4; i++) {
Review Comment:
Rewritten, and it definitely is more coherent this way.
--
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 #13129: [TIR][Arith] Implement kApplyConstraintsToBooleanBranches extension
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #13129:
URL: https://github.com/apache/tvm/pull/13129#issuecomment-1295043417
Thank you!
(Rebased onto main to resolve 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] tvm-bot commented on pull request #13129: [TIR][Arith] Implement kApplyConstraintsToBooleanBranches extension
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13129:
URL: https://github.com/apache/tvm/pull/13129#issuecomment-1283326460
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<!--bot-comment-ccs-start-->
* cc @Hzfengsy, @junrushao <sub>See [#10317](https://github.com/apache/tvm/issues/10317) for details</sub><!--bot-comment-ccs-end-->
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
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 #13129: [TIR][Arith] Implement kApplyConstraintsToBooleanBranches extension
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13129:
URL: https://github.com/apache/tvm/pull/13129#discussion_r1007156911
##########
src/arith/rewrite_simplify.cc:
##########
@@ -1557,7 +1591,44 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const NotNode* op) {
}
PrimExpr RewriteSimplifier::Impl::VisitExpr_(const AndNode* op) {
- PrimExpr ret = IRMutatorWithAnalyzer::VisitExpr_(op);
+ PrimExpr ret = [&]() -> PrimExpr {
+ // If this extension isn't enabled, just delegate out.
+ if (!(enabled_extensions_ & kApplyConstraintsToBooleanBranches)) {
+ return IRMutatorWithAnalyzer::VisitExpr_(op);
+ }
+
+ PrimExpr a = op->a;
+ PrimExpr b = op->b;
+
+ // Alternate which branch is used as the constraint, and which
+ // is being simplified. Because some sub-analyzers expect their
+ // constraints to already be simplified, each branch may require
+ // more than update. The loop condition allows each branch to be
+ // visited up to twice, but only if performs the second visit if
+ // necessary.
+ for (size_t i = 0, last_updated = 0; i < last_updated + 2 && i < 4; i++) {
Review Comment:
Yeah, I wasn't sure the best way to express this one. I only wanted to perform one update per loop iteration, to avoid the fourth simplify (a -> b -> a -> **b**) if it converged earlier.
Hmm, maybe if I rewrite it as a fixed-length loop with early bail-out instead. I'd initially written it without the `i < 4`, so that it would iterate until convergence, but now that I know how the maximum number of loops required, that flexibility isn't needed anymore.
--
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