[VPlan] Verify scalar types in VPlanVerifier. NFCI #122679

lukel97 · 2025-01-13T10:05:02Z

VTypeAnalysis contains some assertions which can be useful for reasoning that the types of various operands match.

This patch teaches VPlanVerifier to invoke VTypeAnalysis to check them, and catches some issues with VPInstruction types that are also fixed here:

Handles the missing cases for CalculateTripCountMinusVF, CanonicalIVIncrementForPart and AnyOf
Fixes ICmp and ActiveLaneMask to return i1 (to align with icmp and @llvm.get.active.lane.mask in the LangRef)

The VPlanVerifier unit tests also need to be fleshed out a bit more to satisfy the stricter assertions

llvmbot · 2025-01-13T10:05:37Z

@llvm/pr-subscribers-vectorizers

@llvm/pr-subscribers-llvm-transforms

Author: Luke Lau (lukel97)

Changes

VTypeAnalysis contains some assertions which can be useful for reasoning that the types of various operands match.

This patch teaches VPlanVerifier to invoke VTypeAnalysis to check them, and catches some issues with VPInstruction types that are also fixed here:

Handles the missing cases for CalculateTripCountMinusVF, CanonicalIVIncrementForPart and AnyOf
Fixes LogicalAnd to return its operands' type (to align with and in the LangRef)
Fixes ICmp and ActiveLaneMask to return i1 (to align with icmp and @llvm.get.active.lane.mask in the LangRef)

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

2 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp (+8-3)
(modified) llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp (+9-2)

diff --git a/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp b/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp
index 35497a7431f766..bc84757420e834 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp
@@ -60,7 +60,10 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
   }
   case Instruction::ICmp:
   case VPInstruction::ActiveLaneMask:
