[GlobalISel] Take the result size into account when const folding icmp #134365

KRM7 · 2025-04-04T11:03:25Z

The current implementation always creates a 1 bit constant for the result of the G_ICMP, which will cause issues if the destination register size is larger than that. With asserts enabled, it will cause a crash in buildConstant:

llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:322: virtual MachineInstrBuilder llvm::MachineIRBuilder::buildConstant(const DstOp &, const ConstantInt &): Assertion `EltTy.getScalarSizeInBits() == Val.getBitWidth() && "creating constant with the wrong size"' failed.

github-actions · 2025-04-04T11:03:44Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-04-07T10:56:52Z

@llvm/pr-subscribers-llvm-globalisel

Author: None (KRM7)

Changes

The current implementation always creates a 1 bit constant for the result of the G_ICMP, which will cause issues if the destination register size is larger than that. With asserts enabled, it will cause a crash in buildConstant:

llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:322: virtual MachineInstrBuilder llvm::MachineIRBuilder::buildConstant(const DstOp &amp;, const ConstantInt &amp;): Assertion `EltTy.getScalarSizeInBits() == Val.getBitWidth() &amp;&amp; "creating constant with the wrong size"' failed.

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

4 Files Affected:

(modified) llvm/include/llvm/CodeGen/GlobalISel/Utils.h (+1)
(modified) llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp (+5-3)
(modified) llvm/lib/CodeGen/GlobalISel/Utils.cpp (+20-12)
(modified) llvm/unittests/CodeGen/GlobalISel/CSETest.cpp (+12)

