[VP] Refactor VectorBuilder to avoid layering violation. NFC by Mel-Chen · Pull Request #99276 · llvm/llvm-project

Mel-Chen · 2024-07-17T06:45:34Z

This patch refactors the handling of reduction to eliminate layering violations.

Introduced getReductionIntrinsicID in LoopUtils.h for mapping recurrence kinds to llvm.vector.reduce.* intrinsic IDs.
Updated VectorBuilder::createSimpleTargetReduction to accept llvm.vector.reduce.* intrinsic directly.
New function VPIntrinsic::getForIntrinsic for mapping intrinsic ID to the same functional VP intrinsic ID.

llvmbot · 2024-07-17T06:46:07Z

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: Mel Chen (Mel-Chen)

Changes

This patch refactors the handling of reduction to eliminate layering violations.

Introduced getReductionIntrinsicID in LoopUtils.h for mapping recurrence kinds to llvm.vector.reduce.* intrinsic IDs.
Updated VectorBuilder::createSimpleTargetReduction to accept llvm.vector.reduce.* intrinsic directly.
New function VPIntrinsic::getForIntrinsic for mapping intrinsic ID to the same functional VP intrinsic ID.

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

6 Files Affected:

(modified) llvm/include/llvm/IR/IntrinsicInst.h (+3)
(modified) llvm/include/llvm/IR/VectorBuilder.h (+2-3)
(modified) llvm/include/llvm/Transforms/Utils/LoopUtils.h (+4)
(modified) llvm/lib/IR/IntrinsicInst.cpp (+13)
(modified) llvm/lib/IR/VectorBuilder.cpp (+5-52)
(modified) llvm/lib/Transforms/Utils/LoopUtils.cpp (+42-2)

diff --git a/llvm/include/llvm/IR/IntrinsicInst.h b/llvm/include/llvm/IR/IntrinsicInst.h
index fe3f92da400f8..90db2b3dcebb3 100644
--- a/llvm/include/llvm/IR/IntrinsicInst.h
+++ b/llvm/include/llvm/IR/IntrinsicInst.h
@@ -569,6 +569,9 @@ class VPIntrinsic : public IntrinsicInst {
   /// The llvm.vp.* intrinsics for this instruction Opcode
   static Intrinsic::ID getForOpcode(unsigned OC);
 
+  /// The llvm.vp.reduce.* intrinsics for this intrinsic.
+  static Intrinsic::ID getForIntrinsic(Intrinsic::ID);
+
   // Whether \p ID is a VP intrinsic ID.
   static bool isVPIntrinsic(Intrinsic::ID);
 
diff --git a/llvm/include/llvm/IR/VectorBuilder.h b/llvm/include/llvm/IR/VectorBuilder.h
index 6af7f6075551d..4f45834abb5bb 100644
--- a/llvm/include/llvm/IR/VectorBuilder.h
+++ b/llvm/include/llvm/IR/VectorBuilder.h
@@ -15,7 +15,6 @@
 #ifndef LLVM_IR_VECTORBUILDER_H
 #define LLVM_IR_VECTORBUILDER_H
 
-#include <llvm/Analysis/IVDescriptors.h>
 #include <llvm/IR/IRBuilder.h>
 #include <llvm/IR/InstrTypes.h>
 #include <llvm/IR/Instruction.h>
@@ -100,11 +99,11 @@ class VectorBuilder {
                                  const Twine &Name = Twine());
 
   /// Emit a VP reduction intrinsic call for recurrence kind.
-  /// \param Kind        The kind of recurrence
+  /// \param RdxID       The intrinsic id of llvm.vector.reduce.*
   /// \param ValTy       The type of operand which the reduction operation is
   ///                    performed.
   /// \param VecOpArray  The operand list.
-  Value *createSimpleTargetReduction(RecurKind Kind, Type *ValTy,
+  Value *createSimpleTargetReduction(Intrinsic::ID RdxID, Type *ValTy,
                                      ArrayRef<Value *> VecOpArray,
                                      const Twine &Name = Twine());
 };
diff --git a/llvm/include/llvm/Transforms/Utils/LoopUtils.h b/llvm/include/llvm/Transforms/Utils/LoopUtils.h
index 1a878126aa082..f358a63ca78bb 100644
--- a/llvm/include/llvm/Transforms/Utils/LoopUtils.h
+++ b/llvm/include/llvm/Transforms/Utils/LoopUtils.h
@@ -358,6 +358,10 @@ bool canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT,
                         SinkAndHoistLICMFlags &LICMFlags,
                         OptimizationRemarkEmitter *ORE = nullptr);
 
