[InstCombine] Match scalable splats in m_ImmConstant #132522

lukel97 · 2025-03-22T05:06:48Z

#118806 fixed an infinite loop in FoldShiftByConstant that could occur when the shift amount was a ConstantExpr.

However this meant that FoldShiftByConstant no longer kicked in for scalable vectors because scalable splats are represented by ConstantExprs.

This fixes it by allowing scalable splats of non-ConstantExprs in m_ImmConstant, which also fixes a few other test cases where scalable splats were being missed.

But I'm also hoping that UseConstantIntForScalableSplat will eventually remove the need for this.

I noticed this when trying to reverse a combine on RISC-V in #132245, and saw that the resulting vector and scalar forms were different.

… again However this meant that FoldShiftByConstant no longer kicked in for scalable vectors because scalable splats are represented by ConstantExprs. This fixes it by explicitly allowing splats of ConstantInts, it's not the prettiest so open to any suggestions. But I'm also hoping that UseConstantIntForScalableSplat will eventually remove the need for this. I noticed this when trying to reverse a combine on RISC-V in llvm#132245, and saw that the resulting vector and scalar forms were different.

llvmbot · 2025-03-22T05:07:26Z

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: Luke Lau (lukel97)

Changes

However this meant that FoldShiftByConstant no longer kicked in for scalable vectors because scalable splats are represented by ConstantExprs.

This fixes it by explicitly allowing splats of ConstantInts, it's not the prettiest so open to any suggestions.

But I'm also hoping that UseConstantIntForScalableSplat will eventually remove the need for this.

I noticed this when trying to reverse a combine on RISC-V in #132245, and saw that the resulting vector and scalar forms were different.

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

