[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.

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

nikic · 2024-10-14T13:03:33Z

Ping. The PR no longer contains MLIR changes now.

dtcxzyw

Middle-end/CodeGen/RISC-V changes LGTM.

llvm/lib/Target/Hexagon/HexagonGenExtract.cpp

dtcxzyw · 2024-10-14T14:40:19Z

APInt::getAllOnes will assert if BitWidth == 0.

[----------] 6 tests from APFloatTest
[ RUN      ] APFloatTest.MinimumNumber
[       OK ] APFloatTest.MinimumNumber (0 ms)
[ RUN      ] APFloatTest.Float8E8M0FNUValues
ADTTests: /home/dtcxzyw/WorkSpace/Projects/compilers/llvm-project/llvm/include/llvm/ADT/APInt.h:116: llvm::APInt::APInt(unsigned int, uint64_t, bool, bool): Assertion `val == 0 && "Value must be zero for 0-bit APInt"' failed.
 #0 0x00007ffff7e15c32 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/lib/libLLVMSupport.so.20.0git+0x215c32)
 #1 0x00007ffff7e12aff llvm::sys::RunSignalHandlers() (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/lib/libLLVMSupport.so.20.0git+0x212aff)
 #2 0x00007ffff7e12c45 SignalHandler(int) Signals.cpp:0:0
 #3 0x00007ffff7442520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #4 0x00007ffff74969fc __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #5 0x00007ffff74969fc __pthread_kill_internal ./nptl/pthread_kill.c:78:10
 #6 0x00007ffff74969fc pthread_kill ./nptl/pthread_kill.c:89:10
 #7 0x00007ffff7442476 gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #8 0x00007ffff74287f3 abort ./stdlib/abort.c:81:7
 #9 0x00007ffff742871b _nl_load_domain ./intl/loadmsgcat.c:1177:9
#10 0x00007ffff7439e96 (/lib/x86_64-linux-gnu/libc.so.6+0x39e96)
#11 0x00007ffff7c97cb6 (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/lib/libLLVMSupport.so.20.0git+0x97cb6)
#12 0x00007ffff7c98b83 llvm::detail::IEEEFloat::makeNaN(bool, bool, llvm::APInt const*) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/lib/libLLVMSupport.so.20.0git+0x98b83)
#13 0x00007ffff7c9e019 llvm::detail::IEEEFloat::convertFromStringSpecials(llvm::StringRef) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/lib/libLLVMSupport.so.20.0git+0x9e019)
#14 0x00007ffff7caef9d llvm::detail::IEEEFloat::convertFromString(llvm::StringRef, llvm::RoundingMode) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/lib/libLLVMSupport.so.20.0git+0xaef9d)
#15 0x00007ffff7caf4f6 llvm::APFloat::APFloat(llvm::fltSemantics const&, llvm::StringRef) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/lib/libLLVMSupport.so.20.0git+0xaf4f6)
#16 0x000055555585e240 (anonymous namespace)::APFloatTest_Float8E8M0FNUValues_Test::TestBody() APFloatTest.cpp:0:0
#17 0x00007ffff7f6e9d1 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/lib/libllvm_gtest.so.20.0git+0x569d1)
#18 0x00007ffff7f6ef2a testing::Test::Run() (.part.0) gtest-all.cc:0:0
#19 0x00007ffff7f6fc02 testing::TestInfo::Run() (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/lib/libllvm_gtest.so.20.0git+0x57c02)
#20 0x00007ffff7f721e9 testing::TestSuite::Run() (.part.0) gtest-all.cc:0:0
#21 0x00007ffff7f7c18b testing::internal::UnitTestImpl::RunAllTests() (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/lib/libllvm_gtest.so.20.0git+0x6418b)
#22 0x00007ffff7f6e441 testing::UnitTest::Run() (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/lib/libllvm_gtest.so.20.0git+0x56441)
#23 0x00007ffff7fb71cc main (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/lib/libllvm_gtest_main.so.20.0git+0x11cc)
#24 0x00007ffff7429d90 __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#25 0x00007ffff7429e40 call_init ./csu/../csu/libc-start.c:128:20
#26 0x00007ffff7429e40 __libc_start_main ./csu/../csu/libc-start.c:379:5
#27 0x00005555557b7075 _start (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/unittests/ADT/./ADTTests+0x263075)

Would be better to handle this corner case in IEEEFloat::makeNaN:

llvm-project/llvm/lib/Support/APFloat.cpp

Line 966 in 81fee74

fill_storage = APInt::getAllOnes(semantics->precision - 1);

davemgreen · 2024-10-15T06:13:58Z

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

@@ -1159,7 +1159,9 @@ class ARMOperand : public MCParsedAsmOperand {
    if (!isImm()) return false;
    const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
    if (!CE) return false;


Can this check that CE->getActiveBits() > 32?

Not quite, but this put me on the right path. I added a check for isUInt<32> and dropped the signed flag.

nikic · 2024-10-15T07:57:22Z

APInt::getAllOnes will assert if BitWidth == 0.

[snip]

For the record, this is fixed by #112227.

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.

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 mentioned this pull request Sep 30, 2024

[MLIR] Update APInt construction to correctly set isSigned/implicitTrunc #110466

Merged

nikic force-pushed the apint-assert branch from 17ba7e8 to ab1d3f2 Compare October 14, 2024 13:02

dtcxzyw reviewed Oct 14, 2024

View reviewed changes

RKSimon reviewed Oct 14, 2024

View reviewed changes

llvm/lib/Target/Hexagon/HexagonGenExtract.cpp Show resolved Hide resolved

davemgreen reviewed Oct 15, 2024

View reviewed changes

nikic added 2 commits October 15, 2024 09:57

apint only

9679519

check for 32-bit immediate instead

665720f

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

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

nikic commented Oct 14, 2024

dtcxzyw left a comment

dtcxzyw commented Oct 14, 2024

davemgreen Oct 15, 2024

nikic Oct 15, 2024

nikic commented Oct 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Feb 15, 2024

nikic commented Feb 15, 2024

vladimirradosavljevic commented Jun 19, 2024

nikic commented Oct 14, 2024

dtcxzyw left a comment

Choose a reason for hiding this comment

dtcxzyw commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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