Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Jan 16, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fe36dfe
[VPlan] Verify scalar types in VPlanVerifier. NFCI
lukel97 Jan 13, 2025
f7b716d
Fix verifier unit tests by filling out stuff necessary for corrrect t…
lukel97 Jan 13, 2025
561b1a9
Change back LogicalAnd and add assert for its operand
lukel97 Jan 14, 2025
f18d8db
Update comment on AnyOf
lukel97 Jan 14, 2025
6ad47a9
Report an error if inferScalarType returns nullptr
lukel97 Jan 14, 2025
71ec919
Update comment
lukel97 Jan 15, 2025
d7fec40
Check other LogicalAnd operand
lukel97 Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions llvm/lib/Transforms/Vectorize/VPlan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down
11 changes: 10 additions & 1 deletion llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment on lines +63 to +66
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this caught by the verifier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

case VPInstruction::ComputeReductionResult: {
auto *PhiR = cast<VPReductionPHIRecipe>(R->getOperand(0));
auto *OrigPhi = cast<PHINode>(PhiR->getUnderlyingValue());
Expand All @@ -71,6 +74,9 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
case VPInstruction::FirstOrderRecurrenceSplice:
case VPInstruction::Not:
case VPInstruction::ResumePhi:
case VPInstruction::CalculateTripCountMinusVF:
case VPInstruction::CanonicalIVIncrementForPart:
case VPInstruction::AnyOf:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return SetResultTyFromOp();
case VPInstruction::ExtractFromEnd: {
Type *BaseTy = inferScalarType(R->getOperand(0));
Expand All @@ -79,6 +85,9 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
return BaseTy;
}
case VPInstruction::LogicalAnd:
assert(inferScalarType(R->getOperand(0))->isIntegerTy(1) &&
inferScalarType(R->getOperand(1))->isIntegerTy(1) &&
"LogicalAnd operands should be bool");
return IntegerType::get(Ctx, 1);
case VPInstruction::PtrAdd:
// Return the type based on the pointer argument (i.e. first operand).
Expand Down
16 changes: 14 additions & 2 deletions llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ using namespace llvm;
namespace {
class VPlanVerifier {
const VPDominatorTree &VPDT;
VPTypeAnalysis &TypeInfo;

SmallPtrSet<BasicBlock *, 8> WrappedIRBBs;

Expand Down Expand Up @@ -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);
};
Expand Down Expand Up @@ -195,6 +197,14 @@ bool VPlanVerifier::verifyVPBasicBlock(const VPBasicBlock *VPBB) {
return false;
}
for (const VPValue *V : R.definedValues()) {
// Verify that we can infer a scalar type for each defined value. With
// assertions enabled, inferScalarType will perform some consistency
// checks during type inference.
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);
// TODO: check dominance of incoming values for phis properly.
Expand Down Expand Up @@ -406,6 +416,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);
}
40 changes: 25 additions & 15 deletions llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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});

Expand All @@ -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);
Expand All @@ -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("");

Expand All @@ -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("");
Expand Down Expand Up @@ -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});

Expand Down
Loading