[TIR][Arith] Implement kApplyConstraintsToBooleanBranches extension #13129
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
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 by @-ing them in a comment.
Generated by tvm-bot |
areusch
added
needs-triage
csullivan
approved these changes
Oct 27, 2022
src/arith/rewrite_simplify.cc
Outdated
| // 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++) { |
Contributor
There was a problem hiding this 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.
Contributor
Author
There was a problem hiding this 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.
Contributor
Author
There was a problem hiding this comment.
Rewritten, and it definitely is more coherent this way.
Contributor
Author
There was a problem hiding this comment.
And updated in the OrNode handler as well.
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.)
Lunderberg
force-pushed
the
apply_constraints_to_boolean_branches
branch
from
9c67b40 to
fdd7ada
Compare
Contributor
Author
|
Thank you! (Rebased onto main to resolve conflict.) |
xinetzone
pushed a commit
to daobook/tvm
that referenced
this pull request
Nov 10, 2022
…pache#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.)
xinetzone
pushed a commit
to daobook/tvm
that referenced
this pull request
Nov 25, 2022
…pache#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.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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),Amay be simplified on the assumption thatBis true. Similarly, when simplifying(A or B),Amay be simplified on the assumption thatBis 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.)