[VPlan] Support live-ins without underlying IR in type analysis. #80723

fhahn · 2024-02-05T18:46:21Z

A VPlan contains multiple live-ins without underlying IR, like VFxUF or VectorTripCount. Trying to infer the scalar type of those causes a crash at the moment.

Update VPTypeAnalysis to take a VPlan in its constructor and assign types to those live-ins up front. All those live-ins share the type of the canonical IV.

A VPlan contains multiple live-ins without underlying IR, like VFxUF or VectorTripCount. Trying to infer the scalar type of those causes a crash at the moment. Update VPTypeAnalysis to take a VPlan in its constructor and assign types to those live-ins up front. All those live-ins share the type of the canonical IV.

llvmbot · 2024-02-05T18:46:50Z

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

A VPlan contains multiple live-ins without underlying IR, like VFxUF or VectorTripCount. Trying to infer the scalar type of those causes a crash at the moment.

Update VPTypeAnalysis to take a VPlan in its constructor and assign types to those live-ins up front. All those live-ins share the type of the canonical IV.

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

4 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/VPlan.h (+4-1)
(modified) llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp (+11-6)
(modified) llvm/lib/Transforms/Vectorize/VPlanAnalysis.h (+2-1)
(modified) llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp (+4-4)

diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 162a3c4b195e5..43f10c315f2e9 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -238,7 +238,7 @@ struct VPTransformState {
                    DominatorTree *DT, IRBuilderBase &Builder,
                    InnerLoopVectorizer *ILV, VPlan *Plan, LLVMContext &Ctx)
       : VF(VF), UF(UF), LI(LI), DT(DT), Builder(Builder), ILV(ILV), Plan(Plan),
-        LVer(nullptr), TypeAnalysis(Ctx) {}
+        LVer(nullptr), TypeAnalysis(*Plan, Ctx) {}
 
   /// The chosen Vectorization and Unroll Factors of the loop being vectorized.
   ElementCount VF;
@@ -2932,6 +2932,9 @@ class VPlan {
     return BackedgeTakenCount;
   }
 
+  /// Return the backedge taken count of the original loop, if set.
+  VPValue *getBackedgeTakenCount() { return BackedgeTakenCount; }
+
   /// The vector trip count.
   VPValue &getVectorTripCount() { return VectorTripCount; }
 
diff --git a/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp b/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp
index b9ffe7e5b7af7..b71bac1546332 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp
@@ -35,12 +35,7 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
     CachedTypes[OtherV] = ResTy;
     return ResTy;
   }
-  case Instruction::ICmp: {
-    // TODO: Check if types for both operands agree. This also requires
-    // type-inference for the vector-trip-count, which is missing at the moment.
-    Type *ResTy = inferScalarType(R->getOperand(0));
-    return ResTy;
-  }
+  case Instruction::ICmp:
   case VPInstruction::FirstOrderRecurrenceSplice: {
     Type *ResTy = inferScalarType(R->getOperand(0));
     VPValue *OtherV = R->getOperand(1);
@@ -203,6 +198,16 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPReplicateRecipe *R) {
   llvm_unreachable("Unhandled opcode");
 }
 