-    return inferScalarType(R->getOperand(1));
+    assert(inferScalarType(R->getOperand(0)) ==
+               inferScalarType(R->getOperand(1)) &&
+           "different types inferred for different operands");
+    return IntegerType::get(Ctx, 1);
   case VPInstruction::ComputeReductionResult: {
     auto *PhiR = cast<VPReductionPHIRecipe>(R->getOperand(0));
     auto *OrigPhi = cast<PHINode>(PhiR->getUnderlyingValue());
@@ -71,6 +74,10 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
   case VPInstruction::FirstOrderRecurrenceSplice:
   case VPInstruction::Not:
   case VPInstruction::ResumePhi:
+  case VPInstruction::CalculateTripCountMinusVF:
+  case VPInstruction::CanonicalIVIncrementForPart:
+  case VPInstruction::AnyOf:
+  case VPInstruction::LogicalAnd:
     return SetResultTyFromOp();
   case VPInstruction::ExtractFromEnd: {
     Type *BaseTy = inferScalarType(R->getOperand(0));
@@ -78,8 +85,6 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
       return VecTy->getElementType();
     return BaseTy;
   }
-  case VPInstruction::LogicalAnd:
-    return IntegerType::get(Ctx, 1);
   case VPInstruction::PtrAdd:
     // Return the type based on the pointer argument (i.e. first operand).
     return inferScalarType(R->getOperand(0));
diff --git a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
index be420a873bef52..647c03ec69ba4f 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
@@ -26,6 +26,7 @@ using namespace llvm;
 namespace {
 class VPlanVerifier {
   const VPDominatorTree &VPDT;
+  VPTypeAnalysis &TypeInfo;
 
   SmallPtrSet<BasicBlock *, 8> WrappedIRBBs;
 
@@ -58,7 +59,8 @@ class VPlanVerifier {
   bool verifyRegionRec(const VPRegionBlock *Region);
 
 public:
-  VPlanVerifier(VPDominatorTree &VPDT) : VPDT(VPDT) {}
+  VPlanVerifier(VPDominatorTree &VPDT, VPTypeAnalysis &TypeInfo)
+      : VPDT(VPDT), TypeInfo(TypeInfo) {}
 
   bool verify(const VPlan &Plan);
 };
@@ -195,6 +197,9 @@ bool VPlanVerifier::verifyVPBasicBlock(const VPBasicBlock *VPBB) {
       return false;
     }
     for (const VPValue *V : R.definedValues()) {
+      // Verify that recipes' operands have matching types.
+      TypeInfo.inferScalarType(V);
+
       for (const VPUser *U : V->users()) {
         auto *UI = dyn_cast<VPRecipeBase>(U);
         // TODO: check dominance of incoming values for phis properly.
@@ -406,6 +411,8 @@ bool VPlanVerifier::verify(const VPlan &Plan) {
 bool llvm::verifyVPlanIsValid(const VPlan &Plan) {
   VPDominatorTree VPDT;
   VPDT.recalculate(const_cast<VPlan &>(Plan));
-  VPlanVerifier Verifier(VPDT);
+  VPTypeAnalysis TypeInfo(
+      const_cast<VPlan &>(Plan).getCanonicalIV()->getScalarType());
+  VPlanVerifier Verifier(VPDT, TypeInfo);
   return Verifier.verify(Plan);
 }

lukel97 · 2025-01-13T10:23:27Z

I should have mentioned that the fixes in this PR come from new assertions that are thrown when calling inferScalarType in VPlanVerifier

david-arm · 2025-01-13T16:39:35Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp

@@ -71,15 +74,17 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
  case VPInstruction::FirstOrderRecurrenceSplice:
  case VPInstruction::Not:
  case VPInstruction::ResumePhi:
+  case VPInstruction::CalculateTripCountMinusVF:
+  case VPInstruction::CanonicalIVIncrementForPart:
+  case VPInstruction::AnyOf:


The documentation for AnyOf says

// Returns a scalar boolean value, which is true if any lane of its single // operand is true.

which implies to me that the input does not have to be a vector of i1 types. For example, the input could be a vector of i32s and any lane that's non-zero could lead to a return value of true. However, the code to handle AnyOf in VPInstruction::generate certainly matches up with your change, i.e.

case VPInstruction::AnyOf: { Value *A = State.get(getOperand(0)); return Builder.CreateOrReduce(A); }

I wonder if it's also worth tightening up the documentation of AnyOf to say something like:

// Returns a scalar boolean value, which is true if any lane of its only // boolean vector operand is true.

david-arm · 2025-01-13T16:49:33Z

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp

@@ -195,6 +197,9 @@ bool VPlanVerifier::verifyVPBasicBlock(const VPBasicBlock *VPBB) {
      return false;
    }
    for (const VPValue *V : R.definedValues()) {
+      // Verify that recipes' operands have matching types.
+      TypeInfo.inferScalarType(V);


It looks like you're dropping the return value here so I'm not sure what exactly you're verifying. Are you essentially relying upon the user building with asserts to catch any issues? It feels like we should be doing something here and printing out to errs() if something looks wrong?

Good point. I'm trying to exercise the assertions in VTypeAnalysis, and I guess it was working because all the callers of verifyVPlanIsValid happen to already be asserts.

Maybe it would make more sense just to pull this out of VPlanVerifier and instead at the call sites add something like:

[[maybe_unused]] VTypeAnalysis TypeInfo(Plan->getCanonicalIV()->getScalarType()); assert(TypeInfo.inferScalarType(V));

i.e. use assert to invoke more asserts.

IIUC the check here would catch violations via the inference assertions as well as uncovered cases. I think it should be fine to rely on the internal consistency checks and maybe check here that the result is non-null and error if it is null? Not sure if we would ever return nullptr on an uncovered case though.

We hit either an llvm_unreachable or fail an assert if there's an uncovered case:

Type *VPTypeAnalysis::inferScalarType(const VPValue *V) { Type *ResultTy = ... assert(ResultTy && "could not infer type for the given VPValue"); CachedTypes[V] = ResultTy; return ResultTy; }

So I don't think it will ever return null. Do we still want to move this out of VPlanAnalysis then?

I guess if the verifier is only ever being called via an assert anyway then this is fine. I was also worried that the compiler may just drop the call to inferScalarType because the result is unused. However, it does look like inferScalarType is not marked as const and it does modify the cache so it should be fine. I guess it doesn't do any harm to add some code to report an error if it returns nullptr, even though we know it won't. :) That at least ensures the code gets executed.

That makes sense, I've added in some error reporting code to have it align with the rest of VPlanVerifier. Lets hope it never gets called :)

fhahn · 2025-01-13T18:47:33Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp

+  case VPInstruction::CalculateTripCountMinusVF:
+  case VPInstruction::CanonicalIVIncrementForPart:
+  case VPInstruction::AnyOf:
+  case VPInstruction::LogicalAnd:


Are there any uses that use LogicalAnd for non-bools? I think the intention is to be only used with bools (LogicalAnd is a poison-safe version of regular bitwise AND for bools). I think that matches the meaning of LogicalAnd in other parts of LLVM (but different to general AND)

It doesn't look like there is, I think I changed in this when trying to hunt down the cause of the other assertions. I'll change it back and add an assert that its operand is also a bool

fhahn · 2025-01-13T18:48:11Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp

+    assert(inferScalarType(R->getOperand(0)) ==
+               inferScalarType(R->getOperand(1)) &&
+           "different types inferred for different operands");
+    return IntegerType::get(Ctx, 1);


Is this caught by the verifier?

Yeah in about 34 of the test cases, it mainly triggers when checking the operands of an or VPInstruction. The below is from llvm/test/Transforms/LoopVectorize/single_early_exit.ll and triggers the asesrtion on vp<%11>:

Live-in vp<%1> = vector-trip-count <x1> vector loop: { vector.body: EMIT vp<%3> = CANONICAL-INDUCTION ir<0>, vp<%index.next> vp<%4> = DERIVED-IV ir<3> + vp<%3> * ir<1> vp<%5> = SCALAR-STEPS vp<%4>, ir<1> CLONE ir<%arrayidx> = getelementptr inbounds ir<%p1>, vp<%5> vp<%6> = vector-pointer ir<%arrayidx> WIDEN ir<%ld1> = load vp<%6> CLONE ir<%arrayidx1> = getelementptr inbounds ir<%p2>, vp<%5> vp<%7> = vector-pointer ir<%arrayidx1> WIDEN ir<%ld2> = load vp<%7> WIDEN ir<%cmp3> = icmp eq ir<%ld1>, ir<%ld2> EMIT vp<%index.next> = add nuw vp<%3>, vp<%0> EMIT vp<%8> = not ir<%cmp3> EMIT vp<%9> = any-of vp<%8> EMIT vp<%10> = icmp eq vp<%index.next>, vp<%1> EMIT vp<%11> = or vp<%9>, vp<%10> EMIT branch-on-cond vp<%11> No successors }

EMIT vp<%10> = icmp has an inferred scalar type of i64 from the vector-trip-count vp<%1>.
But vp<%9> gets i1 from the WIDEN ir<%cmp3> = icmp.

I think the EMIT icmp should also be i1, to bring it inline with VPWidenRecipe:

Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPWidenRecipe *R) { unsigned Opcode = R->getOpcode(); ... switch (Opcode) { case Instruction::ICmp: case Instruction::FCmp: return IntegerType::get(Ctx, 1);

fhahn · 2025-01-13T18:50:55Z

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp

@@ -195,6 +197,9 @@ bool VPlanVerifier::verifyVPBasicBlock(const VPBasicBlock *VPBB) {
      return false;
    }
    for (const VPValue *V : R.definedValues()) {
+      // Verify that recipes' operands have matching types.
+      TypeInfo.inferScalarType(V);


IIUC the check here would catch violations via the inference assertions as well as uncovered cases. I think it should be fine to rely on the internal consistency checks and maybe check here that the result is non-null and error if it is null? Not sure if we would ever return nullptr on an uncovered case though.

I've been exploring verifying the VPlan before and after the EVL transformation steps, and noticed that the VPlan comes out in an invalid state between construction and optimisation. In adjustRecipesForReductions, we leave behind some dead recipes which are invalid: 1) When we replace a link with a reduction recipe, the old link ends up becoming a use-before-def: WIDEN ir<%l7> = add ir<%sum.02>, ir<%indvars.iv>.1 WIDEN ir<%l8> = add ir<%l7>.1, ir<%l3> WIDEN ir<%l9> = add ir<%l8>.1, ir<%l5> ... REDUCE ir<%l7>.1 = ir<%sum.02> + reduce.add (ir<%indvars.iv>.1) REDUCE ir<%l8>.1 = ir<%l7>.1 + reduce.add (ir<%l3>) REDUCE ir<%l9>.1 = ir<%l8>.1 + reduce.add (ir<%l5>) 2) When transforming an AnyOf reduction phi to a boolean, we leave behind a select with mismatching operand types, which will trigger the assertions in VTypeAnalysis after #122679 This adds an extra verification step and deletes the dead recipes eagerly to keep the plan valid.

VTypeAnalysis contains some assertions which can be useful for reasoning that the types of various operands match. This patch teaches VPlanVerifier to invoke VTypeAnalysis to check them, and catches some issues with VPInstruction types that are also fixed here: * Handles the missing cases for CalculateTripCountMinusVF, CanonicalIVIncrementForPart and AnyOf * Fixes LogicalAnd to return its operands' type (to align with `and` in the LangRef) * Fixes ICmp and ActiveLaneMask to return i1 (to align with `icmp` and `@llvm.get.active.lane.mask` in the LangRef)

…ypes

Which hopefully it never will!

fhahn · 2025-01-14T14:42:13Z

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp

@@ -195,6 +197,12 @@ bool VPlanVerifier::verifyVPBasicBlock(const VPBasicBlock *VPBB) {
      return false;
    }
    for (const VPValue *V : R.definedValues()) {
+      // Verify that recipes' operands have matching types.


Might be more precise to say something like below. The code here just checks that the type is non-null, matching types are only check internally by inferScalarType.

Suggested change

// Verify that recipes' operands have matching types.

// Verify that we can infer a scalar type for each defined value. With assertions enabled, inferScalarType will perform some consistency checks during type inference.

david-arm

LGTM! This seems sensible to me, but maybe wait a day in case @fhahn has more comments?

fhahn · 2025-01-15T20:01:02Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp

+    assert(inferScalarType(R->getOperand(0))->isIntegerTy(1) &&
+           "LogicalAnd operand should be bool");


Should this check both operands?

fhahn

LGTM, thanks

I've been exploring verifying the VPlan before and after the EVL transformation steps, and noticed that the VPlan comes out in an invalid state between construction and optimisation. In adjustRecipesForReductions, we leave behind some dead recipes which are invalid: 1) When we replace a link with a reduction recipe, the old link ends up becoming a use-before-def: WIDEN ir<%l7> = add ir<%sum.02>, ir<%indvars.iv>.1 WIDEN ir<%l8> = add ir<%l7>.1, ir<%l3> WIDEN ir<%l9> = add ir<%l8>.1, ir<%l5> ... REDUCE ir<%l7>.1 = ir<%sum.02> + reduce.add (ir<%indvars.iv>.1) REDUCE ir<%l8>.1 = ir<%l7>.1 + reduce.add (ir<%l3>) REDUCE ir<%l9>.1 = ir<%l8>.1 + reduce.add (ir<%l5>) 2) When transforming an AnyOf reduction phi to a boolean, we leave behind a select with mismatching operand types, which will trigger the assertions in VTypeAnalysis after llvm#122679 This adds an extra verification step and deletes the dead recipes eagerly to keep the plan valid.

VTypeAnalysis contains some assertions which can be useful for reasoning that the types of various operands match. This patch teaches VPlanVerifier to invoke VTypeAnalysis to check them, and catches some issues with VPInstruction types that are also fixed here: * Handles the missing cases for CalculateTripCountMinusVF, CanonicalIVIncrementForPart and AnyOf * Fixes ICmp and ActiveLaneMask to return i1 (to align with `icmp` and `@llvm.get.active.lane.mask` in the LangRef) The VPlanVerifier unit tests also need to be fleshed out a bit more to satisfy the stricter assertions

lukel97 requested review from ayalz, fhahn, LiqinWeng and Mel-Chen January 13, 2025 10:05

llvmbot added vectorizers llvm:transforms labels Jan 13, 2025

david-arm reviewed Jan 13, 2025

View reviewed changes

fhahn reviewed Jan 13, 2025

View reviewed changes

lukel97 mentioned this pull request Jan 14, 2025

[VPlan] Verify plan before optimizations. NFC #122678

Merged

lukel97 added 5 commits January 14, 2025 12:51

Fix verifier unit tests by filling out stuff necessary for corrrect t…

f7b716d

…ypes

Change back LogicalAnd and add assert for its operand

561b1a9

Update comment on AnyOf

f18d8db

Report an error if inferScalarType returns nullptr

6ad47a9

Which hopefully it never will!

lukel97 force-pushed the loop-vectorize/verify-types branch from 1b6111a to 6ad47a9 Compare January 14, 2025 14:38

fhahn reviewed Jan 14, 2025

View reviewed changes

Update comment

71ec919

david-arm approved these changes Jan 15, 2025

View reviewed changes

fhahn reviewed Jan 15, 2025

View reviewed changes

Check other LogicalAnd operand

d7fec40

fhahn approved these changes Jan 16, 2025

View reviewed changes

lukel97 merged commit 5c15caa into llvm:main Jan 16, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VPlan] Verify scalar types in VPlanVerifier. NFCI #122679

[VPlan] Verify scalar types in VPlanVerifier. NFCI #122679

lukel97 commented Jan 13, 2025 •

edited

Loading

llvmbot commented Jan 13, 2025 •

edited

Loading

lukel97 commented Jan 13, 2025

david-arm Jan 13, 2025

david-arm Jan 13, 2025

lukel97 Jan 13, 2025

fhahn Jan 13, 2025

lukel97 Jan 14, 2025

david-arm Jan 14, 2025

lukel97 Jan 14, 2025

fhahn Jan 13, 2025

lukel97 Jan 14, 2025 •

edited

Loading

fhahn Jan 13, 2025

lukel97 Jan 14, 2025

fhahn Jan 13, 2025

fhahn Jan 14, 2025

david-arm left a comment

fhahn Jan 15, 2025

lukel97 Jan 16, 2025

fhahn left a comment

	// Verify that recipes' operands have matching types.
	// Verify that we can infer a scalar type for each defined value. With assertions enabled, inferScalarType will perform some consistency checks during type inference.

		assert(inferScalarType(R->getOperand(0))->isIntegerTy(1) &&
		"LogicalAnd operand should be bool");

[VPlan] Verify scalar types in VPlanVerifier. NFCI #122679

[VPlan] Verify scalar types in VPlanVerifier. NFCI #122679

Conversation

lukel97 commented Jan 13, 2025 • edited Loading

llvmbot commented Jan 13, 2025 • edited Loading

lukel97 commented Jan 13, 2025

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

lukel97 Jan 14, 2025 • edited Loading

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

david-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn left a comment

Choose a reason for hiding this comment

lukel97 commented Jan 13, 2025 •

edited

Loading

llvmbot commented Jan 13, 2025 •

edited

Loading

lukel97 Jan 14, 2025 •

edited

Loading