[llvm] [VPlan] Verify scalar types in VPlanVerifier. NFCI (PR #122679)
Luke Lau via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 14 06:38:27 PST 2025
https://github.com/lukel97 updated https://github.com/llvm/llvm-project/pull/122679
>From fe36dfe7a4901e23500931b82065117b5f6dc2cf Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Mon, 13 Jan 2025 17:53:19 +0800
Subject: [PATCH 1/5] [VPlan] Verify scalar types in VPlanVerifier. NFCI
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)
---
llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp | 11 ++++++++---
llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp | 11 +++++++++--
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp b/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp
index 8fea2c6fd33b6f..918276b96d47a1 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);
}
>From f7b716d6ef471f4b37aa58705e28f75c5473c38f Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Mon, 13 Jan 2025 23:46:17 +0800
Subject: [PATCH 2/5] Fix verifier unit tests by filling out stuff necessary
for corrrect types
---
.../Vectorize/VPlanVerifierTest.cpp | 40 ++++++++++++-------
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp b/llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp
index f098ba0bce497b..f818b49fdbe7fe 100644
--- a/llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp
+++ b/llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp
@@ -20,14 +20,17 @@ using VPVerifierTest = VPlanTestBase;
namespace {
TEST_F(VPVerifierTest, VPInstructionUseBeforeDefSameBB) {
VPlan &Plan = getPlan();
- VPInstruction *DefI = new VPInstruction(Instruction::Add, {});
+ VPValue *Zero = Plan.getOrAddLiveIn(ConstantInt::get(Type::getInt32Ty(C), 0));
+ VPInstruction *DefI = new VPInstruction(Instruction::Add, {Zero});
VPInstruction *UseI = new VPInstruction(Instruction::Sub, {DefI});
+ auto *CanIV = new VPCanonicalIVPHIRecipe(Zero, {});
VPBasicBlock *VPBB1 = Plan.getEntry();
VPBB1->appendRecipe(UseI);
VPBB1->appendRecipe(DefI);
VPBasicBlock *VPBB2 = Plan.createVPBasicBlock("");
+ VPBB2->appendRecipe(CanIV);
VPRegionBlock *R1 = Plan.createVPRegionBlock(VPBB2, VPBB2, "R1");
VPBlockUtils::connectBlocks(VPBB1, R1);
VPBlockUtils::connectBlocks(R1, Plan.getScalarHeader());
@@ -44,9 +47,10 @@ TEST_F(VPVerifierTest, VPInstructionUseBeforeDefSameBB) {
TEST_F(VPVerifierTest, VPInstructionUseBeforeDefDifferentBB) {
VPlan &Plan = getPlan();
- VPInstruction *DefI = new VPInstruction(Instruction::Add, {});
+ VPValue *Zero = Plan.getOrAddLiveIn(ConstantInt::get(Type::getInt32Ty(C), 0));
+ VPInstruction *DefI = new VPInstruction(Instruction::Add, {Zero});
VPInstruction *UseI = new VPInstruction(Instruction::Sub, {DefI});
- auto *CanIV = new VPCanonicalIVPHIRecipe(UseI, {});
+ auto *CanIV = new VPCanonicalIVPHIRecipe(Zero, {});
VPInstruction *BranchOnCond =
new VPInstruction(VPInstruction::BranchOnCond, {CanIV});
@@ -73,23 +77,22 @@ TEST_F(VPVerifierTest, VPInstructionUseBeforeDefDifferentBB) {
}
TEST_F(VPVerifierTest, VPBlendUseBeforeDefDifferentBB) {
+ VPlan &Plan = getPlan();
IntegerType *Int32 = IntegerType::get(C, 32);
auto *Phi = PHINode::Create(Int32, 1);
+ VPValue *Zero = Plan.getOrAddLiveIn(ConstantInt::get(Int32, 0));
- VPInstruction *I1 = new VPInstruction(Instruction::Add, {});
- VPInstruction *DefI = new VPInstruction(Instruction::Add, {});
- auto *CanIV = new VPCanonicalIVPHIRecipe(I1, {});
+ VPInstruction *DefI = new VPInstruction(Instruction::Add, {Zero});
+ auto *CanIV = new VPCanonicalIVPHIRecipe(Zero, {});
VPInstruction *BranchOnCond =
new VPInstruction(VPInstruction::BranchOnCond, {CanIV});
auto *Blend = new VPBlendRecipe(Phi, {DefI});
- VPlan &Plan = getPlan();
VPBasicBlock *VPBB1 = Plan.getEntry();
VPBasicBlock *VPBB2 = Plan.createVPBasicBlock("");
VPBasicBlock *VPBB3 = Plan.createVPBasicBlock("");
VPBasicBlock *VPBB4 = Plan.createVPBasicBlock("");
- VPBB1->appendRecipe(I1);
VPBB2->appendRecipe(CanIV);
VPBB3->appendRecipe(Blend);
VPBB4->appendRecipe(DefI);
@@ -116,14 +119,15 @@ TEST_F(VPVerifierTest, VPBlendUseBeforeDefDifferentBB) {
}
TEST_F(VPVerifierTest, DuplicateSuccessorsOutsideRegion) {
- VPInstruction *I1 = new VPInstruction(Instruction::Add, {});
- auto *CanIV = new VPCanonicalIVPHIRecipe(I1, {});
+ VPlan &Plan = getPlan();
+ VPValue *Zero = Plan.getOrAddLiveIn(ConstantInt::get(Type::getInt32Ty(C), 0));
+ VPInstruction *I1 = new VPInstruction(Instruction::Add, {Zero});
+ auto *CanIV = new VPCanonicalIVPHIRecipe(Zero, {});
VPInstruction *BranchOnCond =
new VPInstruction(VPInstruction::BranchOnCond, {CanIV});
VPInstruction *BranchOnCond2 =
new VPInstruction(VPInstruction::BranchOnCond, {I1});
- VPlan &Plan = getPlan();
VPBasicBlock *VPBB1 = Plan.getEntry();
VPBasicBlock *VPBB2 = Plan.createVPBasicBlock("");
@@ -149,14 +153,15 @@ TEST_F(VPVerifierTest, DuplicateSuccessorsOutsideRegion) {
}
TEST_F(VPVerifierTest, DuplicateSuccessorsInsideRegion) {
- VPInstruction *I1 = new VPInstruction(Instruction::Add, {});
- auto *CanIV = new VPCanonicalIVPHIRecipe(I1, {});
+ VPlan &Plan = getPlan();
+ VPValue *Zero = Plan.getOrAddLiveIn(ConstantInt::get(Type::getInt32Ty(C), 0));
+ VPInstruction *I1 = new VPInstruction(Instruction::Add, {Zero});
+ auto *CanIV = new VPCanonicalIVPHIRecipe(Zero, {});
VPInstruction *BranchOnCond =
new VPInstruction(VPInstruction::BranchOnCond, {CanIV});
VPInstruction *BranchOnCond2 =
new VPInstruction(VPInstruction::BranchOnCond, {I1});
- VPlan &Plan = getPlan();
VPBasicBlock *VPBB1 = Plan.getEntry();
VPBasicBlock *VPBB2 = Plan.createVPBasicBlock("");
VPBasicBlock *VPBB3 = Plan.createVPBasicBlock("");
@@ -186,10 +191,15 @@ TEST_F(VPVerifierTest, DuplicateSuccessorsInsideRegion) {
TEST_F(VPVerifierTest, BlockOutsideRegionWithParent) {
VPlan &Plan = getPlan();
+
VPBasicBlock *VPBB1 = Plan.getEntry();
VPBasicBlock *VPBB2 = Plan.createVPBasicBlock("");
- VPInstruction *DefI = new VPInstruction(Instruction::Add, {});
+ VPValue *Zero = Plan.getOrAddLiveIn(ConstantInt::get(Type::getInt32Ty(C), 0));
+ auto *CanIV = new VPCanonicalIVPHIRecipe(Zero, {});
+ VPBB2->appendRecipe(CanIV);
+
+ VPInstruction *DefI = new VPInstruction(Instruction::Add, {Zero});
VPInstruction *BranchOnCond =
new VPInstruction(VPInstruction::BranchOnCond, {DefI});
>From 561b1a951c96131d0d3a00cb1cc92f07c229d93c Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Tue, 14 Jan 2025 12:34:41 +0800
Subject: [PATCH 3/5] Change back LogicalAnd and add assert for its operand
---
llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp b/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp
index 918276b96d47a1..49331e9c25e954 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp
@@ -77,7 +77,6 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
case VPInstruction::CalculateTripCountMinusVF:
case VPInstruction::CanonicalIVIncrementForPart:
case VPInstruction::AnyOf:
- case VPInstruction::LogicalAnd:
return SetResultTyFromOp();
case VPInstruction::ExtractFromEnd: {
Type *BaseTy = inferScalarType(R->getOperand(0));
@@ -85,6 +84,10 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
return VecTy->getElementType();
return BaseTy;
}
+ case VPInstruction::LogicalAnd:
+ assert(inferScalarType(R->getOperand(0))->isIntegerTy(1) &&
+ "LogicalAnd operand should be bool");
+ 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));
>From f18d8dbb57c9cb8e439a37719b42f46f7ea5eee4 Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Tue, 14 Jan 2025 12:40:12 +0800
Subject: [PATCH 4/5] Update comment on AnyOf
---
llvm/lib/Transforms/Vectorize/VPlan.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 1da185f9cfdf47..e696430279d22c 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -1223,8 +1223,8 @@ class VPInstruction : public VPRecipeWithIRFlags,
// operand). Only generates scalar values (either for the first lane only or
// for all lanes, depending on its uses).
PtrAdd,
- // Returns a scalar boolean value, which is true if any lane of its single
- // operand is true.
+ // Returns a scalar boolean value, which is true if any lane of its (only
+ // boolean) vector operand is true.
AnyOf,
};
>From 6ad47a966566d4301f41075db671d01a10df50b4 Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Tue, 14 Jan 2025 22:37:22 +0800
Subject: [PATCH 5/5] Report an error if inferScalarType returns nullptr
Which hopefully it never will!
---
llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
index 647c03ec69ba4f..1333b456ff218c 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
@@ -198,7 +198,10 @@ bool VPlanVerifier::verifyVPBasicBlock(const VPBasicBlock *VPBB) {
}
for (const VPValue *V : R.definedValues()) {
// Verify that recipes' operands have matching types.
- TypeInfo.inferScalarType(V);
+ if (!TypeInfo.inferScalarType(V)) {
+ errs() << "Failed to infer scalar type!\n";
+ return false;
+ }
for (const VPUser *U : V->users()) {
auto *UI = dyn_cast<VPRecipeBase>(U);
More information about the llvm-commits
mailing list