[VPlan] Restrict addOperand to sub-classes that need it (NFC). - #200692
Merged
Conversation
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-vectorizers Author: Florian Hahn (fhahn) ChangesMake addOperand protected and add dedicated helpers in sub-classes that verify the type of the added operand if possible. Most recipes already add all their operands on construction. This patch makes sure that addOperand cannot be used to add operands with invalid types. Patch is 27.95 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/200692.diff 8 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index d07b9896514d7..24672f4a199d9 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -1421,6 +1421,11 @@ class LLVM_ABI_FOR_TEST VPInstruction : public VPRecipeWithIRFlags,
unsigned getOpcode() const { return Opcode; }
+ /// Add \p Op as operand of this VPInstruction. Only supported for AnyOf,
+ /// ComputeReductionResult, BuildVector, BuildStructVector, ExtractLane,
+ /// ExtractLastActive, FirstActiveLane, LastActiveLane.
+ void addOperand(VPValue *Op);
+
/// Generate the instruction.
/// TODO: We currently execute only per-part unless a specific instance is
/// provided.
@@ -1479,7 +1484,9 @@ class LLVM_ABI_FOR_TEST VPInstruction : public VPRecipeWithIRFlags,
assert(!isMasked() && "recipe is already masked");
if (alwaysUnmasked())
return;
- addOperand(Mask);
+ assert(Mask->getScalarType()->isIntegerTy(1) &&
+ "Mask must be an i1 (vector)");
+ VPUser::addOperand(Mask);
}
/// Returns the mask for the VPInstruction. Returns nullptr for unmasked
@@ -1653,6 +1660,15 @@ class VPPhiAccessors {
/// predecessor.
void removeIncomingValueFor(VPBlockBase *IncomingBlock) const;
+ /// Append \p IncomingV as an incoming value to the phi-like recipe.
+ void addIncoming(VPValue *IncomingV) {
+ auto *R = const_cast<VPRecipeBase *>(getAsRecipe());
+ assert((R->getNumOperands() == 0 ||
+ IncomingV->getScalarType() == R->getOperand(0)->getScalarType()) &&
+ "all incoming values must have the same type");
+ R->addOperand(IncomingV);
+ }
+
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
/// Print the recipe.
void printPhiOperands(raw_ostream &O, VPSlotTracker &SlotTracker) const;
@@ -2283,6 +2299,14 @@ class VPVectorEndPointerRecipe : public VPRecipeWithIRFlags {
/// Offset = Stride * (VF - 1) + Part * Stride * VF.
void materializeOffset(unsigned Part = 0);
+ /// Append \p Offset as the offset operand. The offset is an integer index
+ /// expressed in units of SourceElementTy.
+ void addOffset(VPValue *Offset) {
+ assert(Offset->getScalarType()->isIntegerTy() &&
+ "offset must be an integer index");
+ VPUser::addOperand(Offset);
+ }
+
void execute(VPTransformState &State) override;
bool usesFirstLaneOnly(const VPValue *Op) const override {
@@ -2311,7 +2335,7 @@ class VPVectorEndPointerRecipe : public VPRecipeWithIRFlags {
getPointer(), getVFValue(), getSourceElementType(), getStride(),
getGEPNoWrapFlags(), getDebugLoc());
if (auto *Offset = getOffset())
- VEPR->addOperand(Offset);
+ VEPR->addOffset(Offset);
return VEPR;
}
@@ -2346,6 +2370,13 @@ class VPVectorPointerRecipe : public VPRecipeWithIRFlags {
return getNumOperands() > 2 ? getOperand(2) : nullptr;
}
+ /// Add the per-part offset (VFxPart) used for unrolled parts > 0.
+ void addPerPartOffset(VPValue *VFxPart) {
+ assert(VFxPart->getScalarType()->isIntegerTy() &&
+ "per-part offset must be an integer index");
+ VPUser::addOperand(VFxPart);
+ }
+
void execute(VPTransformState &State) override;
Type *getSourceElementType() const { return SourceElementTy; }
@@ -2369,7 +2400,7 @@ class VPVectorPointerRecipe : public VPRecipeWithIRFlags {
new VPVectorPointerRecipe(getOperand(0), SourceElementTy, getStride(),
getGEPNoWrapFlags(), getDebugLoc());
if (auto *VFxPart = getVFxPart())
- Clone->addOperand(VFxPart);
+ Clone->addPerPartOffset(VFxPart);
return Clone;
}
@@ -2462,6 +2493,15 @@ class LLVM_ABI_FOR_TEST VPHeaderPHIRecipe : public VPSingleDefRecipe,
/// Update the incoming value from the loop backedge.
void setBackedgeValue(VPValue *V) { setOperand(1, V); }
+ /// Add \p V as the incoming value from the loop backedge.
+ void addBackedgeValue(VPValue *V) {
+ assert(getNumOperands() == 1 &&
+ "backedge value must be appended right after construction");
+ assert(V->getScalarType() == getScalarType() &&
+ "backedge value must have the same type as the start value");
+ VPUser::addOperand(V);
+ }
+
/// Returns the backedge value as a recipe. The backedge value is guaranteed
/// to be a recipe.
virtual VPRecipeBase &getBackedgeRecipe() {
@@ -2496,6 +2536,20 @@ class VPWidenInductionRecipe : public VPHeaderPHIRecipe {
addOperand(Step);
}
+ /// After unrolling, append the splat-VF step (`VF * step`) and the value of
+ /// the induction at the last unrolled part.
+ void addUnrolledPartOperands(VPValue *SplatVFStep, VPValue *LastPart) {
+ assert(LastPart->getScalarType() == getScalarType() &&
+ "last-part value must match the induction recipe's scalar type");
+ assert((getScalarType()->isPointerTy()
+ ? SplatVFStep->getScalarType()->isIntegerTy()
+ : SplatVFStep->getScalarType() == getScalarType()) &&
+ "splat-step must match the induction type for non-pointer "
+ "inductions, or be an integer index for pointer inductions");
+ VPUser::addOperand(SplatVFStep);
+ VPUser::addOperand(LastPart);
+ }
+
static inline bool classof(const VPRecipeBase *R) {
return R->getVPRecipeID() == VPRecipeBase::VPWidenIntOrFpInductionSC ||
R->getVPRecipeID() == VPRecipeBase::VPWidenPointerInductionSC;
@@ -2947,6 +3001,7 @@ class LLVM_ABI_FOR_TEST VPBlendRecipe : public VPRecipeWithIRFlags {
/// Set mask number \p Idx to \p V.
void setMask(unsigned Idx, VPValue *V) {
assert((Idx > 0 || !isNormalized()) && "First index has no mask!");
+ assert(V->getScalarType()->isIntegerTy(1) && "Mask must be an i1 (vector)");
Idx == 0 ? setOperand(1, V) : setOperand(Idx * 2 + !isNormalized(), V);
}
@@ -3656,6 +3711,8 @@ class LLVM_ABI_FOR_TEST VPWidenMemoryRecipe : public VPIRMetadata {
assert(!IsMasked && "cannot re-set mask");
if (!Mask)
return;
+ assert(Mask->getScalarType()->isIntegerTy(1) &&
+ "Mask must be an i1 (vector)");
getAsRecipe()->addOperand(Mask);
IsMasked = true;
}
@@ -3952,7 +4009,7 @@ class VPActiveLaneMaskPHIRecipe : public VPHeaderPHIRecipe {
VPActiveLaneMaskPHIRecipe *clone() override {
auto *R = new VPActiveLaneMaskPHIRecipe(getOperand(0), getDebugLoc());
if (getNumOperands() == 2)
- R->addOperand(getOperand(1));
+ R->addBackedgeValue(getOperand(1));
return R;
}
@@ -4030,7 +4087,7 @@ class VPWidenCanonicalIVRecipe : public VPRecipeWithIRFlags {
auto *WideCanIV =
new VPWidenCanonicalIVRecipe(getCanonicalIV(), getNoWrapFlags());
if (VPValue *Step = getStepValue())
- WideCanIV->addOperand(Step);
+ WideCanIV->addPerPartStep(Step);
return WideCanIV;
}
@@ -4056,6 +4113,13 @@ class VPWidenCanonicalIVRecipe : public VPRecipeWithIRFlags {
return getNumOperands() == 2 ? getOperand(1) : nullptr;
}
+ /// Add the per-part step (VF * Part) used for unrolled parts.
+ void addPerPartStep(VPValue *Step) {
+ assert(Step->getScalarType() == getScalarType() &&
+ "per-part step must have the same type as the canonical IV");
+ VPUser::addOperand(Step);
+ }
+
protected:
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
/// Print the recipe.
diff --git a/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp b/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
index 80cdf9db2814c..a7d13075a3ea1 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
@@ -114,7 +114,7 @@ void PlainCFGBuilder::fixHeaderPhis() {
assert(Phi->getNumOperands() == 2 &&
"header phi must have exactly 2 operands");
for (BasicBlock *Pred : predecessors(Phi->getParent()))
- PhiR->addOperand(
+ PhiR->addIncoming(
getOrCreateVPOperand(Phi->getIncomingValueForBlock(Pred)));
}
}
@@ -241,7 +241,7 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB,
getOrCreateVPOperand(Phi->getIncomingValue(I));
}
for (VPBlockBase *Pred : VPBB->getPredecessors())
- NewR->addOperand(
+ cast<VPPhi>(NewR)->addIncoming(
VPPredToIncomingValue.lookup(Pred->getExitingBasicBlock()));
}
} else {
@@ -368,7 +368,7 @@ std::unique_ptr<VPlan> PlainCFGBuilder::buildPlainCFG() {
assert(PhiR->getNumOperands() == 0 &&
"no phi operands should be added yet");
for (BasicBlock *Pred : predecessors(EB->getIRBasicBlock()))
- PhiR->addOperand(
+ PhiR->addIncoming(
getOrCreateVPOperand(Phi.getIncomingValueForBlock(Pred)));
}
}
@@ -588,7 +588,7 @@ static void addInitialSkeleton(VPlan &Plan, Type *InductionTy,
auto *ResumePhiR = ScalarPHBuilder.createScalarPhi(
{ResumeFromVectorLoop, VectorPhiR->getOperand(0)},
VectorPhiR->getDebugLoc());
- cast<VPIRPhi>(&ScalarPhiR)->addOperand(ResumePhiR);
+ cast<VPIRPhi>(&ScalarPhiR)->addIncoming(ResumePhiR);
}
}
@@ -1389,7 +1389,7 @@ static void insertCheckBlockBeforeVectorLoop(VPlan &Plan,
auto *Phi = cast<VPPhi>(&R);
assert(Phi->getNumIncoming() == NumPreds - 1 &&
"must have incoming values for all predecessors");
- Phi->addOperand(Phi->getOperand(NumPreds - 2));
+ Phi->addIncoming(Phi->getOperand(NumPreds - 2));
}
}
@@ -1822,7 +1822,7 @@ bool VPlanTransforms::handleFindLastReductions(VPlan &Plan) {
VPValue *AnyOf = Builder.createNaryOp(VPInstruction::AnyOf, {Cond});
VPValue *MaskSelect = Builder.createSelect(AnyOf, Cond, MaskPHI);
- MaskPHI->addOperand(MaskSelect);
+ MaskPHI->addIncoming(MaskSelect);
// Replace select for data.
VPValue *DataSelect =
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index 59084bbaf41b3..3d551cbce9609 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -1455,6 +1455,47 @@ bool VPInstruction::isSingleScalar() const {
}
}
+void VPInstruction::addOperand(VPValue *Op) {
+#ifndef NDEBUG
+ Type *Ty = Op->getScalarType();
+ switch (getOpcode()) {
+ case VPInstruction::AnyOf:
+ case VPInstruction::FirstActiveLane:
+ case VPInstruction::LastActiveLane:
+ assert(Ty == getOperand(0)->getScalarType() && Ty->isIntegerTy(1) &&
+ "appended operand must be an i1 (vector) matching operand 0");
+ break;
+ case VPInstruction::ComputeReductionResult:
+ case VPInstruction::BuildVector:
+ case VPInstruction::BuildStructVector:
+ assert(Ty == getOperand(0)->getScalarType() &&
+ "appended operand must match operand 0's scalar type");
+ break;
+ case VPInstruction::ExtractLane:
+ assert(Ty == getOperand(1)->getScalarType() &&
+ "appended operand must match operand 1's scalar type");
+ break;
+ case VPInstruction::ExtractLastActive: {
+ // The recipe is constructed with 3 operands (result, data, mask). Extra
+ // operands beyond that are appended in (data, mask) pairs.
+ constexpr unsigned NumInitialOperands = 3;
+ assert(getNumOperands() >= NumInitialOperands &&
+ "ExtractLastActive must have at least the initial 3 operands");
+ bool IsMaskSlot = ((getNumOperands() - NumInitialOperands) & 1u) == 1u;
+ assert((IsMaskSlot ? Ty->isIntegerTy(1)
+ : Ty == getOperand(1)->getScalarType()) &&
+ "ExtractLastActive expects alternating data/mask operands "
+ "matching operand 1's type and i1, respectively");
+ break;
+ }
+ default:
+ llvm_unreachable("opcode does not support growing the operand list "
+ "outside of construction");
+ }
+#endif
+ VPUser::addOperand(Op);
+}
+
void VPInstruction::execute(VPTransformState &State) {
assert(!isMasked() && "cannot execute masked VPInstruction");
IRBuilderBase::FastMathFlagGuard FMFGuard(State.Builder);
@@ -2965,7 +3006,7 @@ void VPVectorEndPointerRecipe::materializeOffset(unsigned Part) {
VPValue *Offset = Builder.createAdd(
Offset0,
Builder.createOverflowingOp(Instruction::Mul, {PartxStride, VF}));
- addOperand(Offset);
+ addOffset(Offset);
}
void VPVectorEndPointerRecipe::execute(VPTransformState &State) {
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index 0b592535fb3c9..57ac134a8aece 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -2876,7 +2876,7 @@ addVPLaneMaskPhiAndUpdateExitBranch(VPlan &Plan) {
auto *ALM = Builder.createNaryOp(VPInstruction::ActiveLaneMask,
{InLoopIncrement, TC, ALMMultiplier}, DL,
"active.lane.mask.next");
- LaneMaskPhi->addOperand(ALM);
+ LaneMaskPhi->addBackedgeValue(ALM);
// Replace the original terminator with BranchOnCond. We have to invert the
// mask here because a true condition means jumping to the exit block.
@@ -3348,12 +3348,12 @@ void VPlanTransforms::addExplicitVectorLength(
auto *NextIter = Builder.createAdd(
OpVPEVL, CurrentIteration, CanonicalIVIncrement->getDebugLoc(),
"current.iteration.next", CanonicalIVIncrement->getNoWrapFlags());
- CurrentIteration->addOperand(NextIter);
+ CurrentIteration->addBackedgeValue(NextIter);
VPValue *NextAVL =
Builder.createSub(AVLPhi, OpVPEVL, DebugLoc::getCompilerGenerated(),
"avl.next", {/*NUW=*/true, /*NSW=*/false});
- AVLPhi->addOperand(NextAVL);
+ AVLPhi->addIncoming(NextAVL);
fixupVFUsersForEVL(Plan, *VPEVL);
removeDeadRecipes(Plan);
@@ -3836,7 +3836,7 @@ expandVPWidenIntOrFpInduction(VPWidenIntOrFpInductionRecipe *WidenIVR,
auto *Next = Builder.createNaryOp(AddOp, {Prev, Inc}, Flags,
WidenIVR->getDebugLoc(), "vec.ind.next");
- WidePHI->addOperand(Next);
+ WidePHI->addIncoming(Next);
WidenIVR->replaceAllUsesWith(WidePHI);
}
@@ -3901,7 +3901,7 @@ static void expandVPWidenPointerInduction(VPWidenPointerInductionRecipe *R,
VPValue *InductionGEP =
Builder.createPtrAdd(ScalarPtrPhi, Inc, DL, "ptr.ind");
- ScalarPtrPhi->addOperand(InductionGEP);
+ ScalarPtrPhi->addIncoming(InductionGEP);
}
/// Expand a VPDerivedIVRecipe into executable recipes.
@@ -4350,7 +4350,7 @@ void VPlanTransforms::handleUncountableEarlyExits(VPlan &Plan,
DebugLoc::getUnknown(), "early.exit.value");
}
ExitIRI->removeIncomingValueFor(EarlyExitingVPBB);
- ExitIRI->addOperand(NewIncoming);
+ ExitIRI->addIncoming(NewIncoming);
}
EarlyExitingVPBB->getTerminator()->eraseFromParent();
diff --git a/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp b/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp
index 65ad0feaa71c9..d180b70bfcdc6 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp
@@ -242,8 +242,7 @@ void UnrollState::unrollWidenInductionByUF(
addRecipeForPart(IV, Add, Part);
Prev = Add;
}
- IV->addOperand(VectorStep);
- IV->addOperand(Prev);
+ IV->addUnrolledPartOperands(VectorStep, Prev);
}
void UnrollState::unrollHeaderPHIByUF(VPHeaderPHIRecipe *R,
@@ -355,7 +354,10 @@ void UnrollState::unrollRecipeByUF(VPRecipeBase &R) {
VPValue *VFxPart = Builder.createOverflowingOp(
Instruction::Mul, {VF, Plan.getConstantInt(IndexTy, Part)},
{true, true});
- Copy->addOperand(VFxPart);
+ if (auto *VecPtr = dyn_cast<VPVectorPointerRecipe>(Copy))
+ VecPtr->addPerPartOffset(VFxPart);
+ else
+ cast<VPWidenCanonicalIVRecipe>(Copy)->addPerPartStep(VFxPart);
continue;
}
if (auto *Red = dyn_cast<VPReductionRecipe>(&R)) {
@@ -397,7 +399,7 @@ void UnrollState::unrollRecipeByUF(VPRecipeBase &R) {
}
if (auto *WideCanIV = dyn_cast<VPWidenCanonicalIVRecipe>(&R)) {
// Set Part0 step for WidenCanonicalIV.
- WideCanIV->addOperand(getConstantInt(0));
+ WideCanIV->addPerPartStep(getConstantInt(0));
}
}
@@ -431,26 +433,29 @@ void UnrollState::unrollBlock(VPBlockBase *VPB) {
match(&R, m_FirstActiveLane(m_VPValue(Op1))) ||
match(&R, m_LastActiveLane(m_VPValue(Op1))) ||
match(&R, m_ComputeReductionResult(m_VPValue(Op1)))) {
- addUniformForAllParts(cast<VPInstruction>(&R));
+ auto *VPI = cast<VPInstruction>(&R);
+ addUniformForAllParts(VPI);
for (unsigned Part = 1; Part != UF; ++Part)
- R.addOperand(getValueForPart(Op1, Part));
+ VPI->addOperand(getValueForPart(Op1, Part));
continue;
}
VPValue *Op0;
if (match(&R, m_ExtractLane(m_VPValue(Op0), m_VPValue(Op1)))) {
- addUniformForAllParts(cast<VPInstruction>(&R));
+ auto *VPI = cast<VPInstruction>(&R);
+ addUniformForAllParts(VPI);
for (unsigned Part = 1; Part != UF; ++Part)
- R.addOperand(getValueForPart(Op1, Part));
+ VPI->addOperand(getValueForPart(Op1, Part));
continue;
}
VPValue *Op2;
if (match(&R, m_ExtractLastActive(m_VPValue(), m_VPValue(Op1),
m_VPValue(Op2)))) {
- addUniformForAllParts(cast<VPInstruction>(&R));
+ auto *VPI = cast<VPInstruction>(&R);
+ addUniformForAllParts(VPI);
for (unsigned Part = 1; Part != UF; ++Part) {
- R.addOperand(getValueForPart(Op1, Part));
- R.addOperand(getValueForPart(Op2, Part));
+ VPI->addOperand(getValueForPart(Op1, Part));
+ VPI->addOperand(getValueForPart(Op2, Part));
}
continue;
}
diff --git a/llvm/lib/Transforms/Vectorize/VPlanValue.h b/llvm/lib/Transforms/Vectorize/VPlanValue.h
index 427bc2ea199dc..61c68d4577269 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanValue.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanValue.h
@@ -384,6 +384,8 @@ class VPMultiDefValue : public VPRecipeValue {
class VPUser {
/// Grant access to removeOperand for VPPhiAccessors, the only supported user.
friend class VPPhiAccessors;
+ /// Grant access to addOperand for VPWidenMemoryRecipe.
+ friend class VPWidenMemoryRecipe;
SmallVector<VPValue *, 2> Operands;
@@ -405,6 +407,11 @@ class VPUser {
addOperand(Operand);
}
+ void addOperand(VPValue *Operand) {
+ Operands.push_back(Operand);
+ Operand->addUser(*this);
+ }
+
public:
VPUser() = delete;
VPUser(const VPUser &) = delete;
@@ -414,11 +421,6 @@ class VPUser {
Op->removeUser(*this);
}
- void addOperand(VPValue *Operand) {
- Operands.push_back(Operand);
- Operand->addUser(*this);
- }
-
unsigned getNumOperands() const { return Operands.size(); }
inline VPValue *getOperand(unsigned N) const {
assert(N < Operands.size() && "Operand index out of bounds");
diff --git a/llvm/unittests/Transforms/Vectorize/VPlanTest.cpp b/llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
index ea1ab78ea5fc8..cf3a508d46a8b 100644
--- a/llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
+++ b/llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
@@ -1272,7 +1272,7 @@ TEST_F(VPRecipeTest, CastVPWidenMemoryRecipeToVPUserAndVPRecipeBase) {
auto *Load =
new LoadInst(Int32, PoisonValue::get(Int32Ptr), "", false, Align(1));
VPValue *Addr = Plan.getOrAddLiveIn(ConstantInt::get(Int32, 1));
- VPValue *Mask = Plan.getOrAddLiveIn(ConstantInt::get(Int32, 2));
+ VPValue *Mask = Plan.getOrAddLiveIn(ConstantInt::getTrue(C));
VPWidenLoadRecipe Recipe(*Load, Addr, Mask, true, {}, {});
checkVPRecipeCastImpl<VPWidenLoadRecipe, VPUser, VPIRMetadata>(&Recipe);
@@ -1303,7 +1303,7 @@ TEST_F(VPRecipeTest, CastVPWidenLoadEVLRecipeToVPUser) {
auto *Load =
new LoadInst(Int32, PoisonValue::get(Int32Ptr), "", false, Align(1));
VPValue *Addr = Plan.getOrAddLiveIn(ConstantInt::get(Int32, 1));
- VPValue *Mask = Plan.getOrAddLiveIn(ConstantInt::get(Int32, 2));
+ VPValue *Mask = Plan.getOrAddLiveIn(ConstantInt::getTrue(C));
VPValue *EVL = Plan.getOrAddLiveIn(ConstantInt::get(Int32, 8));
VPWidenLoadRecipe BaseLoad(*Load, Addr, Mask, true, {}, {});
VPWidenLoadEVLRecipe Recipe(BaseLoad, Addr, *EVL, Mask);
@@ -1321,7 +1321,7 @@ TEST_F(VPRecipeTest, CastVPWidenStoreRecipeToVPUser) {
...
[truncated]
|
fhahn
force-pushed
the
vplan-restrict-add-operand
branch
from
June 1, 2026 20:08
ff52ffa to
ab57f50
Compare
Make addOperand protected and add dedicated helpers in sub-classes that verify the type of the added operand if possible. Most recipes already add all their operands on construction. This patch makes sure that addOperand cannot be used to add operands with invalid types.
fhahn
force-pushed
the
vplan-restrict-add-operand
branch
from
June 2, 2026 21:31
ab57f50 to
68a3f37
Compare
lukel97
approved these changes
Jun 3, 2026
lukel97
left a comment
lukel97
left a comment
Contributor
There was a problem hiding this comment.
LGTM. It looks like we're moving closer to the design of LLVM IR where only PHIs can add operands IIRC. It would be nice to look at removing more uses of addOperand later, creating and RAUWing new values as needed
| IntegerType::get(C, 32)); | ||
| VPBasicBlock &VPBB = *Plan.createVPBasicBlock(""); | ||
| VPBB.appendRecipe(I); | ||
| EXPECT_DEATH(I->addOperand(VF), "accessing materialized symbolic value"); |
Contributor
There was a problem hiding this comment.
I guess we never validated addOperands against VPInstruction::getNumOperandsForOpcode beforehand?
Contributor
Author
There was a problem hiding this comment.
Yep, previously we could add any operand after the fact
Comment on lines
+1453
to
+1454
| assert(Ty == getOperand(0)->getScalarType() && Ty->isIntegerTy(1) && | ||
| "appended operand must be an i1 (vector) matching operand 0"); |
Contributor
There was a problem hiding this comment.
Can we assert that getOperand(0)->getScalarType()->isIntegerTy(1) earlier in the VPInstruction constructor?
Contributor
Author
There was a problem hiding this comment.
Yep, I'll do that separately, and removed the bool check here
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
llvm-upstreamsync Bot
pushed a commit
to qualcomm/cpullvm-toolchain
that referenced
this pull request
Jun 5, 2026
…FC). (#200692) Make addOperand protected and add dedicated helpers in sub-classes that verify the type of the added operand if possible. Most recipes already add all their operands on construction. This patch makes sure that addOperand cannot be used to add operands with invalid types. PR: llvm/llvm-project#200692
llvm-sync Bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jun 5, 2026
…FC). (#200692) Make addOperand protected and add dedicated helpers in sub-classes that verify the type of the added operand if possible. Most recipes already add all their operands on construction. This patch makes sure that addOperand cannot be used to add operands with invalid types. PR: llvm/llvm-project#200692
carlobertolli
pushed a commit
to carlobertolli/llvm-project
that referenced
this pull request
Jun 11, 2026
…200692) Make addOperand protected and add dedicated helpers in sub-classes that verify the type of the added operand if possible. Most recipes already add all their operands on construction. This patch makes sure that addOperand cannot be used to add operands with invalid types. PR: llvm#200692
llvm-upstreamsync Bot
pushed a commit
to qualcomm/cpullvm-toolchain
that referenced
this pull request
Jun 11, 2026
…(#201209) With llvm/llvm-project#200692 and llvm/llvm-project#200686, types are no checked at construction, and each operation that changes operands (setOperand, addOperand etc) verifies that the replacement happens with suitable types. This should remove the need for running type checking as part of the verifier. PR: llvm/llvm-project#201209
llvm-sync Bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jun 11, 2026
…(#201209) With llvm/llvm-project#200692 and llvm/llvm-project#200686, types are no checked at construction, and each operation that changes operands (setOperand, addOperand etc) verifies that the replacement happens with suitable types. This should remove the need for running type checking as part of the verifier. PR: llvm/llvm-project#201209
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make addOperand protected and add dedicated helpers in sub-classes that verify the type of the added operand if possible.
Most recipes already add all their operands on construction. This patch makes sure that addOperand cannot be used to add operands with invalid types.