-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Arith] Simplification of ceil, log2, and left_shift #11646
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
fed5cc6
[TIR] Simplify expressions using tir.ceil and tir.log2
Lunderberg 2cc2661
[Arith] Added left shift handling to ConstIntBoundsAnalyzer
Lunderberg 66687e6
Update to avoid left shift of negative numbers
Lunderberg 86d8165
Updated rewriting of log2(x) to only occur in ceil(log2(x))
Lunderberg 1e4e642
Avoid assumptions made of negative arguments to left-shift
Lunderberg 58b9b7e
Recognize bounds of int(ceil(log2(arg)))
Lunderberg 8e112ba
Merge branch 'main' into simplify_ceil_log2
Lunderberg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if a/b has negative
min_value
, does taking the max narrow the bound? shall we returnEverything
if the bound can't be proved?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this might depend on the target. For the
CodegenC
backend, it doesn't narrow the bounds for any legal use of<<
, because negative arguments are entirely undefined behavior anyways. Taking themax
here is a way to express that same constraint. That is, even if we can't prove that the argument is non-negative, its use in a bitshifting operator provides a constraint.That said, since my primary goal is to improve simplifications from
ceil_log2
, perhaps it would be better to look for aceil(log2(x))
call, and use that directly. The aggressive optimizations that C++ compilers make based on undefined behavior reasoning are controversial for a reason, and I'd like to avoid introducing similar logic in TVM unless required.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given simplification logic like this one are used everywhere, it does merit an extra care. Specialization might make sense to be on the safe side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I had assumed that this was safe, because a similar compile-time check is used for the constant folding in
operator<<
, but in that case the left shift would be entirely removed. By removing a conditional, but still allowing the left shift to remain, we're asserting that we are handling the left shift identically to how it will be at runtime.I've updated the handling of left shift to return
Everything
in the case of potentially negative arguments, and added specific handling forceil(log2(x))
.