[APInt] Fix APInt constructions where value does not fit bitwidth (NFCI) #80309

nikic · 2024-02-01T17:10:45Z

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.

github-actions · 2024-02-02T10:42:54Z

✅ With the latest revision this PR passed the C/C++ code formatter.

nikic · 2024-02-07T16:40:22Z

Before I spend time tracking down all assertion failures, I'd like to have some feedback on the general change.

RKSimon

Is the plan to set implicitTrunc in so many places or is this part of tracking down all the regressions?

RKSimon · 2024-02-15T13:04:52Z

llvm/include/llvm/ADT/APInt.h

      : BitWidth(numBits) {
+    if (!implicitTrunc) {
+      if (BitWidth == 0) {
+        assert(val == 0 && "Value must be zero for 0-bit APInt");


Is it worth putting the if-else conditions inside the assert directly?

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.

nikic · 2024-02-15T14:08:48Z

Is the plan to set implicitTrunc in so many places or is this part of tracking down all the regressions?

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.

nikic · 2024-02-15T16:48:19Z

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

vladimirradosavljevic · 2024-06-19T13:25:46Z

@nikic Thanks for working on this!
I discovered issues in downstream 256-bit target that are fixed by this PR, so are you planning to continue to work on this?
Thanks again for the fixes.

Split out from #80309.

Split off from #80309.

This transform is working on signed integer, so this is the logically correct API. Split off from #80309.

Split off from #80309.

efriedma-quic · 2024-10-15T18:18:54Z

clang/lib/Parse/ParseInit.cpp

@@ -437,7 +437,9 @@ ExprResult Parser::createEmbedExpr() {
  SourceLocation StartLoc = ConsumeAnnotationToken();
  if (Data->BinaryData.size() == 1) {
    Res = IntegerLiteral::Create(Context,
-                                 llvm::APInt(CHAR_BIT, Data->BinaryData.back()),
+                                 llvm::APInt(CHAR_BIT, Data->BinaryData.back(),


llvm::APInt(CHAR_BIT, (uint8_t)Data->BinaryData.back()), maybe?

Done, though I used (unsigned char) to match CHAR_BIT more closely.

efriedma-quic · 2024-10-15T18:23:51Z

clang/lib/Sema/SemaExpr.cpp

+  // TODO: Avoid implicit trunc?
+  return IntegerLiteral::Create(
+      Context,
+      llvm::APInt(IntSize, Val, /*isSigned=*/false, /*implicitTrunc=*/true),


It looks like some of the callers are passing in -1... probably the signature needs to be reworked. Okay to leave it for now.

I've changed this to set isSigned=true and updated the signature to use int64_t instead of uint64_t to clarify that a signed value is expected.

And change argument to int64_t for clarity.

RKSimon

LGTM with one minor

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

…unc (llvm#110466) This fixes all the places in MLIR that hit the new assertion added in llvm#106524, in preparation for enabling it by default. 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, or set the implicitTrunc flag to retain the old behavior. I've left TODOs for the latter case in some places, where I think that it may be worthwhile to stop doing implicit truncation in the future. Note that the assertion is currently still disabled by default, so this patch is mostly NFC. This is just the MLIR changes split off from llvm#80309.

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.

llvm-ci · 2024-10-17T07:34:32Z

LLVM Buildbot has detected a new failure on builder openmp-s390x-linux running on systemz-1 while building clang,lldb,llvm at step 6 "test-openmp".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/3573

Here is the relevant piece of the build log for the reference

Step 6 (test-openmp) failure: test (failure)
******************** TEST 'libomp :: tasking/issue-94260-2.c' FAILED ********************
Exit Code: -11

Command Output (stdout):
--
# RUN: at line 1
/home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp   -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src  -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic && /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# note: command had no output on stdout or stderr
# error: command failed with exit status: -11

--

********************

…unc (llvm#110466) This fixes all the places in MLIR that hit the new assertion added in llvm#106524, in preparation for enabling it by default. 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, or set the implicitTrunc flag to retain the old behavior. I've left TODOs for the latter case in some places, where I think that it may be worthwhile to stop doing implicit truncation in the future. Note that the assertion is currently still disabled by default, so this patch is mostly NFC. This is just the MLIR changes split off from llvm#80309.

…CI) (llvm#80309) This fixes all the places that hit the new assertion added in llvm#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.

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.

This enables the assertion introduced in llvm#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#80309. It is possible to opt-out of the check and restore the previous behavior by setting implicitTrunc=true.

…unc (llvm#110466) This fixes all the places in MLIR that hit the new assertion added in llvm#106524, in preparation for enabling it by default. 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, or set the implicitTrunc flag to retain the old behavior. I've left TODOs for the latter case in some places, where I think that it may be worthwhile to stop doing implicit truncation in the future. Note that the assertion is currently still disabled by default, so this patch is mostly NFC. This is just the MLIR changes split off from llvm#80309.

…CI) (llvm#80309) This fixes all the places that hit the new assertion added in llvm#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.

This enables the assertion introduced in llvm#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#80309. It is possible to opt-out of the check and restore the previous behavior by setting implicitTrunc=true.

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.

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.

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 mentioned this pull request Feb 1, 2024

WRONG code: LoopUnroll / SCEVExpander with i128 induction variable. #80289

Closed

nikic force-pushed the apint-assert branch from f56c096 to 506aaa1 Compare February 2, 2024 10:40

nikic force-pushed the apint-assert branch 2 times, most recently from 95f49c5 to c7b2f06 Compare February 7, 2024 16:35

nikic requested review from fhahn, preames, dtcxzyw and efriedma-quic February 7, 2024 16:36

nikic requested a review from RKSimon February 15, 2024 12:12

RKSimon reviewed Feb 15, 2024

View reviewed changes

DataCorrupted self-requested a review March 13, 2024 01:06

vladimirradosavljevic mentioned this pull request Jun 19, 2024

Cherry pick fixes for large signed constant truncation matter-labs/era-compiler-llvm#630

Merged

nikic force-pushed the apint-assert branch from c7b2f06 to f5cc0bf Compare June 20, 2024 13:30

nikic mentioned this pull request Aug 9, 2024

[SCEV] WRONG code #102597

Closed

nikic added a commit that referenced this pull request Aug 12, 2024

[OMPIRBuilder] Use getAllOnesValue()

dc831e8

Split out from #80309.

nikic added a commit that referenced this pull request Aug 12, 2024

[ConstantFolding] Use getSigned()

246c236

Split out from #80309.

nikic added a commit that referenced this pull request Aug 12, 2024

[CGP] Use getAllOnesValue()

3b27fce

Split out from #80309.

nikic added a commit that referenced this pull request Aug 12, 2024

[ExpandMemCmp] Use getAllOnesValue()

06f64e8

Split out from #80309.

nikic added a commit that referenced this pull request Aug 12, 2024

[ConstraintElimination] Use getAllOnesValue()

73d835c

Split out from #80309.

nikic force-pushed the apint-assert branch from f5cc0bf to ca2fc22 Compare August 12, 2024 14:38

nikic mentioned this pull request Aug 13, 2024

[SelectionDAG] Move SelectionDAG::getAllOnesConstant out of line. NFC #102995

Merged

nikic added a commit that referenced this pull request Aug 13, 2024

[MemoryBuiltins] Use getAllOnesValue()

53c41f9

Split out from #80309.

nikic added a commit that referenced this pull request Aug 13, 2024

[InstCombine] Use APInt::getSplat()

4d97ad5

Split off from #80309.

nikic added a commit that referenced this pull request Aug 13, 2024

[IndVars] Use getSigned() in FP transform

7990a7e

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 Aug 13, 2024

[InstCombine] Use getAllOnesValue()

adb4cfe

Split off from #80309.

nikic force-pushed the apint-assert branch from ab1d3f2 to 665720f Compare October 15, 2024 08:24

efriedma-quic reviewed Oct 15, 2024

View reviewed changes

nikic added 3 commits October 16, 2024 12:06

Cast to unsigned char instead

2dba028

Set isSigned=true for ActOnIntegerConstant

99d7e27

And change argument to int64_t for clarity.

Reference issue for implicit trunc todos

38e2027

nikic requested a review from Endilll as a code owner October 16, 2024 10:19

Endilll removed their request for review October 16, 2024 10:23

Fix clang-format

796f5ef

RKSimon approved these changes Oct 16, 2024

View reviewed changes

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp Show resolved Hide resolved

Use maxUIntN

1851f82

nikic merged commit 255a99c into llvm:main Oct 17, 2024
8 checks passed

nikic deleted the apint-assert branch October 17, 2024 06:48

nikic mentioned this pull request Oct 17, 2024

[APInt] Enable APInt ctor assertion by default #112670

Merged

nikic mentioned this pull request Nov 1, 2024

Reapply [APInt] Enable APInt ctor assertion by default #114539

Merged

pavelverigo mentioned this pull request Nov 23, 2024

Setting a global enum on WASM fails silently ziglang/zig#21558

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APInt] Fix APInt constructions where value does not fit bitwidth (NFCI) #80309

[APInt] Fix APInt constructions where value does not fit bitwidth (NFCI) #80309

nikic commented Feb 1, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading

nikic commented Feb 7, 2024

RKSimon left a comment

RKSimon Feb 15, 2024

nikic Feb 15, 2024

nikic commented Feb 15, 2024

nikic commented Feb 15, 2024

vladimirradosavljevic commented Jun 19, 2024

efriedma-quic Oct 15, 2024

nikic Oct 16, 2024

efriedma-quic Oct 15, 2024

nikic Oct 16, 2024

RKSimon left a comment

llvm-ci commented Oct 17, 2024

[APInt] Fix APInt constructions where value does not fit bitwidth (NFCI) #80309

[APInt] Fix APInt constructions where value does not fit bitwidth (NFCI) #80309

Conversation

nikic commented Feb 1, 2024 • edited Loading

github-actions bot commented Feb 2, 2024 • edited Loading

nikic commented Feb 7, 2024

RKSimon left a comment

Choose a reason for hiding this comment

RKSimon Feb 15, 2024

Choose a reason for hiding this comment

nikic Feb 15, 2024

Choose a reason for hiding this comment

nikic commented Feb 15, 2024

nikic commented Feb 15, 2024

vladimirradosavljevic commented Jun 19, 2024

efriedma-quic Oct 15, 2024

Choose a reason for hiding this comment

nikic Oct 16, 2024

Choose a reason for hiding this comment

efriedma-quic Oct 15, 2024

Choose a reason for hiding this comment

nikic Oct 16, 2024

Choose a reason for hiding this comment

RKSimon left a comment

Choose a reason for hiding this comment

llvm-ci commented Oct 17, 2024

nikic commented Feb 1, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading