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

fhahn · 2024-09-23T19:29:01Z

Make legacy cost retrieval independent of getInstructionForCost by sinking it to more specific ::computeCost implementation (specifically VPInterleaveRecipe::computeCost and VPSingleDefRecipe::computeCost).

Inline getInstructionForCost to VPRecipeBase::cost(), as it is now only used to decide which recipes to skip during cost computation and when to apply forced costs.

Update computeCost to return InstructionCost::getMax() for recipes without underlying instructions. Max is used as a sentinel value to handle cases where there is no underlying instruction. At the moment we need to catch those cases when -force-target-instruction-cost is passed to avoid applying the forced cost to auxiliary recipes (like scalar steps). This is needed to match the legacy behavior. Unfortunately we cannot use InstructionCost::getInvalid, as this is used to indicate that an instruction cannot be legalized (e.g. for scalable vectors). Alternatively computeCost could return an optional cost.

llvmbot · 2024-09-23T19:29:35Z

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Update computeCost to return InstructionCost::getMax() for recipes without underlying instructions. Max is used as a sentinel value to handle cases where there is no underlying instruction. At the moment we need to catch those cases when -force-target-instruction-cost is passed to avoid applying the forced cost to auxiliary recipes (like scalar steps). This is needed to match the legacy behavior.

Unfortunately we cannot use InstructionCost::getInvalid, as this is used to indicate that an instruction cannot be legalized (e.g. for scalable vectors).

Alternatively computeCost could return an optional cost.

Full diff: https://github.com/llvm/llvm-project/pull/109708.diff

2 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/VPlan.h (+3-1)
(modified) llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp (+9-11)

diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 0632495bc511cd..6f8a44e1cd1802 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -844,7 +844,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
+  /// should be ignored. Forced target instruction cost is not applied for such
+  /// recipes.
   virtual InstructionCost computeCost(ElementCount VF,
                                       VPCostContext &Ctx) const;
 };
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index 3f7ab416e877bc..ca4622b74275e5 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -276,14 +276,6 @@ static Instruction *getInstructionForCost(const VPRecipeBase *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();
   return nullptr;
 }
 
@@ -293,9 +285,13 @@ InstructionCost VPRecipeBase::cost(ElementCount VF, VPCostContext &Ctx) {
     return 0;
 
   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.
+  if (RecipeCost == InstructionCost::getMax())
+    RecipeCost = 0;
 
   LLVM_DEBUG({
     dbgs() << "Cost of " << RecipeCost << " for VF " << VF << ": ";
@@ -315,7 +311,9 @@ InstructionCost VPRecipeBase::computeCost(ElementCount VF,
     // transform, avoid computing their cost multiple times for now.
     Ctx.SkipCostComputation.insert(UI);
   }
-  return UI ? Ctx.getLegacyCost(UI, VF) : 0;
+  // Max cost is used as a sentinel value to detect recipes without underlying
+  // instructions for which no forced target instruction cost should be applied.
+  return UI ? Ctx.getLegacyCost(UI, VF) : InstructionCost::getMax();
 }
 
 FastMathFlags VPRecipeWithIRFlags::getFastMathFlags() const {

ayalz · 2024-09-24T09:03:29Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

@@ -276,14 +276,6 @@ static Instruction *getInstructionForCost(const VPRecipeBase *R) {
    return dyn_cast_or_null<Instruction>(S->getUnderlyingValue());


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

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?

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

ayalz · 2024-09-24T09:07:32Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

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


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?

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)

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?

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.

ayalz · 2024-09-24T13:17:31Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  // Max cost is used as a sentinel value to detect recipes without underlying
+  // instructions for which no forced target instruction cost should be applied.
+  if (RecipeCost == InstructionCost::getMax())
+    RecipeCost = 0;


So cost is essentially a valid non-overridable/un-forceable value, of zero?

Would adding, say, a "Permanent" state to CostState, along with Valid and InValid, be reasonable?

Yes exactly. Not sure if it would be worth adding a new state for this very specific use case?

ayalz · 2024-09-24T13:18:18Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

@@ -293,9 +285,13 @@ InstructionCost VPRecipeBase::cost(ElementCount VF, VPCostContext &Ctx) {
    return 0;


Note that skipped costs also skip debug dump.

Fixed in the latest version

ayalz · 2024-09-24T13:18:50Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

    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.
+  if (RecipeCost == InstructionCost::getMax())


elseif? (Otherwise a ForceTargetInstructionCost of Max will be replaced by zero.)

updated, thanks!

ayalz · 2024-09-24T13:21:09Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

-  return UI ? Ctx.getLegacyCost(UI, VF) : 0;
+  // Max cost is used as a sentinel value to detect recipes without underlying
+  // instructions for which no forced target instruction cost should be applied.
+  return UI ? Ctx.getLegacyCost(UI, VF) : InstructionCost::getMax();


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.

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

ayalz · 2024-09-24T13:23:15Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -844,7 +844,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


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?

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.

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.

Change is not needed in latest version.

ayalz · 2024-10-08T09:57:44Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -844,7 +844,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


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.

ayalz · 2024-10-08T10:03:45Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+/// 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.


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?

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.

ayalz · 2024-10-08T10:05:49Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

@@ -276,14 +276,6 @@ static Instruction *getInstructionForCost(const VPRecipeBase *R) {
    return dyn_cast_or_null<Instruction>(S->getUnderlyingValue());


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

ayalz · 2024-10-08T13:00:14Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

    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.


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?

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

ayalz · 2024-10-08T13:07:22Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  // InstructionCost::getMax. It is used as  a sentinel value to detect recipes
+  // without underlying instructions for which no forced target instruction cost
+  // should be applied.
+
  Instruction *UI = getInstructionForCost(this);
  if (UI && isa<VPReplicateRecipe>(this)) {


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

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

ayalz · 2024-10-08T13:09:56Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

-  // 0.
+  // the underlying instruction. If there is no underlying instruction or the
+  // cost is computed by the recipe's computeCost, returns
+  // InstructionCost::getMax. It is used as  a sentinel value to detect recipes


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

Comment gone in the latest version.

ayalz · 2024-10-08T13:10:13Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  // without underlying instructions for which no forced target instruction cost
+  // should be applied.


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.

Comment gone in the latest version.

ayalz · 2024-10-08T13:17:43Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

  Instruction *UI = getInstructionForCost(this);
  if (UI && isa<VPReplicateRecipe>(this)) {
    // 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();


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?

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

ayalz · 2024-10-08T13:50:48Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

    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.
+  else if (RecipeCost == InstructionCost::getMax())


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?

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

ayalz · 2024-10-08T14:00:44Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

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


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?

fhahn

The latest version changed quite drastically.

Now no sentinel value is used. Instead VPSingleDefRecipe/VPInterleaveRecipe return their cost via legacy, which removes the use of getInstructionForCost to return legacy costs. getInstructionForCost is only used in ::cost to divide what to skip and when to apply forced costs.

Hopefully this should be simpler and clearer. WDYT?

fhahn · 2024-10-08T18:50:27Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -844,7 +844,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


Change is not needed in latest version.

fhahn · 2024-10-08T18:52:47Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+/// 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.


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.

fhahn · 2024-10-08T18:53:53Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

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


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.

fhahn · 2024-10-08T18:54:01Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

@@ -293,9 +285,13 @@ InstructionCost VPRecipeBase::cost(ElementCount VF, VPCostContext &Ctx) {
    return 0;


Fixed in the latest version

fhahn · 2024-10-08T18:54:22Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

    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.


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

fhahn · 2024-10-08T18:55:26Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  // the underlying instruction. If there is no underlying instruction or the
+  // cost is computed by the recipe's computeCost, returns


Hopefully clearer in the latest version.

fhahn · 2024-10-08T18:55:32Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

-  // 0.
+  // the underlying instruction. If there is no underlying instruction or the
+  // cost is computed by the recipe's computeCost, returns
+  // InstructionCost::getMax. It is used as  a sentinel value to detect recipes


Comment gone in the latest version.

fhahn · 2024-10-08T18:55:41Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  // without underlying instructions for which no forced target instruction cost
+  // should be applied.


Comment gone in the latest version.

fhahn · 2024-10-08T18:56:13Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  // InstructionCost::getMax. It is used as  a sentinel value to detect recipes
+  // without underlying instructions for which no forced target instruction cost
+  // should be applied.
+
  Instruction *UI = getInstructionForCost(this);
  if (UI && isa<VPReplicateRecipe>(this)) {


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

fhahn · 2024-10-08T18:57:05Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

  Instruction *UI = getInstructionForCost(this);
  if (UI && isa<VPReplicateRecipe>(this)) {
    // 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();


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

ayalz

Thanks for reworking this, LGTM!
Sinking legacy costs into individual usages will hopefully help refactor them into VPlan.
Added some minor comments.

ayalz · 2024-10-08T20:06:03Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -915,6 +915,9 @@ class VPSingleDefRecipe : public VPRecipeBase, public VPValue {
    llvm_unreachable("Unhandled VPDefID");
  }

+  InstructionCost computeCost(ElementCount VF,
+                              VPCostContext &Ctx) const override;
+


nit: can a better position be found, rather than between two static classof()'s?

Moved to the end. thanks!

ayalz · 2024-10-08T20:58:11Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -2326,6 +2332,9 @@ class VPInterleaveRecipe : public VPRecipeBase {
  /// Generate the wide load or store, and shuffles.
  void execute(VPTransformState &State) override;

+  InstructionCost computeCost(ElementCount VF,


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

Added thanks!

ayalz · 2024-10-08T20:59:00Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

-  if (UI && ForceTargetInstructionCost.getNumOccurrences() > 0 &&
-      RecipeCost.isValid())
-    RecipeCost = InstructionCost(ForceTargetInstructionCost);
+  // Get the underlying instruction for the recipe, if there is one. Is is used


Suggested change

// Get the underlying instruction for the recipe, if there is one. Is is used

// Get the underlying instruction for the recipe, if there is one. It is used

Fixed, thanks!

ayalz · 2024-10-08T21:14:42Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

@@ -864,6 +861,11 @@ void VPIRInstruction::execute(VPTransformState &State) {
  State.Builder.SetInsertPoint(I.getParent(), std::next(I.getIterator()));
 }

+InstructionCost VPIRInstruction::computeCost(ElementCount VF,
+                                             VPCostContext &Ctx) const {


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.

Added, thanks!

ayalz · 2024-10-08T21:15:22Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

@@ -2156,6 +2158,11 @@ void VPBranchOnMaskRecipe::execute(VPTransformState &State) {
  ReplaceInstWithInst(CurrentTerminator, CondBr);
 }

+InstructionCost VPBranchOnMaskRecipe::computeCost(ElementCount VF,
+                                                  VPCostContext &Ctx) const {


Some explanation for treating this branch as free.

Added thanks!

ayalz · 2024-10-08T21:18:55Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+
+InstructionCost VPSingleDefRecipe::computeCost(ElementCount VF,
+                                               VPCostContext &Ctx) const {
+  Instruction *UI = dyn_cast_or_null<Instruction>(getUnderlyingValue());
  if (UI && isa<VPReplicateRecipe>(this)) {


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

Done, thanks!

ayalz · 2024-10-08T21:24:01Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  // Get the underlying instruction for the recipe, if there is one. Is is used
+  // to
+  //   * decide if cost computation should be skipped for this recipe
+  //   * apply forced target instr


Suggested change

// * apply forced target instr

// * apply forced target instruction cost.

Fixed thanks!

fhahn requested review from rengolin, ayalz, aniragil and ElvisWang123 September 23, 2024 19:29

llvmbot added vectorizers llvm:transforms labels Sep 23, 2024

ayalz reviewed Sep 24, 2024

View reviewed changes

fhahn added 2 commits October 6, 2024 10:35

Merge remote-tracking branch 'origin/main' into vplan-max-cost-sentinel

f409232

!fixup address latest comments, thanks!

0bf2760

ayalz reviewed Oct 8, 2024

View reviewed changes

fhahn added 2 commits October 8, 2024 15:14

Merge remote-tracking branch 'origin/main' into vplan-max-cost-sentinel

9f12dab

Merge remote-tracking branch 'origin/main' into vplan-max-cost-sentinel

16ca2ca

fhahn force-pushed the vplan-max-cost-sentinel branch from b7440b3 to 7fcddd6 Compare October 8, 2024 18:49

fhahn commented Oct 8, 2024

View reviewed changes

!fixup rework

f632630

fhahn changed the title ~~[VPlan] Return Max from computeCost without underlying instrs.~~ [VPlan] Sink retrieving legacy costs to more specific computeCost impls. Oct 8, 2024

fhahn force-pushed the vplan-max-cost-sentinel branch from 7fcddd6 to f632630 Compare October 8, 2024 19:00

ayalz approved these changes Oct 8, 2024

View reviewed changes

fhahn added 2 commits October 9, 2024 13:26

Merge remote-tracking branch 'origin/main' into vplan-max-cost-sentinel

c75d678

!Fixup address latest comments, thanks!

d32eb48

fhahn merged commit fa3258e into llvm:main Oct 9, 2024
6 of 9 checks passed

fhahn deleted the vplan-max-cost-sentinel branch October 9, 2024 12:59

fhahn mentioned this pull request Oct 9, 2024

[VPlan] Implement VPInterleaveRecipe::computeCost. #106067

Merged

		@@ -276,14 +276,6 @@ static Instruction getInstructionForCost(const VPRecipeBase R) {
		return dyn_cast_or_null<Instruction>(S->getUnderlyingValue());

		@@ -293,9 +285,13 @@ InstructionCost VPRecipeBase::cost(ElementCount VF, VPCostContext &Ctx) {
		return 0;

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

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

		// the underlying instruction. If there is no underlying instruction or the
		// cost is computed by the recipe's computeCost, returns

	// Get the underlying instruction for the recipe, if there is one. Is is used
	// Get the underlying instruction for the recipe, if there is one. It is used

	// * apply forced target instr
	// * apply forced target instruction cost.

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

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

Uh oh!

Conversation

fhahn commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fhahn commented Sep 23, 2024 •

edited

Loading