[APInt] Fix APInt constructions where value does not fit bitwidth (NFCI) #80309
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
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
nikic
force-pushed
the
apint-assert
branch
2 times, most recently
from
95f49c5 to
c7b2f06
Compare
|
Before I spend time tracking down all assertion failures, I'd like to have some feedback on the general change. |
RKSimon
reviewed
Feb 15, 2024
llvm/include/llvm/ADT/APInt.h
Outdated
| : BitWidth(numBits) { | ||
| if (!implicitTrunc) { | ||
| if (BitWidth == 0) { | ||
| assert(val == 0 && "Value must be zero for 0-bit APInt"); |
There was a problem hiding this comment.
Is it worth putting the if-else conditions inside the assert directly?
There was a problem hiding this comment.
As the conditions end up fairly complex, I figured it's better to have if/else. If assert() expands to nothing, they'll get optimized away.
Apart from a couple marked TODO, most of them are here to stay. E.g. that bit of code in ConstantFolding has "sext or trunc" semantics, so doing something like "isSigned = true, implicitTrunc = true" there makes sense. Some of them could be avoided, but it's not really clear that it's worthwhile/desirable. E.g. the one in FuzzMutate/OpDescriptor really doesn't really care about the actual value, so doing a truncation is as good as anything else. |
|
As a side note, it seems like not having the implicit truncation also improves compile-time: http://llvm-compile-time-tracker.com/compare.php?from=fd191378dce6b20c100da716f94130af2593df37&to=df515f3e94d5f5d10ecddfb60181d8866817cc27&stat=instructions:u |
|
@nikic Thanks for working on this! |
Closed
nikic
added a commit
that referenced
this pull request
Aug 13, 2024
This transform is working on signed integer, so this is the logically correct API. Split off from #80309.
nikic
added a commit
that referenced
this pull request
Oct 18, 2024
This enables the assertion introduced in #106524, which checks that the value passed to the APInt constructor is indeed a valid N-bit signed or unsigned integer. Places that previously violated the assertion were updated in advance, e.g. in #80309. It is possible to opt-out of the check and restore the previous behavior by setting implicitTrunc=true.
nikic
added a commit
to nikic/llvm-project
that referenced
this pull request
Nov 1, 2024
This enables the assertion introduced in llvm#106524, which checks that the value passed to the constructor is indeed a valid N-bit signed or unsigned integer. Places that previously violated the assertion were updated in advance, e.g. in llvm#80309. It is possible to opt-out of the check and restore the previous behavior by setting implicitTrunc=true. ----- The buildbot failures from the previous attempt should be fixed by a18dd29 and e2074c6.
nikic
added a commit
that referenced
this pull request
Nov 1, 2024
This enables the assertion introduced in #106524, which checks that the value passed to the constructor is indeed a valid N-bit signed or unsigned integer. Places that previously violated the assertion were updated in advance, e.g. in #80309. It is possible to opt-out of the check and restore the previous behavior by setting implicitTrunc=true. ----- The buildbot failures from the previous attempt should be fixed by a18dd29 and e2074c6.
smallp-o-p
pushed a commit
to smallp-o-p/llvm-project
that referenced
this pull request
Nov 3, 2024
This enables the assertion introduced in llvm#106524, which checks that the value passed to the constructor is indeed a valid N-bit signed or unsigned integer. Places that previously violated the assertion were updated in advance, e.g. in llvm#80309. It is possible to opt-out of the check and restore the previous behavior by setting implicitTrunc=true. ----- The buildbot failures from the previous attempt should be fixed by a18dd29 and e2074c6.
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
This enables the assertion introduced in llvm#106524, which checks that the value passed to the constructor is indeed a valid N-bit signed or unsigned integer. Places that previously violated the assertion were updated in advance, e.g. in llvm#80309. It is possible to opt-out of the check and restore the previous behavior by setting implicitTrunc=true. ----- The buildbot failures from the previous attempt should be fixed by a18dd29 and e2074c6.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
Split out from llvm/llvm-project#80309.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
Split out from llvm/llvm-project#80309.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
Split out from llvm/llvm-project#80309.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
Split out from llvm/llvm-project#80309.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
Split out from llvm/llvm-project#80309.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
This transform is working on signed integer, so this is the logically correct API. Split off from llvm/llvm-project#80309.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
Split off from llvm/llvm-project#80309.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
Split off from llvm/llvm-project#80309.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
The -1 constant should be sign extended, not zero extended. Split out from llvm/llvm-project#80309.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
We were missing the signed flag on the negative value, so the range was incorrectly interpreted for integers larger than 64-bit. Split out from llvm/llvm-project#80309.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
Split out from llvm/llvm-project#80309 to avoid assertion failures in the future.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
Split out from llvm/llvm-project#80309 to avoid assertion failures in the future.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
This handles the edge case where BitWidth is 1 and doing the increment gets a value that's not valid in that width, while we just want wrap-around. Split out of llvm/llvm-project#80309.
akiramenai
pushed a commit
to matter-labs/era-compiler-llvm
that referenced
this pull request
Apr 8, 2025
This enables the assertion introduced in llvm/llvm-project#106524, which checks that the value passed to the APInt constructor is indeed a valid N-bit signed or unsigned integer. Places that previously violated the assertion were updated in advance, e.g. in llvm/llvm-project#80309. It is possible to opt-out of the check and restore the previous behavior by setting implicitTrunc=true.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes all the places that hit the new assertion added in #106524 in tests. That is, cases where the value passed to the APInt constructor is not an N-bit signed/unsigned integer, where N is the bit width and signedness is determined by the isSigned flag.
The fixes either set the correct value for isSigned, set the implicitTrunc flag, or perform more calculations inside APInt.
Note that the assertion is currently still disabled by default, so this patch is mostly NFC.