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

nikic · 2024-11-01T13:48:52Z

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.

llvmbot · 2024-11-01T13:49:25Z

@llvm/pr-subscribers-llvm-adt

Author: Nikita Popov (nikic)

Changes

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.

Full diff: https://github.com/llvm/llvm-project/pull/114539.diff

1 Files Affected:

(modified) llvm/include/llvm/ADT/APInt.h (+1-1)

diff --git a/llvm/include/llvm/ADT/APInt.h b/llvm/include/llvm/ADT/APInt.h
index 63a138527b32e1..953b2a27b71526 100644
--- a/llvm/include/llvm/ADT/APInt.h
+++ b/llvm/include/llvm/ADT/APInt.h
@@ -109,7 +109,7 @@ class [[nodiscard]] APInt {
   /// \param implicitTrunc allow implicit truncation of non-zero/sign bits of
   ///                      val beyond the range of numBits
   APInt(unsigned numBits, uint64_t val, bool isSigned = false,
-        bool implicitTrunc = true)
+        bool implicitTrunc = false)
       : BitWidth(numBits) {
     if (!implicitTrunc) {
       if (isSigned) {

When trying to create a const inst from a 32 bit signed value, we don't want to sign-extend it to 64 bits, as the resulting value won't actually fit in an `i32` if it was negative. This fixes crashes in the following two tests after the APInt constructor asserts were enabled in llvm#114539: ``` Failed Tests (2): LLVM :: CodeGen/SPIRV/transcoding/RelationalOperators.ll LLVM :: CodeGen/SPIRV/uitofp-with-bool.ll ```

nathanchance · 2024-11-02T14:30:15Z

I am seeing an assertion failure while building the Linux kernel after this change. cvise spits out as a trivial reproducer:

int am33xx_check_features___trans_tmp_5;
int omap_rev();
void __attribute__((__cold__)) am33xx_check_features() {
  am33xx_check_features___trans_tmp_5 = omap_rev() >> 20 == 363;
}

$ clang --target=arm-linux-gnueabi -O2 -c -o /dev/null id.i
clang: /home/nathan/tmp/cvise.iMmMYHOwp2/src/llvm/include/llvm/ADT/APInt.h:121: llvm::APInt::APInt(unsigned int, uint64_t, bool, bool): Assertion `llvm::isIntN(BitWidth, val) && "Value is not an N-bit signed value"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: clang --target=arm-linux-gnueabi -O2 -c -o /dev/null id.i
1.      <eof> parser at end of file
2.      Code generation
3.      Running pass 'Function Pass Manager' on module 'id.i'.
4.      Running pass 'Constant Hoisting' on function '@am33xx_check_features'
...

If a separate issue would be helpful, I am happy to file one.

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 · 2024-11-04T09:58:55Z

@nathanchance Thanks for the report! Here's a minimized test case:

; RUN: opt -S -passes=consthoist < %s
target triple = "armv4t-unknown-linux-gnueabi"

define i1 @test(i32 %arg) optsize {
entry:
  %shr.mask = and i32 %arg, -1048576
  %cmp = icmp eq i32 %shr.mask, 380633088
  ret i1 %cmp
}

The result here may require truncation. Fix this by removing the calculateOffsetDiff() helper entirely. As far as I can tell, this code does not actually have to deal with different bitwidths. findBaseConstants() will produce ranges of constants with equal types, which is what maximizeConstantsInRange() will then work on. Fixes assertion reported at: #114539 (comment)

nikic · 2024-11-04T10:17:50Z

@nathanchance I've applied a fix in 8851ea6. Let me know if you see further issues.

When trying to create a const inst from a 32 bit signed value, we don't want to sign-extend it to 64 bits, as the resulting value won't actually fit in an `i32` if it was negative. This fixes crashes in the following two tests after the APInt constructor asserts were enabled in #114539: ``` Failed Tests (2): LLVM :: CodeGen/SPIRV/transcoding/RelationalOperators.ll LLVM :: CodeGen/SPIRV/uitofp-with-bool.ll ```

…stant. NFC (#114336) This assert is also present inside the APInt constructor after #114539.

mikaelholmen · 2024-11-05T06:20:59Z

Hi @nikic ,

Another crash like this:
https://godbolt.org/z/57chn4aqP

; RUN: opt -passes=correlated-propagation -S < %s
define i16 @main() {
entry:
  %rem432 = urem i1 false, false
  ret i16 0
}

nikic · 2024-11-05T09:04:31Z

@mikaelholmen Thanks, should be fixed by 46ccefb.

The result here may require truncation. Fix this by removing the calculateOffsetDiff() helper entirely. As far as I can tell, this code does not actually have to deal with different bitwidths. findBaseConstants() will produce ranges of constants with equal types, which is what maximizeConstantsInRange() will then work on. Fixes assertion reported at: llvm#114539 (comment)

…4630) When trying to create a const inst from a 32 bit signed value, we don't want to sign-extend it to 64 bits, as the resulting value won't actually fit in an `i32` if it was negative. This fixes crashes in the following two tests after the APInt constructor asserts were enabled in llvm#114539: ``` Failed Tests (2): LLVM :: CodeGen/SPIRV/transcoding/RelationalOperators.ll LLVM :: CodeGen/SPIRV/uitofp-with-bool.ll ```

…stant. NFC (llvm#114336) This assert is also present inside the APInt constructor after llvm#114539.

mikaelholmen · 2024-11-19T13:59:39Z

Another one here:
opt -passes=instcombine bbi-101454.ll -S -o /dev/null
results in

opt: ../include/llvm/ADT/APInt.h:128: llvm::APInt::APInt(unsigned int, uint64_t, bool, bool): Assertion `llvm::isUIntN(BitWidth, val) && "Value is not an N-bit unsigned value"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: build-all/bin/opt -passes=instcombine bbi-101454.ll -S -o /dev/null
1.	Running pass "function(instcombine<max-iterations=1;verify-fixpoint>)" on module "bbi-101454.ll"
2.	Running pass "instcombine<max-iterations=1;verify-fixpoint>" on function "f_434_1400"
 #0 0x000055a682c8a778 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (build-all/bin/opt+0x4444778)
 #1 0x000055a682c8823e llvm::sys::RunSignalHandlers() (build-all/bin/opt+0x444223e)
 #2 0x000055a682c8afad SignalHandler(int) Signals.cpp:0:0
 #3 0x00007f622ac1dcf0 __restore_rt (/lib64/libpthread.so.0+0x12cf0)
 #4 0x00007f62287d6acf raise (/lib64/libc.so.6+0x4eacf)
 #5 0x00007f62287a9ea5 abort (/lib64/libc.so.6+0x21ea5)
 #6 0x00007f62287a9d79 _nl_load_domain.cold.0 (/lib64/libc.so.6+0x21d79)
 #7 0x00007f62287cf426 (/lib64/libc.so.6+0x47426)
 #8 0x000055a6839925cc llvm::InstCombinerImpl::canonicalizeCondSignextOfHighBitExtractToSignextHighBitExtract(llvm::BinaryOperator&) InstCombineAddSub.cpp:0:0
 #9 0x000055a683993d90 llvm::InstCombinerImpl::visitAdd(llvm::BinaryOperator&) InstCombineAddSub.cpp:0:0
#10 0x000055a6838e6f1a llvm::InstCombinerImpl::run() InstructionCombining.cpp:0:0
#11 0x000055a6838ea7bd combineInstructionsOverFunction(llvm::Function&, llvm::InstructionWorklist&, llvm::AAResults*, llvm::AssumptionCache&, llvm::TargetLibraryInfo&, llvm::TargetTransformInfo&, llvm::DominatorTree&, llvm::OptimizationRemarkEmitter&, llvm::BlockFrequencyInfo*, llvm::BranchProbabilityInfo*, llvm::ProfileSummaryInfo*, llvm::InstCombineOptions const&) InstructionCombining.cpp:0:0
#12 0x000055a6838e9b36 llvm::InstCombinePass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (build-all/bin/opt+0x50a3b36)
#13 0x000055a684042a3d llvm::detail::PassModel<llvm::Function, llvm::InstCombinePass, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) PassBuilderPipelines.cpp:0:0
#14 0x000055a682e93307 llvm::PassManager<llvm::Function, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (build-all/bin/opt+0x464d307)
#15 0x000055a68404877d llvm::detail::PassModel<llvm::Function, llvm::PassManager<llvm::Function, llvm::AnalysisManager<llvm::Function>>, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) PassBuilderPipelines.cpp:0:0
#16 0x000055a682e97e96 llvm::ModuleToFunctionPassAdaptor::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (build-all/bin/opt+0x4651e96)
#17 0x000055a68404221d llvm::detail::PassModel<llvm::Module, llvm::ModuleToFunctionPassAdaptor, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) PassBuilderPipelines.cpp:0:0
#18 0x000055a682e92037 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (build-all/bin/opt+0x464c037)
#19 0x000055a683fe6373 llvm::runPassPipeline(llvm::StringRef, llvm::Module&, llvm::TargetMachine*, llvm::TargetLibraryInfoImpl*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::StringRef, llvm::ArrayRef<llvm::PassPlugin>, llvm::ArrayRef<std::function<void (llvm::PassBuilder&)>>, llvm::opt_tool::OutputKind, llvm::opt_tool::VerifierKind, bool, bool, bool, bool, bool, bool, bool) (build-all/bin/opt+0x57a0373)
#20 0x000055a682c51b4a optMain (build-all/bin/opt+0x440bb4a)
#21 0x00007f62287c2d85 __libc_start_main (/lib64/libc.so.6+0x3ad85)
#22 0x000055a682c4b96e _start (build-all/bin/opt+0x440596e)
Abort (core dumped)

bbi-101454.ll.gz

The (extended) bit width might not fit into the (non-extended) type, resulting in an incorrect truncation of the compared value. Fix this by using m_SpecificInt(), which is both simpler and handles this correctly. Fixes the assertion failure reported in: #114539 (comment)

nikic · 2024-11-19T14:18:32Z

@mikaelholmen Thanks, should be fixed by abac5be.

mikaelholmen · 2024-11-20T06:15:39Z

Yep, thanks!

The (extended) bit width might not fit into the (non-extended) type, resulting in an incorrect truncation of the compared value. Fix this by using m_SpecificInt(), which is both simpler and handles this correctly. Fixes the assertion failure reported in: llvm#114539 (comment)

…114539)" This reverts commit 3494ee9. This change has exposed a bug in LLPC. Reverting for now until fixed.

…9808c3cce Local branch amd-gfx 96d9808 Revert "Reapply [APInt] Enable APInt ctor assertion by default (llvm#114539)" Remote branch main 2de3d00 [mlir][tosa] Fix a crash in `PadOp::fold` (llvm#114921)

mikaelholmen · 2025-01-07T09:49:46Z

Hi @nikic ,

Another failure like this:
opt -passes instcombine bbi-102622.ll -o /dev/null
Result:

opt: ../include/llvm/ADT/APInt.h:121: llvm::APInt::APInt(unsigned int, uint64_t, bool, bool): Assertion `llvm::isIntN(BitWidth, val) && "Value is not an N-bit signed value"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: build-all/bin/opt -passes instcombine bbi-102622.ll -o /dev/null
1.	Running pass "function(instcombine<max-iterations=1;verify-fixpoint>)" on module "bbi-102622.ll"
2.	Running pass "instcombine<max-iterations=1;verify-fixpoint>" on function "test_max_1"
 #0 0x0000564c1b952046 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (build-all/bin/opt+0x448b046)
 #1 0x0000564c1b94fa8e llvm::sys::RunSignalHandlers() (build-all/bin/opt+0x4488a8e)
 #2 0x0000564c1b9528f9 SignalHandler(int) Signals.cpp:0:0
 #3 0x00007fd7d9e41cf0 __restore_rt (/lib64/libpthread.so.0+0x12cf0)
 #4 0x00007fd7d77f2acf raise (/lib64/libc.so.6+0x4eacf)
 #5 0x00007fd7d77c5ea5 abort (/lib64/libc.so.6+0x21ea5)
 #6 0x00007fd7d77c5d79 _nl_load_domain.cold.0 (/lib64/libc.so.6+0x21d79)
 #7 0x00007fd7d77eb426 (/lib64/libc.so.6+0x47426)
 #8 0x0000564c1c680f7b (build-all/bin/opt+0x51b9f7b)
 #9 0x0000564c1c67e98b llvm::InstCombinerImpl::foldICmpAddConstant(llvm::ICmpInst&, llvm::BinaryOperator*, llvm::APInt const&) InstCombineCompares.cpp:0:0
#10 0x0000564c1c6828cc llvm::InstCombinerImpl::foldICmpBinOpWithConstant(llvm::ICmpInst&, llvm::BinaryOperator*, llvm::APInt const&) InstCombineCompares.cpp:0:0
#11 0x0000564c1c6824c8 llvm::InstCombinerImpl::foldICmpInstWithConstant(llvm::ICmpInst&) InstCombineCompares.cpp:0:0
#12 0x0000564c1c692d2e llvm::InstCombinerImpl::visitICmpInst(llvm::ICmpInst&) InstCombineCompares.cpp:0:0
#13 0x0000564c1c5fa12b llvm::InstCombinerImpl::run() InstructionCombining.cpp:0:0
#14 0x0000564c1c5fd9fd combineInstructionsOverFunction(llvm::Function&, llvm::InstructionWorklist&, llvm::AAResults*, llvm::AssumptionCache&, llvm::TargetLibraryInfo&, llvm::TargetTransformInfo&, llvm::DominatorTree&, llvm::OptimizationRemarkEmitter&, llvm::BlockFrequencyInfo*, llvm::BranchProbabilityInfo*, llvm::ProfileSummaryInfo*, llvm::InstCombineOptions const&) InstructionCombining.cpp:0:0
#15 0x0000564c1c5fcd7a llvm::InstCombinePass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (build-all/bin/opt+0x5135d7a)
#16 0x0000564c1cda10ed llvm::detail::PassModel<llvm::Function, llvm::InstCombinePass, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) PassBuilderPipelines.cpp:0:0
#17 0x0000564c1bb71957 llvm::PassManager<llvm::Function, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (build-all/bin/opt+0x46aa957)
#18 0x0000564c1cda8b7d llvm::detail::PassModel<llvm::Function, llvm::PassManager<llvm::Function, llvm::AnalysisManager<llvm::Function>>, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) PassBuilderPipelines.cpp:0:0
#19 0x0000564c1bb7652e llvm::ModuleToFunctionPassAdaptor::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (build-all/bin/opt+0x46af52e)
#20 0x0000564c1cda064d llvm::detail::PassModel<llvm::Module, llvm::ModuleToFunctionPassAdaptor, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) PassBuilderPipelines.cpp:0:0
#21 0x0000564c1bb70647 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (build-all/bin/opt+0x46a9647)
#22 0x0000564c1cd28bbc llvm::runPassPipeline(llvm::StringRef, llvm::Module&, llvm::TargetMachine*, llvm::TargetLibraryInfoImpl*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::StringRef, llvm::ArrayRef<llvm::PassPlugin>, llvm::ArrayRef<std::function<void (llvm::PassBuilder&)>>, llvm::opt_tool::OutputKind, llvm::opt_tool::VerifierKind, bool, bool, bool, bool, bool, bool, bool) (build-all/bin/opt+0x5861bbc)
#23 0x0000564c1b914c90 optMain (build-all/bin/opt+0x444dc90)
#24 0x00007fd7d77ded85 __libc_start_main (/lib64/libc.so.6+0x3ad85)
#25 0x0000564c1b9128ae _start (build-all/bin/opt+0x444b8ae)
Abort (core dumped)

It fails on this in InstCombinerImpl::foldICmpAddConstant:

    auto ComputeTable = [&](bool Op0Val, bool Op1Val) {
      int Res = 0;
      if (Op0Val)
        Res += isa<ZExtInst>(Ext0) ? 1 : -1;
      if (Op1Val)
        Res += isa<ZExtInst>(Ext1) ? 1 : -1;
      return ICmpInst::compare(APInt(BW, Res, true), C, Pred);
    };

bbi-102622.ll.gz

If the bitwidth is 2 and we add two 1s, the result may overflow. This is fine in terms of correctness, but triggers the APInt ctor assertion. Fix this by performing the calculation directly on APInts. Fixes the issue reported in: #114539 (comment)

nikic · 2025-01-07T11:13:50Z

@mikaelholmen Thanks, should be fixed by 63d4e0f.

mikaelholmen · 2025-01-07T13:32:52Z

@mikaelholmen Thanks, should be fixed by 63d4e0f.

Yep, thanks!

If the bitwidth is 2 and we add two 1s, the result may overflow. This is fine in terms of correctness, but triggers the APInt ctor assertion. Fix this by performing the calculation directly on APInts. Fixes the issue reported in: llvm/llvm-project#114539 (comment)

nikic added the skip-precommit-approval PR for CI feedback, not intended for review label Nov 1, 2024

llvmbot added the llvm:adt label Nov 1, 2024

nikic merged commit 3494ee9 into llvm:main Nov 1, 2024
11 checks passed

nikic deleted the apint-assert-enable-2 branch November 1, 2024 15:23

bogner mentioned this pull request Nov 2, 2024

[SPIRV] Fix assert in getOrCreateBaseRegister for i32 -1 #114630

Merged

nikic mentioned this pull request Nov 4, 2024

[SelectionDAG] Remove unneeded assert from SelectionDAG::getSignedConstant. NFC #114336

Merged

topperc added a commit that referenced this pull request Nov 4, 2024

[SelectionDAG] Remove unneeded assert from SelectionDAG::getSignedCon…

97b7474

…stant. NFC (#114336) This assert is also present inside the APInt constructor after #114539.

PhilippRados pushed a commit to PhilippRados/llvm-project that referenced this pull request Nov 6, 2024

[SelectionDAG] Remove unneeded assert from SelectionDAG::getSignedCon…

c5e1343

…stant. NFC (llvm#114336) This assert is also present inside the APInt constructor after llvm#114539.

hamptonm1 mentioned this pull request Nov 18, 2024

Update llvm-project to llvm/llvm-project@01d233ff403823389f848 onnx/onnx-mlir#3011

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nikic commented Nov 1, 2024 •

edited

Loading

llvmbot commented Nov 1, 2024

nathanchance commented Nov 2, 2024

nikic commented Nov 4, 2024

nikic commented Nov 4, 2024

mikaelholmen commented Nov 5, 2024 •

edited

Loading

nikic commented Nov 5, 2024

mikaelholmen commented Nov 19, 2024

nikic commented Nov 19, 2024

mikaelholmen commented Nov 20, 2024

mikaelholmen commented Jan 7, 2025

nikic commented Jan 7, 2025

mikaelholmen commented Jan 7, 2025

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

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

Conversation

nikic commented Nov 1, 2024 • edited Loading

llvmbot commented Nov 1, 2024

nathanchance commented Nov 2, 2024

nikic commented Nov 4, 2024

nikic commented Nov 4, 2024

mikaelholmen commented Nov 5, 2024 • edited Loading

nikic commented Nov 5, 2024

mikaelholmen commented Nov 19, 2024

nikic commented Nov 19, 2024

mikaelholmen commented Nov 20, 2024

mikaelholmen commented Jan 7, 2025

nikic commented Jan 7, 2025

mikaelholmen commented Jan 7, 2025

nikic commented Nov 1, 2024 •

edited

Loading

mikaelholmen commented Nov 5, 2024 •

edited

Loading