+VPTypeAnalysis::VPTypeAnalysis(VPlan &Plan, LLVMContext &Ctx) : Ctx(Ctx) {
+  auto *CanIV = Plan.getCanonicalIV();
+  Type *CanIVTy = inferScalarType(CanIV);
+  CachedTypes[&Plan.getVectorTripCount()] = CanIVTy;
+  CachedTypes[&Plan.getVFxUF()] = CanIVTy;
+  CachedTypes[Plan.getTripCount()] = CanIVTy;
+  if (auto *BTC = Plan.getBackedgeTakenCount())
+    CachedTypes[BTC] = CanIVTy;
+}
+
 Type *VPTypeAnalysis::inferScalarType(const VPValue *V) {
   if (Type *CachedTy = CachedTypes.lookup(V))
     return CachedTy;
diff --git a/llvm/lib/Transforms/Vectorize/VPlanAnalysis.h b/llvm/lib/Transforms/Vectorize/VPlanAnalysis.h
index 7276641551ae8..6b218a39f605c 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanAnalysis.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanAnalysis.h
@@ -15,6 +15,7 @@ namespace llvm {
 
 class LLVMContext;
 class VPValue;
+class VPlan;
 class VPBlendRecipe;
 class VPInstruction;
 class VPWidenRecipe;
@@ -47,7 +48,7 @@ class VPTypeAnalysis {
   Type *inferScalarTypeForRecipe(const VPReplicateRecipe *R);
 
 public:
-  VPTypeAnalysis(LLVMContext &Ctx) : Ctx(Ctx) {}
+  VPTypeAnalysis(VPlan &Plan, LLVMContext &Ctx);
 
   /// Infer the type of \p V. Returns the scalar type of \p V.
   Type *inferScalarType(const VPValue *V);
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index 71f5285f90236..4f4febcd1403f 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -502,7 +502,7 @@ static VPValue *createScalarIVSteps(VPlan &Plan, const InductionDescriptor &ID,
   }
 
   // Truncate base induction if needed.
-  VPTypeAnalysis TypeInfo(SE.getContext());
+  VPTypeAnalysis TypeInfo(Plan, SE.getContext());
   Type *ResultTy = TypeInfo.inferScalarType(BaseIV);
   if (TruncI) {
     Type *TruncTy = TruncI->getType();
@@ -880,7 +880,7 @@ static void simplifyRecipe(VPRecipeBase &R, VPTypeAnalysis &TypeInfo) {
 #ifndef NDEBUG
     // Verify that the cached type info is for both A and its users is still
     // accurate by comparing it to freshly computed types.
-    VPTypeAnalysis TypeInfo2(TypeInfo.getContext());
+    VPTypeAnalysis TypeInfo2(*R.getParent()->getPlan(), TypeInfo.getContext());
     assert(TypeInfo.inferScalarType(A) == TypeInfo2.inferScalarType(A));
     for (VPUser *U : A->users()) {
       auto *R = dyn_cast<VPRecipeBase>(U);
@@ -901,7 +901,7 @@ static void simplifyRecipe(VPRecipeBase &R, VPTypeAnalysis &TypeInfo) {
 static void simplifyRecipes(VPlan &Plan, LLVMContext &Ctx) {
   ReversePostOrderTraversal<VPBlockDeepTraversalWrapper<VPBlockBase *>> RPOT(
       Plan.getEntry());
-  VPTypeAnalysis TypeInfo(Ctx);
+  VPTypeAnalysis TypeInfo(Plan, Ctx);
   for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>(RPOT)) {
     for (VPRecipeBase &R : make_early_inc_range(*VPBB)) {
       simplifyRecipe(R, TypeInfo);
@@ -922,7 +922,7 @@ void VPlanTransforms::truncateToMinimalBitwidths(
   // other uses have different types for their operands, making them invalidly
   // typed.
   DenseMap<VPValue *, VPWidenCastRecipe *> ProcessedTruncs;
-  VPTypeAnalysis TypeInfo(Ctx);
+  VPTypeAnalysis TypeInfo(Plan, Ctx);
   VPBasicBlock *PH = Plan.getEntry();
   for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>(
            vp_depth_first_deep(Plan.getVectorLoopRegion()))) {

fhahn · 2024-02-19T18:49:39Z

ping :)

ayalz · 2024-02-18T18:54:09Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp

+  CachedTypes[&Plan.getVFxUF()] = CanIVTy;
+  CachedTypes[Plan.getTripCount()] = CanIVTy;
+  if (auto *BTC = Plan.getBackedgeTakenCount())
+    CachedTypes[BTC] = CanIVTy;


These live-ins-w/o-an-underlying-value VPValues are temporary place-holders that should be replaced by recipes, as the boundary of VPlan's scope expands; or folded into constants (UF, times non-scalable VF). So fixing their type for now seems ok. Still, it may be better to pass and record the "Canonical Type" of (the start value - a zero that may be replaced - of) the Canonical IV directly, rather than call the recursive inferScalarType() during construction of VPTypeAnalsis.
VPTypeAnalysis::VPTypeAnalysis(Type *CanTy, LLVMContext &Ctx) : CanTy(CanTy), Ctx(Ctx) {}
Perhaps (also) record it in VPlan, as part of addCanonicalIVRecipes(); or at-least have VPlan provide a method to retrieve it.
Plus, have these VPValues lookup this type when needed, rather than listing them all here upfront, and maintaining this list as it changes?
(BTW, is Ctx needed?)
(Another option may be to record their type upon construction, using another subclass of VPValue, is better avoided being place-holders.)

Thanks, updated to take the CanonicalIVType as argument, instead of the whole Plan and avoid caching it as suggested!

The canonical IV type is available once it is added via the type of its start value. Updated patch uses getCanonicalIV()->getScalarType() to retrieve that type.

(BTW, is Ctx needed?)

It is used to construct i1 types for the results of compares at the moment. Now that we have access to a type, we could in theory use that type to retrieve a context, but it seems cleaner to store it explicitly.

ayalz · 2024-02-18T18:59:44Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp

+  if (auto *BTC = Plan.getBackedgeTakenCount())
+    CachedTypes[BTC] = CanIVTy;
+}
+
 Type *VPTypeAnalysis::inferScalarType(const VPValue *V) {
  if (Type *CachedTy = CachedTypes.lookup(V))
    return CachedTy;


An alternative to caching the pre-computed type for all live-ins-w/o-underlying-value, as raised above, may be to change below into:

if (V->isLiveIn()) { if (auto *IRValue = V->getLiveInIRValue()) return IRValue->getType(); return CanonicalType; }

Updated as suggested, thanks!

ayalz · 2024-02-18T20:38:40Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp

@@ -35,12 +35,7 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
    CachedTypes[OtherV] = ResTy;
    return ResTy;
  }
-  case Instruction::ICmp: {
-    // TODO: Check if types for both operands agree. This also requires
-    // type-inference for the vector-trip-count, which is missing at the moment.


nit: should have probably referred to backedge-taken-count, used by an ICmp to form the header mask, rather than vector-trip-count.

ayalz

Thanks for accommodating, this looks good to me, adding a couple of nits, worth explaining what CanonicalIVTy is used for, possibly along with a TODO to promote the temporary live-ins-w/o-underlying-value (which need it) into recipes.

ayalz · 2024-02-21T14:13:10Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  /// Return the backedge taken count of the original loop, if set.
+  VPValue *getBackedgeTakenCount() { return BackedgeTakenCount; }
+


Suggested change

/// Return the backedge taken count of the original loop, if set.

VPValue *getBackedgeTakenCount() { return BackedgeTakenCount; }

is this still needed?

Removed, thanks!

ayalz · 2024-02-21T14:14:24Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp

-    return V->getLiveInIRValue()->getType();
+  if (V->isLiveIn()) {
+    if (auto *IRValue = V->getLiveInIRValue())
+      return IRValue->getType();


nit: worth a comment to explain who's expected to use CanonicalIVTy.

Added a comment ,thanks!

ayalz · 2024-02-21T14:14:49Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.h

@@ -15,6 +15,7 @@ namespace llvm {

 class LLVMContext;
 class VPValue;
+class VPlan;


Suggested change

class VPlan;

is this still needed?

Dropped thanks.

ayalz · 2024-02-21T14:15:19Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.h

@@ -35,6 +36,7 @@ class Type;
 /// of the previously inferred types.
 class VPTypeAnalysis {
  DenseMap<const VPValue *, Type *> CachedTypes;
+  Type *CanonicalIVTy;


nit: worth a comment what this is for.

added ,thanks!

ayalz · 2024-02-21T14:16:05Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.h

@@ -47,7 +49,8 @@ class VPTypeAnalysis {
  Type *inferScalarTypeForRecipe(const VPReplicateRecipe *R);

 public:
-  VPTypeAnalysis(LLVMContext &Ctx) : Ctx(Ctx) {}
+  VPTypeAnalysis(Type *CanonicalIVTy, LLVMContext &Ctx)
+      : CanonicalIVTy(CanonicalIVTy), Ctx(Ctx) {}


nit (unrelated to this patch): does VPTypeAnalysis really need Ctx?

We could retrieve the context from a type separately when needed. Can do separately if preferred?

Is there any current use of Ctx? Sure, if this is indeed dead, should be eliminated separately.

The current uses are to create i1 & void types:

https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp#L63
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp#L163
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp#L189

fhahn requested review from ayalz and aniragil February 5, 2024 18:46

llvmbot added vectorization llvm:transforms labels Feb 5, 2024

ayalz reviewed Feb 20, 2024

View reviewed changes

fhahn added 2 commits February 21, 2024 09:51

Merge branch 'main' into vplan-type-analysis-live-ins

7d88cd3

!fixup store canonica iv type in VPTypeAnalysis.

c49ca00

ayalz approved these changes Feb 21, 2024

View reviewed changes

fhahn added 2 commits February 21, 2024 18:41

Merge branch 'main' into vplan-type-analysis-live-ins

e5f9fec

!fixup address latest comments, thanks!

7e9b7a1

fhahn merged commit 3d66d69 into llvm:main Feb 21, 2024
3 of 4 checks passed

fhahn deleted the vplan-type-analysis-live-ins branch February 21, 2024 19:37

fhahn mentioned this pull request Feb 26, 2024

[VPlan] Add VPComputeVFxUFRecipe, use for type inference #78309

Closed

arcbbb mentioned this pull request Mar 5, 2024

[LV] Decompose WidenIntOrFPInduction into phi and update recipes #82021

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VPlan] Support live-ins without underlying IR in type analysis. #80723

[VPlan] Support live-ins without underlying IR in type analysis. #80723

fhahn commented Feb 5, 2024

llvmbot commented Feb 5, 2024

fhahn commented Feb 19, 2024

ayalz Feb 18, 2024

fhahn Feb 21, 2024 •

edited

ayalz Feb 18, 2024

fhahn Feb 21, 2024

ayalz Feb 18, 2024

fhahn Feb 21, 2024

ayalz left a comment

ayalz Feb 21, 2024

fhahn Feb 21, 2024

ayalz Feb 21, 2024

fhahn Feb 21, 2024

ayalz Feb 21, 2024

fhahn Feb 21, 2024

ayalz Feb 21, 2024

fhahn Feb 21, 2024

ayalz Feb 21, 2024

fhahn Feb 21, 2024

ayalz Feb 22, 2024

fhahn Feb 22, 2024

		/// Return the backedge taken count of the original loop, if set.
		VPValue *getBackedgeTakenCount() { return BackedgeTakenCount; }

[VPlan] Support live-ins without underlying IR in type analysis. #80723

[VPlan] Support live-ins without underlying IR in type analysis. #80723

Conversation

fhahn commented Feb 5, 2024

llvmbot commented Feb 5, 2024

fhahn commented Feb 19, 2024

Choose a reason for hiding this comment

fhahn Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn Feb 21, 2024 •

edited