3 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp (+4-1)
(modified) llvm/test/Transforms/InstCombine/shl-bo.ll (+11)
(modified) llvm/test/Transforms/InstCombine/shl-twice-constant.ll (+11)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
index 90cd279e8a457..91174cc79cd2b 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
@@ -428,7 +428,10 @@ Instruction *InstCombinerImpl::commonShiftTransforms(BinaryOperator &I) {
         return R;
 
   Constant *CUI;
-  if (match(Op1, m_ImmConstant(CUI)))
+  if (match(Op1, m_Constant(CUI)) &&
+      (!isa<ConstantExpr>(CUI) ||
+       (Ty->isVectorTy() &&
+        isa_and_present<ConstantInt>(CUI->getSplatValue()))))
     if (Instruction *Res = FoldShiftByConstant(Op0, CUI, I))
       return Res;
 
diff --git a/llvm/test/Transforms/InstCombine/shl-bo.ll b/llvm/test/Transforms/InstCombine/shl-bo.ll
index c32ac2eacb25a..5ee8716d5d119 100644
--- a/llvm/test/Transforms/InstCombine/shl-bo.ll
+++ b/llvm/test/Transforms/InstCombine/shl-bo.ll
@@ -656,3 +656,14 @@ define <16 x i8> @test_FoldShiftByConstant_CreateAnd(<16 x i8> %in0) {
   %vshl_n = shl <16 x i8> %tmp, <i8 5, i8 5, i8 5, i8 5, i8 5, i8 5, i8 5, i8 5, i8 5, i8 5, i8 5, i8 5, i8 5, i8 5, i8 5, i8 5>
   ret <16 x i8> %vshl_n
 }
+
+define <vscale x 1 x i8> @test_FoldShiftByConstant_CreateAnd_scalable(<vscale x 1 x i8> %x) {
+; CHECK-LABEL: @test_FoldShiftByConstant_CreateAnd_scalable(
+; CHECK-NEXT:    [[TMP1:%.*]] = shl <vscale x 1 x i8> [[X:%.*]], splat (i8 2)
+; CHECK-NEXT:    [[TMP2:%.*]] = and <vscale x 1 x i8> [[TMP1]], splat (i8 8)
+; CHECK-NEXT:    ret <vscale x 1 x i8> [[TMP2]]
+;
+  %1 = and <vscale x 1 x i8> %x, splat (i8 2)
+  %2 = shl <vscale x 1 x i8> %1, splat (i8 2)
+  ret <vscale x 1 x i8> %2
+}
diff --git a/llvm/test/Transforms/InstCombine/shl-twice-constant.ll b/llvm/test/Transforms/InstCombine/shl-twice-constant.ll
index bbdd7fa3d1c40..151db29fe3e5f 100644
--- a/llvm/test/Transforms/InstCombine/shl-twice-constant.ll
+++ b/llvm/test/Transforms/InstCombine/shl-twice-constant.ll
@@ -14,3 +14,14 @@ define i64 @testfunc() {
   %shl2 = shl i64 %shl1, ptrtoint (ptr @c to i64)
   ret i64 %shl2
 }
+
+define <vscale x 1 x i64> @scalable() {
+; CHECK-LABEL: @scalable(
+; CHECK-NEXT:    [[SHL1:%.*]] = shl nuw <vscale x 1 x i64> splat (i64 1), shufflevector (<vscale x 1 x i64> insertelement (<vscale x 1 x i64> poison, i64 ptrtoint (ptr @c2 to i64), i64 0), <vscale x 1 x i64> poison, <vscale x 1 x i32> zeroinitializer)
+; CHECK-NEXT:    [[SHL2:%.*]] = shl <vscale x 1 x i64> [[SHL1]], shufflevector (<vscale x 1 x i64> insertelement (<vscale x 1 x i64> poison, i64 ptrtoint (ptr @c to i64), i64 0), <vscale x 1 x i64> poison, <vscale x 1 x i32> zeroinitializer)
+; CHECK-NEXT:    ret <vscale x 1 x i64> [[SHL2]]
+;
+  %shl1 = shl <vscale x 1 x i64> splat (i64 1), splat (i64 ptrtoint (ptr @c2 to i64))
+  %shl2 = shl <vscale x 1 x i64> %shl1, splat (i64 ptrtoint (ptr @c to i64))
+  ret <vscale x 1 x i64> %shl2
+}

dtcxzyw

I am ok with this workaround.

dtcxzyw · 2025-03-22T06:52:26Z

llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp

  Constant *CUI;
-  if (match(Op1, m_ImmConstant(CUI)))
+  if (match(Op1, m_Constant(CUI)) &&
+      (!isa<ConstantExpr>(CUI) ||


Can you add a fixme here to note that we should remove this special case after we use ConstantInt to represent a scalable vector splat by default?

Done in the matcher for a0f4ac6

nikic

Can we generally accept scalable splats in m_ImmConstant? I doubt this is the only place with this problem.

dtcxzyw · 2025-03-23T03:00:32Z

Can we generally accept scalable splats in m_ImmConstant? I doubt this is the only place with this problem.

I am afraid that it will cause new infinite loop bugs :(

lukel97 · 2025-03-24T04:21:14Z

Can we generally accept scalable splats in m_ImmConstant? I doubt this is the only place with this problem.

I've reworked m_ImmConstant to accept scalable splats, so the changes in InstCombineShifts are gone now.

I had to create two separate structs to help match it, because one binds and the other doesn't (and I couldn't seem to reuse cstval_pred_ty because it's not polymorphic and needs to be specicialized to either ConstantInt or ConstantFP).

It was tempting to try and create one binding matcher and declare a dummy Constant for the non-binding m_ImmConstant(), but it looks like everything here is trying to be tail callable so I avoided it.

lukel97 · 2025-03-24T04:26:42Z

I am afraid that it will cause new infinite loop bugs :(

In this new version I've added a check to make sure that the splat value also doesn't contain any ConstantExprs, I think that should be enough?

lukel97 · 2025-04-02T13:57:51Z

Ping

dtcxzyw

LG

llvm/include/llvm/IR/PatternMatch.h

Co-authored-by: Yingwei Zheng <[email protected]>

nikic

LGTM

antmox · 2025-04-03T12:41:43Z

Hi, Could this be the cause of these test-suite failures ?

https://lab.llvm.org/buildbot/#/builders/41/builds/5897
https://lab.llvm.org/buildbot/#/builders/4/builds/6002
https://lab.llvm.org/buildbot/#/builders/199/builds/2539
https://lab.llvm.org/buildbot/#/builders/143/builds/6643
https://lab.llvm.org/buildbot/#/builders/17/builds/6974
https://lab.llvm.org/buildbot/#/builders/198/builds/3365

I am not sure at all about this. This is just because the issue seems to be in the InstCombine area

clang++: ../llvm/llvm/lib/Analysis/InstructionSimplify.cpp:3931: Value *simplifyICmpInst(CmpPredicate, Value *, Value *, const SimplifyQuery &, unsigned int): Assertion `RExt && "Constant-fold of ImmConstant should not fail"' failed.                                                                                                                                                    

Stack dump:
 #0 0x0000ab90f89a6890 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+++0x81c6890)
 #1 0x0000ab90f89a478c llvm::sys::RunSignalHandlers() (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+++0x81c478c)
 #2 0x0000ab90f890c294 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
 #3 0x0000f61a9946b8f8 (linux-vdso.so.1+0x8f8)
 #4 0x0000f61a98ecf1f0 __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #5 0x0000f61a98e8a67c gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #6 0x0000f61a98e77130 abort ./stdlib/abort.c:81:7
 #7 0x0000f61a98e83fd4 __assert_fail_base ./assert/assert.c:91:7
 #8 0x0000f61a98e8404c (/lib/aarch64-linux-gnu/libc.so.6+0x3404c)
 #9 0x0000ab90f7a542fc simplifyICmpInst(llvm::CmpPredicate, llvm::Value*, llvm::Value*, llvm::SimplifyQuery const&, unsigned int) InstructionSimplify.cpp:0:0
#10 0x0000ab90f7a60bc8 isDivZero(llvm::Value*, llvm::Value*, llvm::SimplifyQuery const&, unsigned int, bool) InstructionSimplify.cpp:0:0
#11 0x0000ab90f7a60318 simplifyDivRem(llvm::Instruction::BinaryOps, llvm::Value*, llvm::Value*, llvm::SimplifyQuery const&, unsigned int) InstructionSimplify.cpp:0:0
#12 0x0000ab90f7a60e78 simplifyRem(llvm::Instruction::BinaryOps, llvm::Value*, llvm::Value*, llvm::SimplifyQuery const&, unsigned int) InstructionSimplify.cpp:0:0
#13 0x0000ab90f86662ac llvm::InstCombinerImpl::visitURem(llvm::BinaryOperator&) InstCombineMulDivRem.cpp:0:0
#14 0x0000ab90f85b45f8 llvm::InstCombinerImpl::run() InstructionCombining.cpp:0:0
#15 0x0000ab90f85b7a4c 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
#16 0x0000ab90f85b70e0 llvm::InstCombinePass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+++0x7dd70e0)

lukel97 · 2025-04-03T13:00:21Z

Hi, Could this be the cause of these test-suite failures ?

https://lab.llvm.org/buildbot/#/builders/41/builds/5897 https://lab.llvm.org/buildbot/#/builders/4/builds/6002 https://lab.llvm.org/buildbot/#/builders/199/builds/2539 https://lab.llvm.org/buildbot/#/builders/143/builds/6643 https://lab.llvm.org/buildbot/#/builders/17/builds/6974 https://lab.llvm.org/buildbot/#/builders/198/builds/3365

I am not sure at all about this. This is just because the issue seems to be in the InstCombine area

clang++: ../llvm/llvm/lib/Analysis/InstructionSimplify.cpp:3931: Value *simplifyICmpInst(CmpPredicate, Value *, Value *, const SimplifyQuery &, unsigned int): Assertion `RExt && "Constant-fold of ImmConstant should not fail"' failed.                                                                                                                                                    

Stack dump:
 #0 0x0000ab90f89a6890 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+++0x81c6890)
 #1 0x0000ab90f89a478c llvm::sys::RunSignalHandlers() (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+++0x81c478c)
 #2 0x0000ab90f890c294 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
 #3 0x0000f61a9946b8f8 (linux-vdso.so.1+0x8f8)
 #4 0x0000f61a98ecf1f0 __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #5 0x0000f61a98e8a67c gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #6 0x0000f61a98e77130 abort ./stdlib/abort.c:81:7
 #7 0x0000f61a98e83fd4 __assert_fail_base ./assert/assert.c:91:7
 #8 0x0000f61a98e8404c (/lib/aarch64-linux-gnu/libc.so.6+0x3404c)
 #9 0x0000ab90f7a542fc simplifyICmpInst(llvm::CmpPredicate, llvm::Value*, llvm::Value*, llvm::SimplifyQuery const&, unsigned int) InstructionSimplify.cpp:0:0
#10 0x0000ab90f7a60bc8 isDivZero(llvm::Value*, llvm::Value*, llvm::SimplifyQuery const&, unsigned int, bool) InstructionSimplify.cpp:0:0
#11 0x0000ab90f7a60318 simplifyDivRem(llvm::Instruction::BinaryOps, llvm::Value*, llvm::Value*, llvm::SimplifyQuery const&, unsigned int) InstructionSimplify.cpp:0:0
#12 0x0000ab90f7a60e78 simplifyRem(llvm::Instruction::BinaryOps, llvm::Value*, llvm::Value*, llvm::SimplifyQuery const&, unsigned int) InstructionSimplify.cpp:0:0
#13 0x0000ab90f86662ac llvm::InstCombinerImpl::visitURem(llvm::BinaryOperator&) InstCombineMulDivRem.cpp:0:0
#14 0x0000ab90f85b45f8 llvm::InstCombinerImpl::run() InstructionCombining.cpp:0:0
#15 0x0000ab90f85b7a4c 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
#16 0x0000ab90f85b70e0 llvm::InstCombinePass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1.install/bin/clang+++0x7dd70e0)

Oh thanks for pointing that out, I didn't notice the InstructionSimplify failure. Yesterday I mainly saw some errors about a missing Transforms/Vectorize/./VectorizeTests/18/53 test, so maybe that got buried in the buildbot emails. Taking a look now

This reverts commit df9e5ae. This is triggering an assertion failure on llvm-test-suite with -enable-vplan-native-path: https://lab.llvm.org/buildbot/#/builders/198/builds/3365

lukel97 · 2025-04-03T14:19:10Z

@antmox I was able to reproduce it locally and it goes away without this patch, thanks for pointing me to it. Reverted now in b61e387. The missing test was actually a missing llvm-test-suite executable caused by that assertion

antmox · 2025-04-03T14:43:26Z

thanks @lukel97
about the error message, I guess it is not part of the buildbot emails
I found it in the buildbot build pages, like https://lab.llvm.org/buildbot/#/builders/41/builds/5897/steps/18/logs/stdio

lukel97 · 2025-04-03T15:19:33Z

It looks like the crash is caused by casts of m_ImmConstant splats not being folded by ConstantFoldCastOperand, triggering the "Constant-fold of ImmConstant should not fail" assertion. But #133207 actually fixes that, so I'll land that first.

…32522)" This reverts commit b61e387.

…" (#134262) This reapplies #132522. Previously casts of scalable m_ImmConstant splats weren't being folded by ConstantFoldCastOperand, triggering the "Constant-fold of ImmConstant should not fail" assertion. There are no changes to the code in this PR, instead we just needed #133207 to land first. A test has been added for the assertion in llvm/test/Transforms/InstSimplify/vec-icmp-of-cast.ll @icmp_ult_sext_scalable_splat_is_true. <hr/> #118806 fixed an infinite loop in FoldShiftByConstant that could occur when the shift amount was a ConstantExpr. However this meant that FoldShiftByConstant no longer kicked in for scalable vectors because scalable splats are represented by ConstantExprs. This fixes it by allowing scalable splats of non-ConstantExprs in m_ImmConstant, which also fixes a few other test cases where scalable splats were being missed. But I'm also hoping that UseConstantIntForScalableSplat will eventually remove the need for this. I noticed this when trying to reverse a combine on RISC-V in #132245, and saw that the resulting vector and scalar forms were different.

Local branch origin/amd-gfx 01fc438 Merged main:52f3cad9ffa3 into origin/amd-gfx:31272f8ef7f7 Remote branch main b61e387 Revert "[InstCombine] Match scalable splats in m_ImmConstant (llvm#132522)"

lukel97 added 2 commits March 22, 2025 12:33

Precommit tests

5cb140c

lukel97 requested review from dtcxzyw, goldsteinn and paulwalker-arm March 22, 2025 05:06

lukel97 requested a review from nikic as a code owner March 22, 2025 05:06

llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Mar 22, 2025

dtcxzyw approved these changes Mar 22, 2025

View reviewed changes

nikic reviewed Mar 22, 2025

View reviewed changes

llvmbot added the llvm:ir label Mar 24, 2025

Rework to adjust m_ImmConstant

a0f4ac6

lukel97 force-pushed the instcombine/FoldShiftByConstant-scalable-splat branch from b7d69b0 to a0f4ac6 Compare March 24, 2025 04:18

Remove fixme

e1b5509

lukel97 changed the title ~~[InstCombine] Allow folds of shifts by constants for scalable vectors again~~ [InstCombine] Match scalable splats in m_ImmConstant Mar 24, 2025

Merge branch 'main' into instcombine/FoldShiftByConstant-scalable-splat

0de7c62

dtcxzyw approved these changes Apr 2, 2025

View reviewed changes

llvm/include/llvm/IR/PatternMatch.h Outdated Show resolved Hide resolved

Update llvm/include/llvm/IR/PatternMatch.h

ae87e3b

Co-authored-by: Yingwei Zheng <[email protected]>

nikic approved these changes Apr 2, 2025

View reviewed changes

lukel97 merged commit df9e5ae into llvm:main Apr 2, 2025
11 checks passed

lukel97 added a commit to lukel97/llvm-project that referenced this pull request Apr 3, 2025

Reapply "[InstCombine] Match scalable splats in m_ImmConstant (llvm#1…

598af22

…32522)" This reverts commit b61e387.

lukel97 mentioned this pull request Apr 3, 2025

Reapply "[InstCombine] Match scalable splats in m_ImmConstant (#132522)" #134262

Merged

[InstCombine] Match scalable splats in m_ImmConstant #132522

[InstCombine] Match scalable splats in m_ImmConstant #132522

Uh oh!

Conversation

lukel97 commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtcxzyw left a comment

Choose a reason for hiding this comment

Uh oh!

dtcxzyw Mar 22, 2025

Choose a reason for hiding this comment

Uh oh!

lukel97 Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

dtcxzyw commented Mar 23, 2025

Uh oh!

lukel97 commented Mar 24, 2025

Uh oh!

lukel97 commented Mar 24, 2025

Uh oh!

lukel97 commented Apr 2, 2025

Uh oh!

dtcxzyw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antmox commented Apr 3, 2025

Uh oh!

lukel97 commented Apr 3, 2025

Uh oh!

lukel97 commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antmox commented Apr 3, 2025

Uh oh!

lukel97 commented Apr 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lukel97 commented Mar 22, 2025 •

edited

Loading

llvmbot commented Mar 22, 2025 •

edited

Loading

lukel97 commented Apr 3, 2025 •

edited

Loading