Skip to content

[VPlan] Sink retrieving legacy costs to more specific computeCost impls. #109708

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

Merged
merged 8 commits into from
Oct 9, 2024
Merged

[VPlan] Sink retrieving legacy costs to more specific computeCost impls. #109708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8117ba1
[VPlan] Return Max from computeCost without underlying instrs.
fhahn Sep 23, 2024
f409232
Merge remote-tracking branch 'origin/main' into vplan-max-cost-sentinel
fhahn Oct 6, 2024
0bf2760
!fixup address latest comments, thanks!
fhahn Oct 6, 2024
9f12dab
Merge remote-tracking branch 'origin/main' into vplan-max-cost-sentinel
fhahn Oct 8, 2024
16ca2ca
Merge remote-tracking branch 'origin/main' into vplan-max-cost-sentinel
fhahn Oct 8, 2024
f632630
!fixup rework
fhahn Oct 8, 2024
c75d678
Merge remote-tracking branch 'origin/main' into vplan-max-cost-sentinel
fhahn Oct 9, 2024
d32eb48
!Fixup address latest comments, thanks!
fhahn Oct 9, 2024
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
16 changes: 16 additions & 0 deletions llvm/lib/Transforms/Vectorize/VPlan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,6 +930,10 @@ class VPSingleDefRecipe : public VPRecipeBase, public VPValue {
const Instruction *getUnderlyingInstr() const {
return cast<Instruction>(getUnderlyingValue());
}

/// Return the cost of this VPSingleDefRecipe.
InstructionCost computeCost(ElementCount VF,
VPCostContext &Ctx) const override;
};

/// Class to record LLVM IR flag for a recipe along with it.
Expand Down Expand Up @@ -1411,6 +1415,10 @@ class VPIRInstruction : public VPRecipeBase {

void execute(VPTransformState &State) override;

/// Return the cost of this VPIRInstruction.
InstructionCost computeCost(ElementCount VF,
VPCostContext &Ctx) const override;

Instruction &getInstruction() { return I; }

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
Expand Down Expand Up @@ -2391,6 +2399,10 @@ class VPInterleaveRecipe : public VPRecipeBase {
/// Generate the wide load or store, and shuffles.
void execute(VPTransformState &State) override;

/// Return the cost of this VPInterleaveRecipe.
InstructionCost computeCost(ElementCount VF,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to /// document, here and elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added thanks!

VPCostContext &Ctx) const override;

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
/// Print the recipe.
void print(raw_ostream &O, const Twine &Indent,
Expand Down Expand Up @@ -2624,6 +2636,10 @@ class VPBranchOnMaskRecipe : public VPRecipeBase {
/// conditional branch.
void execute(VPTransformState &State) override;

/// Return the cost of this VPBranchOnMaskRecipe.
InstructionCost computeCost(ElementCount VF,
VPCostContext &Ctx) const override;

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
/// Print the recipe.
void print(raw_ostream &O, const Twine &Indent,
Expand Down
80 changes: 49 additions & 31 deletions llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,33 +280,28 @@ void VPRecipeBase::moveBefore(VPBasicBlock &BB,
insertBefore(BB, I);
}

/// Return the underlying instruction to be used for computing \p R's cost via
/// the legacy cost model. Return nullptr if there's no suitable instruction.
static Instruction *getInstructionForCost(const VPRecipeBase *R) {
if (auto *S = dyn_cast<VPSingleDefRecipe>(R))
return dyn_cast_or_null<Instruction>(S->getUnderlyingValue());
if (auto *IG = dyn_cast<VPInterleaveRecipe>(R))
return IG->getInsertPos();
// Currently the legacy cost model only calculates the instruction cost with
// underlying instruction. Removing the WidenMem here will prevent
// force-target-instruction-cost overwriting the cost of recipe with
// underlying instruction which is inconsistent with the legacy model.
// TODO: Remove WidenMem from this function when we don't need to compare to
// the legacy model.
if (auto *WidenMem = dyn_cast<VPWidenMemoryRecipe>(R))
return &WidenMem->getIngredient();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth leaving a note, that even though widen loads and stores have an underlying instruction ("ingredient"), null is returned - in order to match legacy cost model behavior with force-target-instruction-cost?

The instruction a recipe provides for cost affects both

  • skipCostComputation() - widen loads and stores are irrelevant for skipping?
  • force-target-instruction-cost - widen loads and stores should be assigned zero instead of this overriding cost?
  • w/o force-target-instruction-cost - should be assigned zero cost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified the the comment about the return value for the function.

Nullptr is returned if either the recipe doesn't have an underlying instruction or computeCost is implemented and there is no need for the underlying instruction (i.e. it should not be needed to be skipped)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nullptr is returned if either the recipe doesn't have an underlying instruction or computeCost is implemented and there is no need for the underlying instruction (i.e. it should not be needed to be skipped)

Regarding the "else" part of this "or" - not needing UI because recipe is not to be skipped is fine (if it's absence from SkipCostComputation it won't be skipped), but would UI still be needed in order to apply ForceTargetInstructionCost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been reworked in the latest version. The function is gone and inline to ::cost(), which clarifies that it is only used to decide to skip and when to apply forced costs.

return nullptr;
}

InstructionCost VPRecipeBase::cost(ElementCount VF, VPCostContext &Ctx) {
auto *UI = getInstructionForCost(this);
if (UI && Ctx.skipCostComputation(UI, VF.isVector()))
return 0;

InstructionCost RecipeCost = computeCost(VF, Ctx);
if (UI && ForceTargetInstructionCost.getNumOccurrences() > 0 &&
RecipeCost.isValid())
RecipeCost = InstructionCost(ForceTargetInstructionCost);
// Get the underlying instruction for the recipe, if there is one. It is used
// to
// * decide if cost computation should be skipped for this recipe,
// * apply forced target instruction cost.
Instruction *UI = nullptr;
if (auto *S = dyn_cast<VPSingleDefRecipe>(this))
UI = dyn_cast_or_null<Instruction>(S->getUnderlyingValue());
else if (auto *IG = dyn_cast<VPInterleaveRecipe>(this))
UI = IG->getInsertPos();
else if (auto *WidenMem = dyn_cast<VPWidenMemoryRecipe>(this))
UI = &WidenMem->getIngredient();

InstructionCost RecipeCost;
if (UI && Ctx.skipCostComputation(UI, VF.isVector())) {
RecipeCost = 0;
} else {
RecipeCost = computeCost(VF, Ctx);
if (UI && ForceTargetInstructionCost.getNumOccurrences() > 0 &&
RecipeCost.isValid())
RecipeCost = InstructionCost(ForceTargetInstructionCost);
}

LLVM_DEBUG({
dbgs() << "Cost of " << RecipeCost << " for VF " << VF << ": ";
Expand All @@ -317,11 +312,14 @@ InstructionCost VPRecipeBase::cost(ElementCount VF, VPCostContext &Ctx) {

InstructionCost VPRecipeBase::computeCost(ElementCount VF,
VPCostContext &Ctx) const {
// Compute the cost for the recipe falling back to the legacy cost model using
// the underlying instruction. If there is no underlying instruction, returns
// 0.
Instruction *UI = getInstructionForCost(this);
if (UI && isa<VPReplicateRecipe>(this)) {
llvm_unreachable("subclasses should implement computeCost");
}

InstructionCost VPSingleDefRecipe::computeCost(ElementCount VF,
VPCostContext &Ctx) const {
Instruction *UI = dyn_cast_or_null<Instruction>(getUnderlyingValue());
if (isa<VPReplicateRecipe>(this)) {
assert(UI && "VPReplicateRecipe must have an underlying instruction");
// VPReplicateRecipe may be cloned as part of an existing VPlan-to-VPlan
// transform, avoid computing their cost multiple times for now.
Ctx.SkipCostComputation.insert(UI);
Expand Down Expand Up @@ -870,6 +868,13 @@ void VPIRInstruction::execute(VPTransformState &State) {
State.Builder.SetInsertPoint(I.getParent(), std::next(I.getIterator()));
}

InstructionCost VPIRInstruction::computeCost(ElementCount VF,
VPCostContext &Ctx) const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some comment explaining that this wraps an existing IR instruction on the border of VPlan's scope, hence does not contribute to VPlan's cost modeling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, thanks!

// The recipe wraps an existing IR instruction on the border of VPlan's scope,
// hence it does not contribute to the cost-modeling for the VPlan.
return 0;
}

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
void VPIRInstruction::print(raw_ostream &O, const Twine &Indent,
VPSlotTracker &SlotTracker) const {
Expand Down Expand Up @@ -2210,6 +2215,14 @@ void VPBranchOnMaskRecipe::execute(VPTransformState &State) {
ReplaceInstWithInst(CurrentTerminator, CondBr);
}

InstructionCost VPBranchOnMaskRecipe::computeCost(ElementCount VF,
VPCostContext &Ctx) const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some explanation for treating this branch as free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added thanks!

// The legacy cost model doesn't assign costs to branches for individual
// replicate regions. Match the current behavior in the VPlan cost model for
// now.
return 0;
}

void VPPredInstPHIRecipe::execute(VPTransformState &State) {
assert(State.Lane && "Predicated instruction PHI works per instance.");
Instruction *ScalarPredInst =
Expand Down Expand Up @@ -2892,6 +2905,11 @@ void VPInterleaveRecipe::print(raw_ostream &O, const Twine &Indent,
}
#endif

InstructionCost VPInterleaveRecipe::computeCost(ElementCount VF,
VPCostContext &Ctx) const {
return Ctx.getLegacyCost(IG->getInsertPos(), VF);
}

void VPCanonicalIVPHIRecipe::execute(VPTransformState &State) {
Value *Start = getStartValue()->getLiveInIRValue();
PHINode *Phi = PHINode::Create(Start->getType(), 2, "index");
Expand Down
Loading