diff --git a/llvm/include/llvm/CodeGen/GlobalISel/Utils.h b/llvm/include/llvm/CodeGen/GlobalISel/Utils.h
index 44141844f42f4..3260b50ce317c 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/Utils.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/Utils.h
@@ -325,6 +325,7 @@ ConstantFoldCountZeros(Register Src, const MachineRegisterInfo &MRI,
 
 std::optional<SmallVector<APInt>>
 ConstantFoldICmp(unsigned Pred, const Register Op1, const Register Op2,
+                 unsigned DstSizeInBits, unsigned ExtOp,
                  const MachineRegisterInfo &MRI);
 
 /// Test if the given value is known to have exactly one bit set. This differs
diff --git a/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
index bf8e847011d7c..10c72641ce2df 100644
--- a/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
@@ -189,10 +189,12 @@ MachineInstrBuilder CSEMIRBuilder::buildInstr(unsigned Opc,
     assert(SrcOps.size() == 3 && "Invalid sources");
     assert(DstOps.size() == 1 && "Invalid dsts");
     LLT SrcTy = SrcOps[1].getLLTTy(*getMRI());
+    LLT DstTy = DstOps[0].getLLTTy(*getMRI());
+    auto BoolExtOp = getBoolExtOp(SrcTy.isVector(), false);
 
-    if (std::optional<SmallVector<APInt>> Cst =
-            ConstantFoldICmp(SrcOps[0].getPredicate(), SrcOps[1].getReg(),
-                             SrcOps[2].getReg(), *getMRI())) {
+    if (std::optional<SmallVector<APInt>> Cst = ConstantFoldICmp(
+            SrcOps[0].getPredicate(), SrcOps[1].getReg(), SrcOps[2].getReg(),
+            DstTy.getScalarSizeInBits(), BoolExtOp, *getMRI())) {
       if (SrcTy.isVector())
         return buildBuildVectorConstant(DstOps[0], *Cst);
       return buildConstant(DstOps[0], Cst->front());
diff --git a/llvm/lib/CodeGen/GlobalISel/Utils.cpp b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
index 223d69c362185..8b2b0b4083287 100644
--- a/llvm/lib/CodeGen/GlobalISel/Utils.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
@@ -1027,13 +1027,21 @@ llvm::ConstantFoldCountZeros(Register Src, const MachineRegisterInfo &MRI,
 
 std::optional<SmallVector<APInt>>
 llvm::ConstantFoldICmp(unsigned Pred, const Register Op1, const Register Op2,
+                       unsigned DstSizeInBits, unsigned ExtOp,
                        const MachineRegisterInfo &MRI) {
+  assert(ExtOp == TargetOpcode::G_SEXT || ExtOp == TargetOpcode::G_ZEXT ||
+         ExtOp == TargetOpcode::G_ANYEXT);
+
   LLT Ty = MRI.getType(Op1);
   if (Ty != MRI.getType(Op2))
     return std::nullopt;
 
-  auto TryFoldScalar = [&MRI, Pred](Register LHS,
-                                    Register RHS) -> std::optional<APInt> {
+  const APInt FalseCst = APInt::getZero(DstSizeInBits);
+  const APInt TrueCst = (ExtOp == TargetOpcode::G_SEXT)
+                            ? APInt::getAllOnes(DstSizeInBits)
+                            : APInt::getOneBitSet(DstSizeInBits, 0);
+
+  auto TryFoldScalar = [&](Register LHS, Register RHS) -> std::optional<APInt> {
     auto LHSCst = getIConstantVRegVal(LHS, MRI);
     auto RHSCst = getIConstantVRegVal(RHS, MRI);
     if (!LHSCst || !RHSCst)
@@ -1041,25 +1049,25 @@ llvm::ConstantFoldICmp(unsigned Pred, const Register Op1, const Register Op2,
 
     switch (Pred) {
     case CmpInst::Predicate::ICMP_EQ:
-      return APInt(/*numBits=*/1, LHSCst->eq(*RHSCst));
+      return LHSCst->eq(*RHSCst) ? TrueCst : FalseCst;
     case CmpInst::Predicate::ICMP_NE:
-      return APInt(/*numBits=*/1, LHSCst->ne(*RHSCst));
+      return LHSCst->ne(*RHSCst) ? TrueCst : FalseCst;
     case CmpInst::Predicate::ICMP_UGT:
-      return APInt(/*numBits=*/1, LHSCst->ugt(*RHSCst));
+      return LHSCst->ugt(*RHSCst) ? TrueCst : FalseCst;
     case CmpInst::Predicate::ICMP_UGE:
-      return APInt(/*numBits=*/1, LHSCst->uge(*RHSCst));
+      return LHSCst->uge(*RHSCst) ? TrueCst : FalseCst;
     case CmpInst::Predicate::ICMP_ULT:
-      return APInt(/*numBits=*/1, LHSCst->ult(*RHSCst));
+      return LHSCst->ult(*RHSCst) ? TrueCst : FalseCst;
     case CmpInst::Predicate::ICMP_ULE:
-      return APInt(/*numBits=*/1, LHSCst->ule(*RHSCst));
+      return LHSCst->ule(*RHSCst) ? TrueCst : FalseCst;
     case CmpInst::Predicate::ICMP_SGT:
-      return APInt(/*numBits=*/1, LHSCst->sgt(*RHSCst));
+      return LHSCst->sgt(*RHSCst) ? TrueCst : FalseCst;
     case CmpInst::Predicate::ICMP_SGE:
-      return APInt(/*numBits=*/1, LHSCst->sge(*RHSCst));
+      return LHSCst->sge(*RHSCst) ? TrueCst : FalseCst;
     case CmpInst::Predicate::ICMP_SLT:
-      return APInt(/*numBits=*/1, LHSCst->slt(*RHSCst));
+      return LHSCst->slt(*RHSCst) ? TrueCst : FalseCst;
     case CmpInst::Predicate::ICMP_SLE:
-      return APInt(/*numBits=*/1, LHSCst->sle(*RHSCst));
+      return LHSCst->sle(*RHSCst) ? TrueCst : FalseCst;
     default:
       return std::nullopt;
     }
diff --git a/llvm/unittests/CodeGen/GlobalISel/CSETest.cpp b/llvm/unittests/CodeGen/GlobalISel/CSETest.cpp
index cd6e32311a9ee..0f479f295601b 100644
--- a/llvm/unittests/CodeGen/GlobalISel/CSETest.cpp
+++ b/llvm/unittests/CodeGen/GlobalISel/CSETest.cpp
@@ -500,6 +500,18 @@ TEST_F(AArch64GISelMITest, TestConstantFoldICMP) {
     EXPECT_TRUE(I->getOperand(1).getCImm()->getZExtValue());
   }
 
+  {
+    auto I = CSEB.buildICmp(CmpInst::Predicate::ICMP_EQ, s32, One, One);
+    EXPECT_TRUE(I->getOpcode() == TargetOpcode::G_CONSTANT);
+    EXPECT_TRUE(I->getOperand(1).getCImm()->getZExtValue());
+  }
+
+  {
+    auto I = CSEB.buildICmp(CmpInst::Predicate::ICMP_EQ, s32, One, Two);
+    EXPECT_TRUE(I->getOpcode() == TargetOpcode::G_CONSTANT);
+    EXPECT_FALSE(I->getOperand(1).getCImm()->getZExtValue());
+  }
+
   LLT VecTy = LLT::fixed_vector(2, s32);
   LLT DstTy = LLT::fixed_vector(2, s1);
   auto Three = CSEB.buildConstant(s32, 3);

KRM7 · 2025-04-07T10:59:04Z

@arsenm @shiltian @aemerson

llvm/unittests/CodeGen/GlobalISel/CSETest.cpp

llvm/lib/CodeGen/GlobalISel/Utils.cpp

arsenm · 2025-04-13T10:44:58Z

llvm/lib/CodeGen/GlobalISel/Utils.cpp

+    const APInt FalseCst = APInt::getZero(DstScalarSizeInBits);
+    const APInt TrueCst = (ExtOp == TargetOpcode::G_SEXT)
+                              ? APInt::getAllOnes(DstScalarSizeInBits)
+                              : APInt::getOneBitSet(DstScalarSizeInBits, 0);


Can you make this a helper function, and use it in the implementation below? Should avoid eagerly constructing these two APInts

arsenm · 2025-04-13T10:45:12Z

llvm/lib/CodeGen/GlobalISel/Utils.cpp

    if (!LHSCst || !RHSCst)
      return std::nullopt;


This ought to have early exited after the RHS failed before checking the LHS

arsenm · 2025-04-13T10:45:26Z

llvm/unittests/CodeGen/GlobalISel/CSETest.cpp

+    EXPECT_TRUE(I->getOpcode() == TargetOpcode::G_CONSTANT);
+    EXPECT_EQ(I->getOperand(1).getCImm()->getZExtValue(), 0);
+  }
+


Check a vector case?

KRM7 · 2025-04-16T08:33:52Z

Addressed review comments @arsenm @shiltian @aemerson

github-actions · 2025-05-05T17:02:18Z

@KRM7 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvm#134365) The current implementation always creates a 1 bit constant for the result of the `G_ICMP`, which will cause issues if the destination register size is larger than that. With asserts enabled, it will cause a crash in `buildConstant`: ``` llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:322: virtual MachineInstrBuilder llvm::MachineIRBuilder::buildConstant(const DstOp &, const ConstantInt &): Assertion `EltTy.getScalarSizeInBits() == Val.getBitWidth() && "creating constant with the wrong size"' failed. ```

KRM7 force-pushed the icmp-constfold-crash branch 4 times, most recently from 464fed3 to bad4cc7 Compare April 7, 2025 08:43

KRM7 marked this pull request as ready for review April 7, 2025 10:56

llvmbot added the llvm:globalisel label Apr 7, 2025

arsenm reviewed Apr 7, 2025

View reviewed changes

KRM7 force-pushed the icmp-constfold-crash branch from bad4cc7 to 99807b1 Compare April 7, 2025 12:44

KRM7 requested a review from arsenm April 7, 2025 13:23

arsenm reviewed Apr 13, 2025

View reviewed changes

[GlobalISel] Take the result size into account when const folding icmp

49ce91e

KRM7 force-pushed the icmp-constfold-crash branch from 99807b1 to 49ce91e Compare April 15, 2025 14:31

KRM7 requested a review from arsenm April 29, 2025 08:20

arsenm approved these changes May 5, 2025

View reviewed changes

arsenm merged commit 0926d94 into llvm:main May 5, 2025
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[GlobalISel] Take the result size into account when const folding icmp #134365

[GlobalISel] Take the result size into account when const folding icmp #134365

KRM7 commented Apr 4, 2025

Uh oh!

github-actions bot commented Apr 4, 2025

Uh oh!

llvmbot commented Apr 7, 2025

Uh oh!

KRM7 commented Apr 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arsenm Apr 13, 2025

Uh oh!

arsenm Apr 13, 2025

Uh oh!

arsenm Apr 13, 2025

Uh oh!

KRM7 commented Apr 16, 2025

Uh oh!

Uh oh!

github-actions bot commented May 5, 2025

Uh oh!

Uh oh!

[GlobalISel] Take the result size into account when const folding icmp #134365

[GlobalISel] Take the result size into account when const folding icmp #134365

Conversation

KRM7 commented Apr 4, 2025

Uh oh!

github-actions bot commented Apr 4, 2025

Uh oh!

llvmbot commented Apr 7, 2025

Uh oh!

KRM7 commented Apr 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arsenm Apr 13, 2025

Choose a reason for hiding this comment

Uh oh!

arsenm Apr 13, 2025

Choose a reason for hiding this comment

Uh oh!

arsenm Apr 13, 2025

Choose a reason for hiding this comment

Uh oh!

KRM7 commented Apr 16, 2025

Uh oh!

Uh oh!

github-actions bot commented May 5, 2025

Uh oh!

Uh oh!