Skip to content

[VectorCombine] Remove requirement for Instructions in shuffleToIdentity #93543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

davemgreen
Copy link
Collaborator

This removes the check that both operands of the original shuffle are instructions, which is a relic from a previous version that held more variables as Instructions.

@llvmbot
Copy link
Member

llvmbot commented May 28, 2024

@llvm/pr-subscribers-llvm-transforms

Author: David Green (davemgreen)

Changes

This removes the check that both operands of the original shuffle are instructions, which is a relic from a previous version that held more variables as Instructions.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/Vectorize/VectorCombine.cpp (+1-2)
  • (modified) llvm/test/Transforms/VectorCombine/AArch64/shuffletoidentity.ll (+2-5)
diff --git a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
index 056f0d6b3ee6c..ffed73cdb18c0 100644
--- a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
+++ b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
@@ -1757,8 +1757,7 @@ static Value *generateNewInstTree(ArrayRef<InstLane> Item, FixedVectorType *Ty,
 // do so.
 bool VectorCombine::foldShuffleToIdentity(Instruction &I) {
   auto *Ty = dyn_cast<FixedVectorType>(I.getType());
-  if (!Ty || !isa<Instruction>(I.getOperand(0)) ||
-      !isa<Instruction>(I.getOperand(1)))
+  if (!Ty)
     return false;
 
   SmallVector<InstLane> Start(Ty->getNumElements());
diff --git a/llvm/test/Transforms/VectorCombine/AArch64/shuffletoidentity.ll b/llvm/test/Transforms/VectorCombine/AArch64/shuffletoidentity.ll
index 5cbda8a1e112e..71c49636a1735 100644
--- a/llvm/test/Transforms/VectorCombine/AArch64/shuffletoidentity.ll
+++ b/llvm/test/Transforms/VectorCombine/AArch64/shuffletoidentity.ll
@@ -15,9 +15,7 @@ define <8 x i8> @trivial(<8 x i8> %a) {
 
 define <4 x i32> @add_same_operands(<4 x i32> %x) {
 ; CHECK-LABEL: @add_same_operands(
-; CHECK-NEXT:    [[SHUF:%.*]] = shufflevector <4 x i32> [[X:%.*]], <4 x i32> poison, <4 x i32> <i32 3, i32 2, i32 1, i32 0>
-; CHECK-NEXT:    [[ADD:%.*]] = add <4 x i32> [[SHUF]], [[SHUF]]
-; CHECK-NEXT:    [[REVSHUF:%.*]] = shufflevector <4 x i32> [[ADD]], <4 x i32> poison, <4 x i32> <i32 3, i32 2, i32 1, i32 0>
+; CHECK-NEXT:    [[REVSHUF:%.*]] = add <4 x i32> [[X:%.*]], [[X]]
 ; CHECK-NEXT:    ret <4 x i32> [[REVSHUF]]
 ;
   %shuf = shufflevector <4 x i32> %x, <4 x i32> poison, <4 x i32> <i32 3, i32 2, i32 1, i32 0>
@@ -364,8 +362,7 @@ define <8 x i8> @inner_shuffle(<8 x i8> %a, <8 x i8> %b, <8 x i8> %c) {
 define <4 x i32> @extrause_add_same_operands(<4 x i32> %x) {
 ; CHECK-LABEL: @extrause_add_same_operands(
 ; CHECK-NEXT:    [[SHUF:%.*]] = shufflevector <4 x i32> [[X:%.*]], <4 x i32> poison, <4 x i32> <i32 3, i32 2, i32 1, i32 0>
-; CHECK-NEXT:    [[ADD:%.*]] = add <4 x i32> [[SHUF]], [[SHUF]]
-; CHECK-NEXT:    [[REVSHUF:%.*]] = shufflevector <4 x i32> [[ADD]], <4 x i32> poison, <4 x i32> <i32 3, i32 2, i32 1, i32 0>
+; CHECK-NEXT:    [[REVSHUF:%.*]] = add <4 x i32> [[X]], [[X]]
 ; CHECK-NEXT:    [[ADD2:%.*]] = add <4 x i32> [[SHUF]], [[REVSHUF]]
 ; CHECK-NEXT:    ret <4 x i32> [[ADD2]]
 ;

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This removes the check that both operands of the original shuffle are
instructions, which is a relic from a previous version that held more variables
as Instructions.
@davemgreen davemgreen force-pushed the gh-shuffleToIdentity-removeinstnscheck branch from 45a435e to e7174f5 Compare May 29, 2024 08:15
@davemgreen davemgreen merged commit 93d8d74 into llvm:main May 29, 2024
4 of 7 checks passed
@davemgreen davemgreen deleted the gh-shuffleToIdentity-removeinstnscheck branch May 29, 2024 08:36
vg0204 pushed a commit to vg0204/llvm-project that referenced this pull request May 29, 2024
…ity (llvm#93543)

This removes the check that both operands of the original shuffle are
instructions, which is a relic from a previous version that held more
variables as Instructions.
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request May 31, 2024
xfails: Driver/clang-offload-wrapper.c

reverts: fails hipCatch2
93d8d74 [VectorCombine] Remove requirement for Instructions in shuffleToIdentity (llvm#93543)

Change-Id: I79fde99dfce78964c2cbcb2ec3233a8a90ed8751
@ronlieb
Copy link
Contributor

ronlieb commented May 31, 2024

@davemgreen we noticed a regression in two of our hip tests that seemed to be caused by this patch.
dont have a reproducer as of now. reporting it now. we will work on a test case , might take a day or two on our end.

@davemgreen
Copy link
Collaborator Author

Hi - yeah I can take a look if you have a reproducer. The regression is in the performance, not correctness? There is a quick cost-model added in #93937, in case that helps your problem.

searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Jun 19, 2024
…ity (llvm#93543)

This removes the check that both operands of the original shuffle are
instructions, which is a relic from a previous version that held more
variables as Instructions.

Change-Id: I2c095ceeb066d98a666952cfc52ddad523c16f2f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants