Skip to content
Merged
4 changes: 3 additions & 1 deletion llvm/lib/Transforms/Vectorize/VPlan.h
Original file line number Diff line number Diff line change
Expand Up @@ -828,7 +828,9 @@ class VPRecipeBase : public ilist_node_with_parent<VPRecipeBase, VPBasicBlock>,
protected:
/// Compute the cost of this recipe either using a recipe's specialized
/// implementation or using the legacy cost model and the underlying
/// instructions.
/// instructions. Returns InstructionCost::max() if the cost of this recipe
Copy link
Collaborator

Choose a reason for hiding this comment

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

The underlying instruction can be passed as argument, except that this base implementation should be a default for all derived VPlan-based computations, where the latter should be independent on UI, if possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, also there are some computeCost implementation for recipes with underlying instruction, for which the default should not trigger. In particular, all recipes which currently have computeCost implemented in subclasses should be included with force-target-instruction-cost.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively computeCost could return an optional cost.

Perhaps better to return no cost at all, as in optional, when cost should be ignored, rather than a max sentinel. The latter requires additional attention to remain unmodified, preventing (mistaken) updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change is not needed in latest version.

/// should be ignored. Forced target instruction cost is not applied for such
/// recipes.
virtual InstructionCost computeCost(ElementCount VF,
VPCostContext &Ctx) const;
};
Expand Down
31 changes: 17 additions & 14 deletions llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -275,20 +275,15 @@ void VPRecipeBase::moveBefore(VPBasicBlock &BB,
}

/// 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.
/// the legacy cost model. Return nullptr if there's no suitable instruction or
/// computeCost is already implemented for the recipe and there is no need for
/// the underlying instruction, i.e. it does not need to be skipped for cost
/// computations.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The gotten Instruction For Cost is used for (a) computing cost via legacy model - zero or greater, valid or not, (b) skipping cost - valid zero cost is used if skipped, (c) qualifying for Forced cost overwriting - valid zero cost is used if unqualified. All uses agree on UI or null?

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 completely changed in the latest version.

I've implemented ::computeCost for recipes as needed, so there's no need to use this function with getLegacyCost. This more clearly scopes where the legacy cost is still used and how the instructions are obtained (recipe specific). There's no need to remove recipes from getInstructionForCost as their computeCost moves to not use legacy cost.

static Instruction *getInstructionForCost(const VPRecipeBase *R) {
if (auto *S = dyn_cast<VPSingleDefRecipe>(R))
return dyn_cast_or_null<Instruction>(S->getUnderlyingValue());
Copy link
Collaborator

Choose a reason for hiding this comment

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

While we're here, this could return S->getUnderlyingInstr(), if the latter does cast_if_present rather than cast?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I think originally the intention was to only use getUnderlyingInstr if it is guaranteed to be available, but perhaps it is time to revisit this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, right. May be good to revisit this intention.

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

Expand All @@ -298,9 +293,13 @@ InstructionCost VPRecipeBase::cost(ElementCount VF, VPCostContext &Ctx) {
return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that skipped costs also skip debug dump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest version


InstructionCost RecipeCost = computeCost(VF, Ctx);
if (UI && ForceTargetInstructionCost.getNumOccurrences() > 0 &&
RecipeCost.isValid())
if (ForceTargetInstructionCost.getNumOccurrences() > 0 &&
(RecipeCost.isValid() && RecipeCost != InstructionCost::getMax()))
RecipeCost = InstructionCost(ForceTargetInstructionCost);
// Max cost is used as a sentinel value to detect recipes without underlying
// instructions for which no forced target instruction cost should be applied.
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 indicate why such a sentinel is needed, rather than detecting the absence of an underlying instruction by directly checking UI, as done now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No sentinel is needed in the latest version, it uses UI now, thanks!

else if (RecipeCost == InstructionCost::getMax())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the logic be as follows:

If UI is to be skipped, zero cost should be (dumped? and) returned.

If VPlan-based cost cannot be computed (yet, i.e., computeCost() returned null), fallback on legacy cost model using UI - asserting UI is available?

If computed cost (VPlan or legacy based) is invalid, it should be (dumped and) returned.

If ForceTargetInstructionCost, the cost should be either ForceTargetInstructionCost or zero, depending on whether UI exists or not.

Sounds right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworked to implement the relevant ::comptueCosts to return the legacy cost, and use UI to decide when to skip and apply forced cost.

RecipeCost = 0;

LLVM_DEBUG({
dbgs() << "Cost of " << RecipeCost << " for VF " << VF << ": ";
Expand All @@ -312,15 +311,19 @@ 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.
// the underlying instruction. If there is no underlying instruction or the
// cost is computed by the recipe's computeCost, returns
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "else" part of the "or" is unclear, at-least in this patch alone - getMax is returned below if and only if there is no underlying instruction (assuming Ctx.getLegacyCost() never returns Max). Concrete recipes may return Max even if they have an underlying instruction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully clearer in the latest version.

// InstructionCost::getMax. It is used as a sentinel value to detect recipes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// InstructionCost::getMax. It is used as a sentinel value to detect recipes
// InstructionCost::getMax. It is used as a sentinel value to detect recipes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment gone in the latest version.

// without underlying instructions for which no forced target instruction cost
// should be applied.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// without underlying instructions for which no forced target instruction cost
// should be applied.
// without underlying instructions for which forced target instruction cost
// should not be applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment gone in the latest version.


Instruction *UI = getInstructionForCost(this);
if (UI && isa<VPReplicateRecipe>(this)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(independent) Have VPReplicateRecipe::computeCost() take care of inserting UI into SkipCostComputation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no such implementation yet, but can be added soon! Now moved slightly closer to VPSingleDefRecipe::computeCost.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that replicate recipes by definition must have a UI, so the latter can be asserted instead of checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

// 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);
}
return UI ? Ctx.getLegacyCost(UI, VF) : 0;
return UI ? Ctx.getLegacyCost(UI, VF) : InstructionCost::getMax();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment above about returning zero when no underlying instruction is there, should be updated.

Under ForceTargetInstructionCost, all UI's with valid costs are assigned ForceTargetInstructionCost cost. Therefore, under ForceTargetInstructionCost, all recipes w/ UI's and valid costs should also be assigned this cost, and all recipes w/o UI's should be assigned zero cost?

Need to distinguish between (zero, but could be any other valid) cost of a recipe that has no underlying, which should be ignored by ForceTargetInstructionCost because legacy only addresses UI's, and recipes w/ underlying (whose legacy cost is returned, can be zero or any other valid cost) which are subject to 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.

<Move the comment above to update the existing comment, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps the default base-class implementation of VPRecipeBase::computeCost() should return null, delegating the fallback use of Ctx.getLegacyCost(UI, VF) to its caller VPRecipeBase::cost(), as outlined above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted to use getLegacyCost in a few relevant sub-classes (VPSingleDefRecipe/VPInterleaveRecipe). Should hopefully be clearer and simpler now

}

FastMathFlags VPRecipeWithIRFlags::getFastMathFlags() const {
Expand Down
Loading