Skip to content

[InstCombine] Pull vector reverse through fneg #146349

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
merged 1 commit into from
Jun 30, 2025

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Jun 30, 2025

This follows on from #144933 (comment), and allows us to remove the reverse (fneg (reverse x)) combine.

A separate patch will handle the case for fabs. I haven't checked if we perform this canonicalization for either unops or binops for vp.reverse

This follows on from llvm#144933 (comment), and allows us to remove the reverse (fneg (reverse x)) combine.

A separate patch will handle the case for fabs. I haven't checked if we perform this canonicalization for either unops or binops for vp.reverse
@lukel97 lukel97 requested a review from nikic as a code owner June 30, 2025 13:35
@lukel97 lukel97 requested review from nikic, dtcxzyw, preames and RKSimon June 30, 2025 13:35
@llvmbot llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Jun 30, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 30, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Luke Lau (lukel97)

Changes

This follows on from #144933 (comment), and allows us to remove the reverse (fneg (reverse x)) combine.

A separate patch will handle the case for fabs. I haven't checked if we perform this canonicalization for either unops or binops for vp.reverse


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

3 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp (+6)
  • (modified) llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp (+1-17)
  • (modified) llvm/test/Transforms/InstCombine/vector-reverse.ll (+3-3)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
index e721f0cd5f9e3..f727eb0a63e05 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
@@ -3049,6 +3049,12 @@ Instruction *InstCombinerImpl::visitFNeg(UnaryOperator &I) {
   if (match(OneUse, m_Shuffle(m_Value(X), m_Poison(), m_Mask(Mask))))
     return new ShuffleVectorInst(Builder.CreateFNegFMF(X, &I), Mask);
 
+  // fneg (reverse x) --> reverse (fneg x)
+  if (match(OneUse, m_VecReverse(m_Value(X)))) {
+    Value *Reverse = Builder.CreateVectorReverse(Builder.CreateFNegFMF(X, &I));
+    return replaceInstUsesWith(I, Reverse);
+  }
+
   return nullptr;
 }
 
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index e33d111167c04..47cc424f4ac07 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -3549,23 +3549,6 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
     }
     break;
   }
-  case Intrinsic::vector_reverse: {
-    Value *Vec = II->getArgOperand(0);
-    // Note: We canonicalize reverse after binops, so we don't need a
-    // corresponding binop case here. TODO: Consider canonicalizing
-    // reverse after fneg?
-
-    // rev(unop rev(X)) --> unop X
-    Value *X;
-    if (match(Vec, m_OneUse(m_UnOp(m_VecReverse(m_Value(X)))))) {
-      auto *OldUnOp = cast<UnaryOperator>(Vec);
-      auto *NewUnOp = UnaryOperator::CreateWithCopiedFlags(
-          OldUnOp->getOpcode(), X, OldUnOp, OldUnOp->getName(),
-          II->getIterator());
-      return replaceInstUsesWith(CI, NewUnOp);
-    }
-    break;
-  }
   case Intrinsic::experimental_vp_reverse: {
     Value *X;
     Value *Vec = II->getArgOperand(0);
@@ -3573,6 +3556,7 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
     if (!match(Mask, m_AllOnes()))
       break;
     Value *EVL = II->getArgOperand(2);
+    // TODO: Canonicalize experimental.vp.reverse after unop/binops?
     // rev(unop rev(X)) --> unop X
     if (match(Vec,
               m_OneUse(m_UnOp(m_Intrinsic<Intrinsic::experimental_vp_reverse>(
diff --git a/llvm/test/Transforms/InstCombine/vector-reverse.ll b/llvm/test/Transforms/InstCombine/vector-reverse.ll
index c9c68d2241b34..8421d6082dc44 100644
--- a/llvm/test/Transforms/InstCombine/vector-reverse.ll
+++ b/llvm/test/Transforms/InstCombine/vector-reverse.ll
@@ -161,9 +161,9 @@ define <vscale x 4 x i32> @binop_reverse_splat_LHS_1(<vscale x 4 x i32> %a, i32
 
 define <vscale x 4 x float> @unop_reverse(<vscale x 4 x float> %a) {
 ; CHECK-LABEL: @unop_reverse(
-; CHECK-NEXT:    [[A_REV:%.*]] = tail call <vscale x 4 x float> @llvm.vector.reverse.nxv4f32(<vscale x 4 x float> [[A:%.*]])
-; CHECK-NEXT:    [[NEG:%.*]] = fneg fast <vscale x 4 x float> [[A_REV]]
-; CHECK-NEXT:    ret <vscale x 4 x float> [[NEG]]
+; CHECK-NEXT:    [[NEG:%.*]] = fneg fast <vscale x 4 x float> [[A_REV:%.*]]
+; CHECK-NEXT:    [[NEG1:%.*]] = call <vscale x 4 x float> @llvm.vector.reverse.nxv4f32(<vscale x 4 x float> [[NEG]])
+; CHECK-NEXT:    ret <vscale x 4 x float> [[NEG1]]
 ;
   %a.rev = tail call <vscale x 4 x float> @llvm.vector.reverse.nxv4f32(<vscale x 4 x float> %a)
   %neg = fneg fast <vscale x 4 x float> %a.rev

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

@lukel97 lukel97 merged commit 6f7370c into llvm:main Jun 30, 2025
9 of 10 checks passed
lukel97 added a commit to lukel97/llvm-project that referenced this pull request Jun 30, 2025
This is the intrinsic version of llvm#146349, and handles fabs as well as other intrinsics.

It's largely a copy of InstCombinerImpl::foldShuffledIntrinsicOperands but a bit simpler since we don't need to find a common mask.

Creating a separate function seems to be cleaner than trying to shoehorn it into the existing one.
lukel97 added a commit that referenced this pull request Jul 1, 2025
This is the intrinsic version of #146349, and handles fabs as well as
other intrinsics.

It's largely a copy of InstCombinerImpl::foldShuffledIntrinsicOperands
but a bit simpler since we don't need to find a common mask.

Creating a separate function seems to be cleaner than trying to shoehorn
it into the existing one.
rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
This follows on from
llvm#144933 (comment),
and allows us to remove the reverse (fneg (reverse x)) combine.

A separate patch will handle the case for fabs. I haven't checked if we
perform this canonicalization for either unops or binops for vp.reverse
rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
This follows on from
llvm#144933 (comment),
and allows us to remove the reverse (fneg (reverse x)) combine.

A separate patch will handle the case for fabs. I haven't checked if we
perform this canonicalization for either unops or binops for vp.reverse
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants