-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[ValueTracking] Improve KnownBits for signed min-max clamping #120576
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
Changes from 4 commits
cb2efa6
54b6643
a72e960
6636dc6
5422719
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,263 @@ | ||||||
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5 | ||||||
| ; RUN: opt < %s -passes=aggressive-instcombine -mtriple=x86_64 -S | FileCheck %s | ||||||
dtcxzyw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| ; RUN: opt < %s -passes=aggressive-instcombine -mtriple=x86_64 -S | FileCheck %s | |
| ; RUN: opt < %s -passes=aggressive-instcombine -S | FileCheck %s |
Please add an appropriate data layout instead, something like target datalayout = "n:32:16:8" probably works.
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.
Added target datalayout = "n8:16:32", removed -mtriple.
Uh oh!
There was an error while loading. Please reload this page.
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.
The ConstantRange constructor will assert if CLow and CHigh+1 are the same value.
For example, this unittest I threw together
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.
Nice catch, this should be using
ConstantRange::getNonEmpty().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.
This is indeed a bug, I should have added "if" from the very beginning. I made the correction and added further LIT tests to check if everything works fine. Also, it turned out that TruncInstCombine.cpp uses the constraint from the min-max clamp only for some and not all binary operators. I used lshr in the lit tests and this definitely fires the min-max clamp constraint.
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.
Here's the fix: #121206. I'm still playing with some tests. Thanks for the comment and sorry for the trouble.
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.
OK, I added the fix and some additional LIT tests in #121206. I added more explanations there.
@nikic If isSignedMinMaxIntrinsicClamp returned true, the range is valid, i.e., *CLow <= *CRight. Thus, *CLow < *CRight + 1, except when *CRight is the max signed value. In such a case *CRight + 1 = min signed value and this is still ok if *CLow is larger than min signed value (the range is a valid interval from *CLow to max signed value).
The problem is when CLow->isMinSignedValue() && CHigh->isMaxSignedValue(). The constructor of ConstantRange has the asserion:
assert((Lower != Upper || (Lower.isMaxValue() || Lower.isMinValue())) &&
"Lower == Upper, but they aren't min or max value!");
with Lower.isMinValue() and not Lower.isMinSignedValue(). And so it fails.