[InstCombine] Let shrinkSplatShuffle act on vectors of different lengths #148593

Adar-Dagan · 2025-07-14T09:10:44Z

shrinkSplatShuffle in InstCombine would only move truncs up through shuffles if those shuffles inputs had the exact same type as their output, this PR weakens this constraint to only requiring that the scalar type of the input and output match.

github-actions · 2025-07-14T09:11:01Z

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-07-14T09:11:37Z

@llvm/pr-subscribers-llvm-transforms

Author: Adar Dagan (Adar-Dagan)

Changes

shrinkSplatShuffle in InstCombine would only move truncs up through shuffles if those shuffles inputs had the exact same type as their output, this PR weakens this constraint to only requiring that the scalar type of the input and output match.

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

3 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp (+6-2)
(modified) llvm/test/Transforms/InstCombine/trunc-inseltpoison.ll (+2-2)
(modified) llvm/test/Transforms/InstCombine/trunc.ll (+2-2)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
index 033ef8be700eb..8a98fd3235915 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
@@ -708,10 +708,14 @@ static Instruction *shrinkSplatShuffle(TruncInst &Trunc,
   auto *Shuf = dyn_cast<ShuffleVectorInst>(Trunc.getOperand(0));
   if (Shuf && Shuf->hasOneUse() && match(Shuf->getOperand(1), m_Undef()) &&
       all_equal(Shuf->getShuffleMask()) &&
-      Shuf->getType() == Shuf->getOperand(0)->getType()) {
+      Shuf->getType()->getScalarType() ==
+          Shuf->getOperand(0)->getType()->getScalarType()) {
     // trunc (shuf X, Undef, SplatMask) --> shuf (trunc X), Poison, SplatMask
     // trunc (shuf X, Poison, SplatMask) --> shuf (trunc X), Poison, SplatMask
-    Value *NarrowOp = Builder.CreateTrunc(Shuf->getOperand(0), Trunc.getType());
+    auto *const NewTruncTy =
+        VectorType::get(Trunc.getType()->getScalarType(),
+                        cast<VectorType>(Shuf->getOperand(0)->getType())->getElementCount());
+    Value *NarrowOp = Builder.CreateTrunc(Shuf->getOperand(0), NewTruncTy);
     return new ShuffleVectorInst(NarrowOp, Shuf->getShuffleMask());
   }
 
diff --git a/llvm/test/Transforms/InstCombine/trunc-inseltpoison.ll b/llvm/test/Transforms/InstCombine/trunc-inseltpoison.ll
index 33fa2c375f1ec..f83352c94ad89 100644
--- a/llvm/test/Transforms/InstCombine/trunc-inseltpoison.ll
+++ b/llvm/test/Transforms/InstCombine/trunc-inseltpoison.ll
@@ -959,8 +959,8 @@ define <3 x i31> @wide_splat3(<3 x i33> %x) {
 
 define <8 x i8> @wide_lengthening_splat(<4 x i16> %v) {
 ; CHECK-LABEL: @wide_lengthening_splat(
-; CHECK-NEXT:    [[SHUF:%.*]] = shufflevector <4 x i16> [[V:%.*]], <4 x i16> poison, <8 x i32> zeroinitializer
-; CHECK-NEXT:    [[TR:%.*]] = trunc <8 x i16> [[SHUF]] to <8 x i8>
+; CHECK-NEXT:    [[TMP1:%.*]] = trunc <4 x i16> [[V:%.*]] to <4 x i8>
+; CHECK-NEXT:    [[TR:%.*]] = shufflevector <4 x i8> [[TMP1]], <4 x i8> poison, <8 x i32> zeroinitializer
 ; CHECK-NEXT:    ret <8 x i8> [[TR]]
 ;
   %shuf = shufflevector <4 x i16> %v, <4 x i16> %v, <8 x i32> zeroinitializer
diff --git a/llvm/test/Transforms/InstCombine/trunc.ll b/llvm/test/Transforms/InstCombine/trunc.ll
index a85ce716fbdfa..8f727e365e88e 100644
--- a/llvm/test/Transforms/InstCombine/trunc.ll
+++ b/llvm/test/Transforms/InstCombine/trunc.ll
@@ -960,8 +960,8 @@ define <3 x i31> @wide_splat3(<3 x i33> %x) {
 
 define <8 x i8> @wide_lengthening_splat(<4 x i16> %v) {
 ; CHECK-LABEL: @wide_lengthening_splat(
-; CHECK-NEXT:    [[SHUF:%.*]] = shufflevector <4 x i16> [[V:%.*]], <4 x i16> poison, <8 x i32> zeroinitializer
-; CHECK-NEXT:    [[TR:%.*]] = trunc <8 x i16> [[SHUF]] to <8 x i8>
+; CHECK-NEXT:    [[TMP1:%.*]] = trunc <4 x i16> [[V:%.*]] to <4 x i8>
+; CHECK-NEXT:    [[TR:%.*]] = shufflevector <4 x i8> [[TMP1]], <4 x i8> poison, <8 x i32> zeroinitializer
 ; CHECK-NEXT:    ret <8 x i8> [[TR]]
 ;
   %shuf = shufflevector <4 x i16> %v, <4 x i16> %v, <8 x i32> zeroinitializer

github-actions · 2025-07-14T10:00:37Z

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

nikic · 2025-07-14T19:51:35Z

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

-    return new ShuffleVectorInst(NarrowOp, Shuf->getShuffleMask());
+    auto *const NewTruncTy = VectorType::get(
+        Trunc.getType()->getScalarType(),
+        cast<VectorType>(Shuf->getOperand(0)->getType())->getElementCount());


I think this is Shuf->getOperand(0)->getType()->getWithNewType(Trunc.getType()->getScalarType())?

nikic · 2025-07-14T19:52:31Z

llvm/test/Transforms/InstCombine/trunc.ll

-; CHECK-NEXT:    [[SHUF:%.*]] = shufflevector <4 x i16> [[V:%.*]], <4 x i16> poison, <8 x i32> zeroinitializer
-; CHECK-NEXT:    [[TR:%.*]] = trunc <8 x i16> [[SHUF]] to <8 x i8>
+; CHECK-NEXT:    [[TMP1:%.*]] = trunc <4 x i16> [[V:%.*]] to <4 x i8>
+; CHECK-NEXT:    [[TR:%.*]] = shufflevector <4 x i8> [[TMP1]], <4 x i8> poison, <8 x i32> zeroinitializer


Also test shortening splat? I think in that case the profitability is less clear.

Yeah I think we want to restrict it to only when Shuf->getOperand(0) is shorter than Shuf?

I don't understand why it wouldn't be profitable in that case, could you please elaborate?

Also added test

We're now performing trunc on a wider type than before, and that can be slower on some targets. E.g. a <8 x i16> trunc may take twice as many uops as a <4 x i16> trunc. There is definitely at least hardware on RISC-V where this is the case.

Thanks! changed

RKSimon

This is getting very close to needing to be cost based, which suggests we should move it the entire fold to VectorCombine - we already have VectorCombine::foldShuffleOfCastops for a shuffle of 2 matching casts, relaxing this to handle a single cast wouldn't be a huge amount of work.

Adar-Dagan · 2025-07-15T12:52:31Z

This is getting very close to needing to be cost based, which suggests we should move it the entire fold to VectorCombine - we already have VectorCombine::foldShuffleOfCastops for a shuffle of 2 matching casts, relaxing this to handle a single cast wouldn't be a huge amount of work.

@RKSimon
From what I see, VectorCombine::foldShuffleOfCastops tries to do the opposite transformation from what I am doing here, it tries to move the castop to be below the shuffle.

I think the transformation I am adding to here fits in InstCombine because we generally try to move up truncations and then act on smaller types.

lukel97 · 2025-07-16T05:00:13Z

llvm/test/Transforms/InstCombine/trunc.ll

 ; CHECK-NEXT:    ret <8 x i8> [[TR]]
 ;
  %shuf = shufflevector <4 x i16> %v, <4 x i16> %v, <8 x i32> zeroinitializer
  %tr = trunc <8 x i16> %shuf to <8 x i8>
  ret <8 x i8> %tr
 }

+define <4 x i8> @wide_shortening_splat(<8 x i16> %v) {


Nit, add a comment above explaining that this is a negative test?

lukel97 · 2025-07-16T05:03:43Z

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

    // trunc (shuf X, Undef, SplatMask) --> shuf (trunc X), Poison, SplatMask
    // trunc (shuf X, Poison, SplatMask) --> shuf (trunc X), Poison, SplatMask
-    Value *NarrowOp = Builder.CreateTrunc(Shuf->getOperand(0), Trunc.getType());
-    return new ShuffleVectorInst(NarrowOp, Shuf->getShuffleMask());
+    auto *const NewTruncTy = Shuf->getOperand(0)->getType()->getWithNewType(


Style nit, avoid using auto when it doesn't make the type clearer https://llvm.org/docs/CodingStandards.html#id29

Suggested change

auto *const NewTruncTy = Shuf->getOperand(0)->getType()->getWithNewType(

Type *NewTruncTy = Shuf->getOperand(0)->getType()->getWithNewType(

lukel97 · 2025-07-16T05:05:24Z

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

+    Value *NarrowOp =
+        Builder.CreateTrunc(Shuf->getOperand(0), NewTruncTy, Trunc.getName());
+    return new ShuffleVectorInst(NarrowOp, Shuf->getShuffleMask(),
+                                 Shuf->getName());


I don't think many places in InstCombine preserve the names, should these changes be left out and posted as a separate NFC?

I do think it would be nicer if the names where preserved but changed to be consistent with the rest of the pass

lukel97

LGTM, but agree with @RKSimon that we should look at doing these type of transforms in VectorCombine

lukel97 · 2025-07-16T15:59:25Z

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

    // trunc (shuf X, Undef, SplatMask) --> shuf (trunc X), Poison, SplatMask
    // trunc (shuf X, Poison, SplatMask) --> shuf (trunc X), Poison, SplatMask
-    Value *NarrowOp = Builder.CreateTrunc(Shuf->getOperand(0), Trunc.getType());
+    Type *const NewTruncTy = Shuf->getOperand(0)->getType()->getWithNewType(


I don't think const pointers are very common in LLVM, it can probably be dropped

Expand move trunc through shuffle splat

6fa9358

Adar-Dagan requested a review from nikic as a code owner July 14, 2025 09:10

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

Adar-Dagan force-pushed the main branch from e887b3d to 6fa9358 Compare July 14, 2025 10:33

nikic requested review from fhahn, preames, RKSimon and lukel97 July 14, 2025 19:48

nikic reviewed Jul 14, 2025

View reviewed changes

RKSimon reviewed Jul 15, 2025

View reviewed changes

Adar-Dagan force-pushed the main branch from acb3e1f to 383fb91 Compare July 16, 2025 03:56

lukel97 reviewed Jul 16, 2025

View reviewed changes

Answer comments

eb8cac3

Adar-Dagan force-pushed the main branch from 383fb91 to eb8cac3 Compare July 16, 2025 11:12

lukel97 approved these changes Jul 16, 2025

View reviewed changes

	auto *const NewTruncTy = Shuf->getOperand(0)->getType()->getWithNewType(
	Type *NewTruncTy = Shuf->getOperand(0)->getType()->getWithNewType(

[InstCombine] Let shrinkSplatShuffle act on vectors of different lengths #148593

Are you sure you want to change the base?

[InstCombine] Let shrinkSplatShuffle act on vectors of different lengths #148593

Conversation

Adar-Dagan commented Jul 14, 2025

Uh oh!

github-actions bot commented Jul 14, 2025

Uh oh!

llvmbot commented Jul 14, 2025

Uh oh!

github-actions bot commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RKSimon left a comment

Choose a reason for hiding this comment

Uh oh!

Adar-Dagan commented Jul 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukel97 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jul 14, 2025 •

edited

Loading