[VPlan] Populate and use VPIRMetadata from VPInstructions (NFC) #167253
+125
−106
Conversation
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
Member
|
@llvm/pr-subscribers-vectorizers @llvm/pr-subscribers-llvm-transforms Author: Florian Hahn (fhahn) ChangesUpdate VPlan to populate VPIRMetadata during VPInstruction construction and use it when creating widened recipes, instead of constructing VPIRMetadata from the underlying IR instruction each time. This centralizes VPIRMetadata in VPInstructions and ensures metadata is consistently available throughout VPlan transformations. Patch is 27.58 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/167253.diff 9 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h b/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
index 04b05627fa769..c65ff446173f7 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
@@ -65,7 +65,8 @@ class VPBuilder {
VPInstruction *createInstruction(unsigned Opcode,
ArrayRef<VPValue *> Operands, DebugLoc DL,
const Twine &Name = "") {
- return tryInsertInstruction(new VPInstruction(Opcode, Operands, DL, Name));
+ return tryInsertInstruction(
+ new VPInstruction(Opcode, Operands, {}, DL, Name));
}
public:
@@ -150,11 +151,11 @@ class VPBuilder {
/// its underlying Instruction.
VPInstruction *createNaryOp(unsigned Opcode, ArrayRef<VPValue *> Operands,
Instruction *Inst = nullptr,
+ const VPIRMetadata &MD = {},
+ DebugLoc DL = DebugLoc::getUnknown(),
const Twine &Name = "") {
- DebugLoc DL = DebugLoc::getUnknown();
- if (Inst)
- DL = Inst->getDebugLoc();
- VPInstruction *NewVPInst = createInstruction(Opcode, Operands, DL, Name);
+ VPInstruction *NewVPInst =
+ tryInsertInstruction(new VPInstruction(Opcode, Operands, MD, DL, Name));
NewVPInst->setUnderlyingValue(Inst);
return NewVPInst;
}
@@ -212,7 +213,7 @@ class VPBuilder {
DebugLoc DL = DebugLoc::getUnknown(),
const Twine &Name = "") {
return tryInsertInstruction(
- new VPInstruction(VPInstruction::LogicalAnd, {LHS, RHS}, DL, Name));
+ new VPInstruction(VPInstruction::LogicalAnd, {LHS, RHS}, {}, DL, Name));
}
VPInstruction *
@@ -223,7 +224,7 @@ class VPBuilder {
FMFs ? new VPInstruction(Instruction::Select, {Cond, TrueVal, FalseVal},
*FMFs, {}, DL, Name)
: new VPInstruction(Instruction::Select, {Cond, TrueVal, FalseVal},
- DL, Name);
+ {}, DL, Name);
return tryInsertInstruction(Select);
}
@@ -329,7 +330,7 @@ class VPBuilder {
else if (Opcode == Instruction::ZExt)
Flags = VPIRFlags::NonNegFlagsTy(false);
return tryInsertInstruction(
- new VPWidenCastRecipe(Opcode, Op, ResultTy, Flags));
+ new VPWidenCastRecipe(Opcode, Op, ResultTy, nullptr, Flags));
}
VPScalarIVStepsRecipe *
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 666033b1fac62..b994d8cd04235 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -7552,14 +7552,13 @@ VPWidenMemoryRecipe *VPRecipeBuilder::tryToWidenMemory(VPInstruction *VPI,
}
if (VPI->getOpcode() == Instruction::Load) {
auto *Load = cast<LoadInst>(I);
- return new VPWidenLoadRecipe(*Load, Ptr, Mask, Consecutive, Reverse,
- VPIRMetadata(*Load, LVer), I->getDebugLoc());
+ return new VPWidenLoadRecipe(*Load, Ptr, Mask, Consecutive, Reverse, *VPI,
+ VPI->getDebugLoc());
}
StoreInst *Store = cast<StoreInst>(I);
return new VPWidenStoreRecipe(*Store, Ptr, VPI->getOperand(0), Mask,
- Consecutive, Reverse,
- VPIRMetadata(*Store, LVer), VPI->getDebugLoc());
+ Consecutive, Reverse, *VPI, VPI->getDebugLoc());
}
/// Creates a VPWidenIntOrFpInductionRecipe for \p PhiR. If needed, it will
@@ -7670,7 +7669,7 @@ VPSingleDefRecipe *VPRecipeBuilder::tryToWidenCall(VPInstruction *VPI,
},
Range);
if (ShouldUseVectorIntrinsic)
- return new VPWidenIntrinsicRecipe(*CI, ID, Ops, CI->getType(),
+ return new VPWidenIntrinsicRecipe(*CI, ID, Ops, CI->getType(), *VPI,
VPI->getDebugLoc());
Function *Variant = nullptr;
@@ -7762,7 +7761,7 @@ VPWidenRecipe *VPRecipeBuilder::tryToWiden(VPInstruction *VPI) {
auto *SafeRHS =
Builder.createSelect(Mask, Ops[1], One, VPI->getDebugLoc());
Ops[1] = SafeRHS;
- return new VPWidenRecipe(*I, Ops);
+ return new VPWidenRecipe(*I, Ops, *VPI, VPI->getDebugLoc());
}
[[fallthrough]];
}
@@ -7808,7 +7807,7 @@ VPWidenRecipe *VPRecipeBuilder::tryToWiden(VPInstruction *VPI) {
// For other binops, the legacy cost model only checks the second operand.
NewOps[1] = GetConstantViaSCEV(NewOps[1]);
}
- return new VPWidenRecipe(*I, NewOps);
+ return new VPWidenRecipe(*I, NewOps, *VPI, VPI->getDebugLoc());
}
case Instruction::ExtractValue: {
SmallVector<VPValue *> NewOps(VPI->operands());
@@ -7816,7 +7815,7 @@ VPWidenRecipe *VPRecipeBuilder::tryToWiden(VPInstruction *VPI) {
assert(EVI->getNumIndices() == 1 && "Expected one extractvalue index");
unsigned Idx = EVI->getIndices()[0];
NewOps.push_back(Plan.getConstantInt(32, Idx));
- return new VPWidenRecipe(*I, NewOps);
+ return new VPWidenRecipe(*I, NewOps, *VPI, VPI->getDebugLoc());
}
};
}
@@ -7900,8 +7899,8 @@ VPReplicateRecipe *VPRecipeBuilder::handleReplication(VPInstruction *VPI,
assert((Range.Start.isScalar() || !IsUniform || !IsPredicated ||
(Range.Start.isScalable() && isa<IntrinsicInst>(I))) &&
"Should not predicate a uniform recipe");
- auto *Recipe = new VPReplicateRecipe(I, VPI->operands(), IsUniform,
- BlockInMask, VPIRMetadata(*I, LVer));
+ auto *Recipe =
+ new VPReplicateRecipe(I, VPI->operands(), IsUniform, BlockInMask, *VPI);
return Recipe;
}
@@ -8159,7 +8158,7 @@ VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe(VPSingleDefRecipe *R,
if (Instruction::isCast(VPI->getOpcode())) {
auto *CI = cast<CastInst>(Instr);
return new VPWidenCastRecipe(CI->getOpcode(), VPI->getOperand(0),
- CI->getType(), *CI);
+ CI->getType(), CI, {}, *VPI);
}
return tryToWiden(VPI);
@@ -8189,7 +8188,8 @@ VPRecipeBuilder::tryToCreatePartialReduction(VPInstruction *Reduction,
SmallVector<VPValue *, 2> Ops;
Ops.push_back(Plan.getOrAddLiveIn(Zero));
Ops.push_back(BinOp);
- BinOp = new VPWidenRecipe(*ReductionI, Ops);
+ BinOp = new VPWidenRecipe(*ReductionI, Ops, VPIRMetadata(),
+ ReductionI->getDebugLoc());
Builder.insert(BinOp->getDefiningRecipe());
ReductionOpcode = Instruction::Add;
}
@@ -8229,7 +8229,7 @@ void LoopVectorizationPlanner::buildVPlansWithVPRecipes(ElementCount MinVF,
// candidates built later for specific VF ranges.
auto VPlan0 = VPlanTransforms::buildVPlan0(
OrigLoop, *LI, Legal->getWidestInductionType(),
- getDebugLocFromInstOrOperands(Legal->getPrimaryInduction()), PSE);
+ getDebugLocFromInstOrOperands(Legal->getPrimaryInduction()), PSE, &LVer);
auto MaxVFTimes2 = MaxVF * 2;
for (ElementCount VF = MinVF; ElementCount::isKnownLT(VF, MaxVFTimes2);) {
@@ -8335,7 +8335,7 @@ VPlanPtr LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(
// VPInstructions in the loop.
// ---------------------------------------------------------------------------
VPRecipeBuilder RecipeBuilder(*Plan, OrigLoop, TLI, &TTI, Legal, CM, PSE,
- Builder, BlockMaskCache, LVer);
+ Builder, BlockMaskCache);
RecipeBuilder.collectScaledReductions(Range);
// Scan the body of the loop in a topological order to visit each basic block
@@ -8378,9 +8378,9 @@ VPlanPtr LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(
Legal->isInvariantAddressOfReduction(SI->getPointerOperand())) {
// Only create recipe for the final invariant store of the reduction.
if (Legal->isInvariantStoreOfReduction(SI)) {
- auto *Recipe =
- new VPReplicateRecipe(SI, R.operands(), true /* IsUniform */,
- nullptr /*Mask*/, VPIRMetadata(*SI, LVer));
+ auto *Recipe = new VPReplicateRecipe(
+ SI, R.operands(), true /* IsUniform */, nullptr /*Mask*/,
+ *cast<VPInstruction>(SingleDef));
Recipe->insertBefore(*MiddleVPBB, MBIP);
}
R.eraseFromParent();
@@ -8545,7 +8545,7 @@ VPlanPtr LoopVectorizationPlanner::tryToBuildVPlan(VFRange &Range) {
// addScalarResumePhis.
DenseMap<VPBasicBlock *, VPValue *> BlockMaskCache;
VPRecipeBuilder RecipeBuilder(*Plan, OrigLoop, TLI, &TTI, Legal, CM, PSE,
- Builder, BlockMaskCache, nullptr /*LVer*/);
+ Builder, BlockMaskCache);
for (auto &R : Plan->getVectorLoopRegion()->getEntryBasicBlock()->phis()) {
if (isa<VPCanonicalIVPHIRecipe>(&R))
continue;
diff --git a/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h b/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
index a7000aff06379..87280b83fc0e5 100644
--- a/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
+++ b/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
@@ -84,10 +84,6 @@ class VPRecipeBuilder {
/// A mapping of partial reduction exit instructions to their scaling factor.
DenseMap<const Instruction *, unsigned> ScaledReductionMap;
- /// Loop versioning instance for getting noalias metadata guaranteed by
- /// runtime checks.
- LoopVersioning *LVer;
-
/// Check if \p I can be widened at the start of \p Range and possibly
/// decrease the range such that the returned value holds for the entire \p
/// Range. The function should not be called for memory instructions or calls.
@@ -144,11 +140,9 @@ class VPRecipeBuilder {
LoopVectorizationLegality *Legal,
LoopVectorizationCostModel &CM,
PredicatedScalarEvolution &PSE, VPBuilder &Builder,
- DenseMap<VPBasicBlock *, VPValue *> &BlockMaskCache,
- LoopVersioning *LVer)
+ DenseMap<VPBasicBlock *, VPValue *> &BlockMaskCache)
: Plan(Plan), OrigLoop(OrigLoop), TLI(TLI), TTI(TTI), Legal(Legal),
- CM(CM), PSE(PSE), Builder(Builder), BlockMaskCache(BlockMaskCache),
- LVer(LVer) {}
+ CM(CM), PSE(PSE), Builder(Builder), BlockMaskCache(BlockMaskCache) {}
std::optional<unsigned> getScalingForReduction(const Instruction *ExitInst) {
auto It = ScaledReductionMap.find(ExitInst);
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 5851b3ab7978c..57e376d3267b1 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -1099,9 +1099,10 @@ class LLVM_ABI_FOR_TEST VPInstruction : public VPRecipeWithIRFlags,
public:
VPInstruction(unsigned Opcode, ArrayRef<VPValue *> Operands,
+ const VPIRMetadata &MD = {},
DebugLoc DL = DebugLoc::getUnknown(), const Twine &Name = "")
: VPRecipeWithIRFlags(VPDef::VPInstructionSC, Operands, DL),
- VPIRMetadata(), Opcode(Opcode), Name(Name.str()) {}
+ VPIRMetadata(MD), Opcode(Opcode), Name(Name.str()) {}
VPInstruction(unsigned Opcode, ArrayRef<VPValue *> Operands,
const VPIRFlags &Flags, const VPIRMetadata &MD = {},
@@ -1310,7 +1311,7 @@ class VPPhiAccessors {
struct LLVM_ABI_FOR_TEST VPPhi : public VPInstruction, public VPPhiAccessors {
VPPhi(ArrayRef<VPValue *> Operands, DebugLoc DL, const Twine &Name = "")
- : VPInstruction(Instruction::PHI, Operands, DL, Name) {}
+ : VPInstruction(Instruction::PHI, Operands, {}, DL, Name) {}
static inline bool classof(const VPUser *U) {
auto *VPI = dyn_cast<VPInstruction>(U);
@@ -1453,9 +1454,12 @@ class LLVM_ABI_FOR_TEST VPWidenRecipe : public VPRecipeWithIRFlags,
: VPRecipeWithIRFlags(VPDef::VPWidenSC, Operands, Flags, DL),
VPIRMetadata(Metadata), Opcode(Opcode) {}
- VPWidenRecipe(Instruction &I, ArrayRef<VPValue *> Operands)
- : VPRecipeWithIRFlags(VPDef::VPWidenSC, Operands, I), VPIRMetadata(I),
- Opcode(I.getOpcode()) {}
+ VPWidenRecipe(Instruction &I, ArrayRef<VPValue *> Operands,
+ const VPIRMetadata &Metadata, DebugLoc DL)
+ : VPRecipeWithIRFlags(VPDef::VPWidenSC, Operands, VPIRFlags(I), DL),
+ VPIRMetadata(Metadata), Opcode(I.getOpcode()) {
+ setUnderlyingValue(&I);
+ }
~VPWidenRecipe() override = default;
@@ -1495,31 +1499,26 @@ class VPWidenCastRecipe : public VPRecipeWithIRFlags, public VPIRMetadata {
public:
VPWidenCastRecipe(Instruction::CastOps Opcode, VPValue *Op, Type *ResultTy,
- CastInst &UI)
- : VPRecipeWithIRFlags(VPDef::VPWidenCastSC, Op, UI), VPIRMetadata(UI),
- Opcode(Opcode), ResultTy(ResultTy) {
- assert(UI.getOpcode() == Opcode &&
- "opcode of underlying cast doesn't match");
- }
-
- VPWidenCastRecipe(Instruction::CastOps Opcode, VPValue *Op, Type *ResultTy,
- const VPIRFlags &Flags = {},
+ CastInst *UI = nullptr, const VPIRFlags &Flags = {},
const VPIRMetadata &Metadata = {},
DebugLoc DL = DebugLoc::getUnknown())
- : VPRecipeWithIRFlags(VPDef::VPWidenCastSC, Op, Flags, DL),
+ : VPRecipeWithIRFlags(VPDef::VPWidenCastSC, Op,
+ UI ? VPIRFlags(*UI) : Flags,
+ UI ? UI->getDebugLoc() : DL),
VPIRMetadata(Metadata), Opcode(Opcode), ResultTy(ResultTy) {
assert(flagsValidForOpcode(Opcode) &&
"Set flags not supported for the provided opcode");
+ assert((!UI || UI->getOpcode() == Opcode) &&
+ "opcode of underlying cast doesn't match");
+ setUnderlyingValue(UI);
}
~VPWidenCastRecipe() override = default;
VPWidenCastRecipe *clone() override {
- auto *New = new VPWidenCastRecipe(Opcode, getOperand(0), ResultTy, *this,
- *this, getDebugLoc());
- if (auto *UV = getUnderlyingValue())
- New->setUnderlyingValue(UV);
- return New;
+ return new VPWidenCastRecipe(Opcode, getOperand(0), ResultTy,
+ cast_or_null<CastInst>(getUnderlyingValue()),
+ *this, *this, getDebugLoc());
}
VP_CLASSOF_IMPL(VPDef::VPWidenCastSC)
@@ -1563,9 +1562,10 @@ class VPWidenIntrinsicRecipe : public VPRecipeWithIRFlags, public VPIRMetadata {
public:
VPWidenIntrinsicRecipe(CallInst &CI, Intrinsic::ID VectorIntrinsicID,
ArrayRef<VPValue *> CallArguments, Type *Ty,
+ const VPIRMetadata &MD = {},
DebugLoc DL = DebugLoc::getUnknown())
: VPRecipeWithIRFlags(VPDef::VPWidenIntrinsicSC, CallArguments, CI),
- VPIRMetadata(CI), VectorIntrinsicID(VectorIntrinsicID), ResultTy(Ty),
+ VPIRMetadata(MD), VectorIntrinsicID(VectorIntrinsicID), ResultTy(Ty),
MayReadFromMemory(CI.mayReadFromMemory()),
MayWriteToMemory(CI.mayWriteToMemory()),
MayHaveSideEffects(CI.mayHaveSideEffects()) {}
@@ -1590,7 +1590,8 @@ class VPWidenIntrinsicRecipe : public VPRecipeWithIRFlags, public VPIRMetadata {
VPWidenIntrinsicRecipe *clone() override {
if (Value *CI = getUnderlyingValue())
return new VPWidenIntrinsicRecipe(*cast<CallInst>(CI), VectorIntrinsicID,
- operands(), ResultTy, getDebugLoc());
+ operands(), ResultTy, *this,
+ getDebugLoc());
return new VPWidenIntrinsicRecipe(VectorIntrinsicID, operands(), ResultTy,
getDebugLoc());
}
diff --git a/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp b/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
index f405c40611fcb..34372c1ed75fb 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
@@ -21,6 +21,7 @@
#include "llvm/Analysis/LoopIterator.h"
#include "llvm/Analysis/ScalarEvolution.h"
#include "llvm/IR/MDBuilder.h"
+#include "llvm/Transforms/Utils/LoopVersioning.h"
#define DEBUG_TYPE "vplan"
@@ -36,6 +37,9 @@ class PlainCFGBuilder {
// Loop Info analysis.
LoopInfo *LI;
+ // Loop versioning for alias metadata.
+ LoopVersioning *LVer;
+
// Vectorization plan that we are working on.
std::unique_ptr<VPlan> Plan;
@@ -64,8 +68,8 @@ class PlainCFGBuilder {
void createVPInstructionsForVPBB(VPBasicBlock *VPBB, BasicBlock *BB);
public:
- PlainCFGBuilder(Loop *Lp, LoopInfo *LI)
- : TheLoop(Lp), LI(LI), Plan(std::make_unique<VPlan>(Lp)) {}
+ PlainCFGBuilder(Loop *Lp, LoopInfo *LI, LoopVersioning *LVer)
+ : TheLoop(Lp), LI(LI), LVer(LVer), Plan(std::make_unique<VPlan>(Lp)) {}
/// Build plain CFG for TheLoop and connect it to Plan's entry.
std::unique_ptr<VPlan> buildPlainCFG();
@@ -185,7 +189,8 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB,
// recipes.
if (Br->isConditional()) {
VPValue *Cond = getOrCreateVPOperand(Br->getCondition());
- VPIRBuilder.createNaryOp(VPInstruction::BranchOnCond, {Cond}, Inst);
+ VPIRBuilder.createNaryOp(VPInstruction::BranchOnCond, {Cond}, Inst,
+ VPIRMetadata(*Inst), Inst->getDebugLoc());
}
// Skip the rest of the Instruction processing for Branch instructions.
@@ -199,7 +204,8 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB,
SmallVector<VPValue *> Ops = {getOrCreateVPOperand(SI->getCondition())};
for (auto Case : SI->cases())
Ops.push_back(getOrCreateVPOperand(Case.getCaseValue()));
- VPIRBuilder.createNaryOp(Instruction::Switch, Ops, Inst);
+ VPIRBuilder.createNaryOp(Instruction::Switch, Ops, Inst,
+ VPIRMetadata(*Inst), Inst->getDebugLoc());
continue;
}
@@ -227,6 +233,18 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB,
VPPredToIncomingValue.lookup(Pred->getExitingBasicBlock()));
}
} else {
+ // Build VPIRMetadata from the instruction and add loop versioning
+ // metadata for loads and stores.
+ VPIRMetadata MD(*Inst);
+ if (isa<LoadInst, StoreInst>(Inst) && LVer) {
+ const auto &[AliasScopeMD, NoAliasMD] =
+ LVer->getNoAliasMetadataFor(Inst);
+ if (AliasScopeMD)
+ MD.addMetadata(LLVMContext::MD_alias_scope, AliasScopeMD);
+ if (NoAliasMD)
+ MD.addMetadata(LLVMContext::MD_noalias, NoAliasMD);
+ }
+
// Translate LLVM-IR operands into VPValue operands and set them in the
// new VPInstruction.
SmallVector<VPValue *, 4> VPOperands;
@@ -235,8 +253,8 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB,
// Build VPInstruction for any arbitrary Instruction without specific
// representation in VPlan.
- NewR = cast<VPInstruction>(
- VPIRBuilder.createNaryOp(Inst->getOpcode(), VPOperands, Inst));
+ NewR = cast<VPInstruction>(VPIRBuilder.createNaryOp(
+ Inst->getOpcode(), VPOperands, Inst, MD, Inst->getDebugLoc()));
}
IRDef2VPValue[Inst] = NewR;
@@ -531,8 +549,9 @@ static void addInitialSkeleton(VPlan &Plan, Type *InductionTy, DebugLoc IVDL,
std::unique_ptr<VPlan>
VPlanTransforms::buildVPlan0(Loop *TheLoop, LoopInfo &LI, Type *InductionTy,
- DebugLoc IVDL, PredicatedScalarEvolution &PSE) {
- PlainCFGBuilder Builder(TheLoop, &LI);
+ DebugLoc IVDL, PredicatedScalarEvolution &PSE,
+ LoopVersioning *LVer) {
+ PlainCFGBuilder Builder(TheLoop, &LI, LVer);
std::unique_ptr<VPlan> VPlan0 = Builder.buildPlainCFG();
addInitialSkeleton(*VPlan0, InductionTy, IVDL, PSE, TheLoop);
return VPlan0;
diff --git a/llvm/lib/Transforms/Vectorize/VPlanSLP.cpp b/llvm/lib/Transforms/Vectorize/VPlanSLP.cpp
index bab7a9e4205f7..6b6d26a68942a 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanSLP.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanSLP.cpp
@@ -516,7 +516,8 @@ VPIn...
[truncated]
|
lukel97
reviewed
Nov 10, 2025
| Opcode(I.getOpcode()) {} | ||
| VPWidenRecipe(Instruction &I, ArrayRef<VPValue *> Operands, | ||
| const VPIRMetadata &Metadata, DebugLoc DL) | ||
| : VPRecipeWithIRFlags(VPDef::VPWidenSC, Operands, VPIRFlags(I), DL), |
Contributor
There was a problem hiding this comment.
How come we don't use the VPRecipeWithIRFlags(const unsigned char, ArrayRef<VPValue *>, Instruction) constructor anymore?
Contributor
There was a problem hiding this comment.
I think the two changes here are:
- We're trying to avoid stashing the Instruction
Iin the VPValue that VPSingleDefRecipe derives from. - We want the metadata to come from the
Metadataargument, rather than automatically fromI.
Perhaps it's 2) that's most important, i.e. override the metadata on the instruction based on new information, i.e. runtime checks?
I do think that there are now a confusing number of different constructors for different classes. Perhaps worth adding a comment here @fhahn about why you're favouring one constructor over another to make it easier for the reader?
Contributor
There was a problem hiding this comment.
We're trying to avoid stashing the Instruction I in the VPValue that VPSingleDefRecipe derives from.
It looks like we're still setting the underlying value below, if that's what you mean by stashing the instruction. Maybe if we want to make the underlying value more explicit as well then we should remove the VPRecipeWithIRFlags(const unsigned char, ArrayRef<VPValue *>, Instruction) constructor altogether?
Contributor
Author
There was a problem hiding this comment.
I kept changed this back to use the variant with Instruction for now, as it is unrelated to the patch. I'll soon post a follow-up patch, that does something similar as this patch for VPIRFlags, removing retrieving flags from the underlying instruction.
(using the metadata/flags from the underlying instruction can cause subtle mis-compiles, if the flags were already dropped, then we could add them back by accident when converting recipes)
david-arm
reviewed
Nov 10, 2025
| /// its underlying Instruction. | ||
| VPInstruction *createNaryOp(unsigned Opcode, ArrayRef<VPValue *> Operands, | ||
| Instruction *Inst = nullptr, | ||
| const VPIRMetadata &MD = {}, |
Contributor
There was a problem hiding this comment.
Maybe good to be consistent and also add a similar default parameter to createInstruction?
Contributor
Author
There was a problem hiding this comment.
Currently most helpers/constructors take DL/Name at the end, and the changes her try to keep things consistent.
I could add it to createInstruction and update all places that need. Some places do not support metatadata (like createNot, createAnd.
| const Twine &Name = "") { | ||
| return tryInsertInstruction( | ||
| new VPInstruction(VPInstruction::LogicalAnd, {LHS, RHS}, DL, Name)); | ||
| new VPInstruction(VPInstruction::LogicalAnd, {LHS, RHS}, {}, DL, Name)); |
Contributor
There was a problem hiding this comment.
Use default parameter similar to createNaryOp?
Contributor
Author
There was a problem hiding this comment.
Updated to call createNaryOp here. I did not introduce another argument here, as LogicalAnd does not support any metadata.
| *FMFs, {}, DL, Name) | ||
| : new VPInstruction(Instruction::Select, {Cond, TrueVal, FalseVal}, | ||
| DL, Name); | ||
| {}, DL, Name); |
Contributor
There was a problem hiding this comment.
Use default parameter similar to createNaryOp?
Contributor
Author
There was a problem hiding this comment.
Currently we don't need to set VPIRmetadata for such selects. I undid the changes needed here for now, thanks
| Opcode(I.getOpcode()) {} | ||
| VPWidenRecipe(Instruction &I, ArrayRef<VPValue *> Operands, | ||
| const VPIRMetadata &Metadata, DebugLoc DL) | ||
| : VPRecipeWithIRFlags(VPDef::VPWidenSC, Operands, VPIRFlags(I), DL), |
Contributor
There was a problem hiding this comment.
I think the two changes here are:
- We're trying to avoid stashing the Instruction
Iin the VPValue that VPSingleDefRecipe derives from. - We want the metadata to come from the
Metadataargument, rather than automatically fromI.
Perhaps it's 2) that's most important, i.e. override the metadata on the instruction based on new information, i.e. runtime checks?
I do think that there are now a confusing number of different constructors for different classes. Perhaps worth adding a comment here @fhahn about why you're favouring one constructor over another to make it easier for the reader?
| DebugLoc DL = DebugLoc::getUnknown()) | ||
| : VPRecipeWithIRFlags(VPDef::VPWidenCastSC, Op, Flags, DL), | ||
| : VPRecipeWithIRFlags(VPDef::VPWidenCastSC, Op, | ||
| UI ? VPIRFlags(*UI) : Flags, |
Contributor
There was a problem hiding this comment.
I think this sort of thing could lead to confusion. It's not obvious to the caller that Flags is completely ignored if UI is non-null. I think it would be better to have different explicit constructors for different scenarios, i.e.
VPWidenCastRecipe(Instruction::CastOps Opcode, VPValue *Op, Type *ResultTy,
CastInst &UI, const VPIRMetadata &Metadata = {}) { ... }
VPWidenCastRecipe(Instruction::CastOps Opcode, VPValue *Op, Type *ResultTy,
const VPIRFlags &Flags = {}, DebugLoc DL = DebugLoc::getUnknown(), const VPIRMetadata &Metadata = {}) { ... }
Contributor
Author
There was a problem hiding this comment.
I restored the additional constructor for now, thanks
fhahn
force-pushed
the
vplan-recipe-construction-early
branch
from
29e74c5 to
6521d2a
Compare
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions h,cpp -- llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h llvm/lib/Transforms/Vectorize/LoopVectorize.cpp llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h llvm/lib/Transforms/Vectorize/VPlan.h llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp llvm/lib/Transforms/Vectorize/VPlanSLP.cpp llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp llvm/lib/Transforms/Vectorize/VPlanTransforms.h llvm/unittests/Transforms/Vectorize/VPlanTest.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp b/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
index 20519518d..89fd7d64e 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
@@ -254,12 +254,14 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB,
if (auto *CI = dyn_cast<CastInst>(Inst)) {
NewR = VPIRBuilder.createScalarCast(CI->getOpcode(), VPOperands[0],
- CI->getType(), CI->getDebugLoc(), {}, MD);
+ CI->getType(), CI->getDebugLoc(),
+ {}, MD);
NewR->setUnderlyingValue(CI);
} else {
// Build VPInstruction for any arbitrary Instruction without specific
// representation in VPlan.
- NewR = VPIRBuilder.createNaryOp(Inst->getOpcode(), VPOperands, Inst, MD, Inst->getDebugLoc());
+ NewR = VPIRBuilder.createNaryOp(Inst->getOpcode(), VPOperands, Inst, MD,
+ Inst->getDebugLoc());
}
}
|
Update VPlan to populate VPIRMetadata during VPInstruction construction and use it when creating widened recipes, instead of constructing VPIRMetadata from the underlying IR instruction each time. This centralizes VPIRMetadata in VPInstructions and ensures metadata is consistently available throughout VPlan transformations.
fhahn
force-pushed
the
vplan-recipe-construction-early
branch
from
6521d2a to
8910c1a
Compare
lukel97
approved these changes
Nov 17, 2025
| if (VPI->getOpcode() == Instruction::Load) { | ||
| auto *Load = cast<LoadInst>(I); | ||
| return new VPWidenLoadRecipe(*Load, Ptr, Mask, Consecutive, Reverse, | ||
| VPIRMetadata(*Load, LVer), I->getDebugLoc()); |
Contributor
There was a problem hiding this comment.
Have we removed all the uses of the VPIRMetadata(Instruction, LoopVersioning) constructor now? Can we delete it?
Contributor
|
Looks like one of the VPlan unit tests is failing on CI by the way |
david-arm
reviewed
Nov 17, 2025
| const auto &[AliasScopeMD, NoAliasMD] = | ||
| LVer->getNoAliasMetadataFor(Inst); | ||
| if (AliasScopeMD) | ||
| MD.addMetadata(LLVMContext::MD_alias_scope, AliasScopeMD); |
Contributor
There was a problem hiding this comment.
Is this still NFC? Where were we adding this metadata before your patch?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Update VPlan to populate VPIRMetadata during VPInstruction construction and use it when creating widened recipes, instead of constructing VPIRMetadata from the underlying IR instruction each time.
This centralizes VPIRMetadata in VPInstructions and ensures metadata is consistently available throughout VPlan transformations.