+/// Returns the llvm.vector.reduce intrinsic that corresponds to the recurrence
+/// kind.
+Intrinsic::ID getReductionIntrinsicID(RecurKind RK);
+
 /// Returns the arithmetic instruction opcode used when expanding a reduction.
 unsigned getArithmeticReductionInstruction(Intrinsic::ID RdxID);
 
diff --git a/llvm/lib/IR/IntrinsicInst.cpp b/llvm/lib/IR/IntrinsicInst.cpp
index 64a14da55b15e..da3e751da832a 100644
--- a/llvm/lib/IR/IntrinsicInst.cpp
+++ b/llvm/lib/IR/IntrinsicInst.cpp
@@ -599,6 +599,19 @@ Intrinsic::ID VPIntrinsic::getForOpcode(unsigned IROPC) {
   return Intrinsic::not_intrinsic;
 }
 
+Intrinsic::ID VPIntrinsic::getForIntrinsic(Intrinsic::ID Id) {
+  switch (Id) {
+  default:
+    break;
+
+#define BEGIN_REGISTER_VP_INTRINSIC(VPID, ...) break;
+#define VP_PROPERTY_FUNCTIONAL_INTRINSIC(INTRIN) case Intrinsic::INTRIN:
+#define END_REGISTER_VP_INTRINSIC(VPID) return Intrinsic::VPID;
+#include "llvm/IR/VPIntrinsics.def"
+  }
+  return Intrinsic::not_intrinsic;
+}
+
 bool VPIntrinsic::canIgnoreVectorLengthParam() const {
   using namespace PatternMatch;
 
diff --git a/llvm/lib/IR/VectorBuilder.cpp b/llvm/lib/IR/VectorBuilder.cpp
index 5ff3082879895..8dbf25277bf5d 100644
--- a/llvm/lib/IR/VectorBuilder.cpp
+++ b/llvm/lib/IR/VectorBuilder.cpp
@@ -60,60 +60,13 @@ Value *VectorBuilder::createVectorInstruction(unsigned Opcode, Type *ReturnTy,
   return createVectorInstructionImpl(VPID, ReturnTy, InstOpArray, Name);
 }
 
-Value *VectorBuilder::createSimpleTargetReduction(RecurKind Kind, Type *ValTy,
+Value *VectorBuilder::createSimpleTargetReduction(Intrinsic::ID RdxID,
+                                                  Type *ValTy,
                                                   ArrayRef<Value *> InstOpArray,
                                                   const Twine &Name) {
-  Intrinsic::ID VPID;
-  switch (Kind) {
-  case RecurKind::Add:
-    VPID = Intrinsic::vp_reduce_add;
-    break;
-  case RecurKind::Mul:
-    VPID = Intrinsic::vp_reduce_mul;
-    break;
-  case RecurKind::And:
-    VPID = Intrinsic::vp_reduce_and;
-    break;
-  case RecurKind::Or:
-    VPID = Intrinsic::vp_reduce_or;
-    break;
-  case RecurKind::Xor:
-    VPID = Intrinsic::vp_reduce_xor;
-    break;
-  case RecurKind::FMulAdd:
-  case RecurKind::FAdd:
-    VPID = Intrinsic::vp_reduce_fadd;
-    break;
-  case RecurKind::FMul:
-    VPID = Intrinsic::vp_reduce_fmul;
-    break;
-  case RecurKind::SMax:
-    VPID = Intrinsic::vp_reduce_smax;
-    break;
-  case RecurKind::SMin:
-    VPID = Intrinsic::vp_reduce_smin;
-    break;
-  case RecurKind::UMax:
-    VPID = Intrinsic::vp_reduce_umax;
-    break;
-  case RecurKind::UMin:
-    VPID = Intrinsic::vp_reduce_umin;
-    break;
-  case RecurKind::FMax:
-    VPID = Intrinsic::vp_reduce_fmax;
-    break;
-  case RecurKind::FMin:
-    VPID = Intrinsic::vp_reduce_fmin;
-    break;
-  case RecurKind::FMaximum:
-    VPID = Intrinsic::vp_reduce_fmaximum;
-    break;
-  case RecurKind::FMinimum:
-    VPID = Intrinsic::vp_reduce_fminimum;
-    break;
-  default:
-    llvm_unreachable("No VPIntrinsic for this reduction");
-  }
+  auto VPID = VPIntrinsic::getForIntrinsic(RdxID);
+  assert(VPReductionIntrinsic::isVPReduction(VPID) &&
+         "No VPIntrinsic for this reduction");
   return createVectorInstructionImpl(VPID, ValTy, InstOpArray, Name);
 }
 
diff --git a/llvm/lib/Transforms/Utils/LoopUtils.cpp b/llvm/lib/Transforms/Utils/LoopUtils.cpp
index ff93035ce0652..02d0860acc622 100644
--- a/llvm/lib/Transforms/Utils/LoopUtils.cpp
+++ b/llvm/lib/Transforms/Utils/LoopUtils.cpp
@@ -918,6 +918,44 @@ bool llvm::hasIterationCountInvariantInParent(Loop *InnerLoop,
   return true;
 }
 
+Intrinsic::ID llvm::getReductionIntrinsicID(RecurKind RK) {
+  switch (RK) {
+  default:
+    llvm_unreachable("Unexpected recurrence kind");
+  case RecurKind::Add:
+    return Intrinsic::vector_reduce_add;
+  case RecurKind::Mul:
+    return Intrinsic::vector_reduce_mul;
+  case RecurKind::And:
+    return Intrinsic::vector_reduce_and;
+  case RecurKind::Or:
+    return Intrinsic::vector_reduce_or;
+  case RecurKind::Xor:
+    return Intrinsic::vector_reduce_xor;
+  case RecurKind::FMulAdd:
+  case RecurKind::FAdd:
+    return Intrinsic::vector_reduce_fadd;
+  case RecurKind::FMul:
+    return Intrinsic::vector_reduce_fmul;
+  case RecurKind::SMax:
+    return Intrinsic::vector_reduce_smax;
+  case RecurKind::SMin:
+    return Intrinsic::vector_reduce_smin;
+  case RecurKind::UMax:
+    return Intrinsic::vector_reduce_umax;
+  case RecurKind::UMin:
+    return Intrinsic::vector_reduce_umin;
+  case RecurKind::FMax:
+    return Intrinsic::vector_reduce_fmax;
+  case RecurKind::FMin:
+    return Intrinsic::vector_reduce_fmin;
+  case RecurKind::FMaximum:
+    return Intrinsic::vector_reduce_fmaximum;
+  case RecurKind::FMinimum:
+    return Intrinsic::vector_reduce_fminimum;
+  }
+}
+
 unsigned llvm::getArithmeticReductionInstruction(Intrinsic::ID RdxID) {
   switch (RdxID) {
   case Intrinsic::vector_reduce_fadd:
@@ -1197,12 +1235,13 @@ Value *llvm::createSimpleTargetReduction(VectorBuilder &VBuilder, Value *Src,
   RecurKind Kind = Desc.getRecurrenceKind();
   assert(!RecurrenceDescriptor::isAnyOfRecurrenceKind(Kind) &&
          "AnyOf reduction is not supported.");
+  Intrinsic::ID Id = getReductionIntrinsicID(Kind);
   auto *SrcTy = cast<VectorType>(Src->getType());
   Type *SrcEltTy = SrcTy->getElementType();
   Value *Iden =
       Desc.getRecurrenceIdentity(Kind, SrcEltTy, Desc.getFastMathFlags());
   Value *Ops[] = {Iden, Src};
-  return VBuilder.createSimpleTargetReduction(Kind, SrcTy, Ops);
+  return VBuilder.createSimpleTargetReduction(Id, SrcTy, Ops);
 }
 
 Value *llvm::createTargetReduction(IRBuilderBase &B,
@@ -1242,9 +1281,10 @@ Value *llvm::createOrderedReduction(VectorBuilder &VBuilder,
   assert(Src->getType()->isVectorTy() && "Expected a vector type");
   assert(!Start->getType()->isVectorTy() && "Expected a scalar type");
 
+  Intrinsic::ID Id = getReductionIntrinsicID(RecurKind::FAdd);
   auto *SrcTy = cast<VectorType>(Src->getType());
   Value *Ops[] = {Start, Src};
-  return VBuilder.createSimpleTargetReduction(RecurKind::FAdd, SrcTy, Ops);
+  return VBuilder.createSimpleTargetReduction(Id, SrcTy, Ops);
 }
 
 void llvm::propagateIRFlags(Value *I, ArrayRef<Value *> VL, Value *OpValue,

frasercrmck · 2024-07-17T09:09:52Z

llvm/include/llvm/IR/IntrinsicInst.h

  /// The llvm.vp.* intrinsics for this instruction Opcode
  static Intrinsic::ID getForOpcode(unsigned OC);

+  /// The llvm.vp.reduce.* intrinsics for this intrinsic.


intrinsic for this intrinsic.

Also, doesn't this function now return more than just reduction intrinsics? It handles all intrinsic->vp.intrinsic mappings we have.

Should we unit test this function?

Might want to document what happens when passed VP intrinsics, too

165f0bc Fix the comment.
be39dca This commit defined the behavior when passed VP intrinsics. I define the behavior to return the passed VP intrinsic ID directly. Please let me know if you feel this is unsuitable.
86f96f1 Add round trip tests for getForIntrinsic.

llvm/include/llvm/IR/VectorBuilder.h

frasercrmck · 2024-07-17T09:29:29Z

llvm/lib/IR/IntrinsicInst.cpp

  return Intrinsic::not_intrinsic;
 }

+Intrinsic::ID VPIntrinsic::getForIntrinsic(Intrinsic::ID Id) {


Could this be made constexpr? I see you're using it with a constant RecurKind::FAdd later on.

chapuni

Looks good from my side. Thanks.

With this, aa94a43 may be reverted.

chapuni · 2024-07-24T02:18:59Z

Could anyone take another look?
Or we have to revert #90184 to fix the layering violation in the release/19.x

topperc · 2024-07-24T02:41:42Z

llvm/lib/Transforms/Utils/LoopUtils.cpp

  return true;
 }

+constexpr Intrinsic::ID llvm::getReductionIntrinsicID(RecurKind RK) {


This function seems to only be used to create a VP reduction. Why can't we rename this to getVPReductionIntrinsicID and just return the VP intrinsic ID directly? Why do we need to convert from non-VP to VP?

Is this because we're trying to copy the createVectorInstruction design where we pass the non-VP opcode?

I guess we will need this functionality for EVL vectorization of other intrinsics so maybe this is the right design.

topperc · 2024-07-24T03:00:25Z

llvm/unittests/IR/VPIntrinsicTest.cpp

+    ASSERT_EQ(*RoundTripIntrinsicID, IntrinsicID);
+    ++FullTripCounts;
+  }
+  ASSERT_NE(FullTripCounts, 0u);


Why is this a counter if it only checks non-zero?

9f8748e
There are two ways to improve this:

If we only need to check whether at least one full trip is completed, boolean is good enough.

Perform a detailed check of the number of full trips executed. Currently, there will be a total of 54 full trips.

For now, I will adopt the first way.
If it needs to be changed to the second way, I have a question: Is there a simple way, besides using
#define VP_PROPERTY_FUNCTIONAL_INTRINSIC(INTRIN) FullTripCountsAns++;, to obtain the total number of corresponding equivalent non-VP intrinsics in VPIntrinsic?

This patch refactors the handling of reduction to eliminate layering violations. * Introduced `getReductionIntrinsicID` in LoopUtils.h for mapping recurrence kinds to llvm.vector.reduce.* intrinsic IDs. * Updated `VectorBuilder::createSimpleTargetReduction` to accept llvm.vector.reduce.* intrinsic directly. * New function `VPIntrinsic::getForIntrinsic` for mapping intrinsic ID to the same functional VP intrinsic ID.

topperc

LGTM

Summary: This patch refactors the handling of reduction to eliminate layering violations. * Introduced `getReductionIntrinsicID` in LoopUtils.h for mapping recurrence kinds to llvm.vector.reduce.* intrinsic IDs. * Updated `VectorBuilder::createSimpleTargetReduction` to accept llvm.vector.reduce.* intrinsic directly. * New function `VPIntrinsic::getForIntrinsic` for mapping intrinsic ID to the same functional VP intrinsic ID. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250539

Since #99276 has been landed, the dependency has become redundant. This reverts commit aa94a43. (llvmorg-19-init-17718-gaa94a43178e1)

) This patch refactors the handling of reduction to eliminate layering violations. * Introduced `getReductionIntrinsicID` in LoopUtils.h for mapping recurrence kinds to llvm.vector.reduce.* intrinsic IDs. * Updated `VectorBuilder::createSimpleTargetReduction` to accept llvm.vector.reduce.* intrinsic directly. * New function `VPIntrinsic::getForIntrinsic` for mapping intrinsic ID to the same functional VP intrinsic ID. (cherry picked from commit 6d12b3f)