[VPlan][NFC] Add cost for `VPWidenMemoryRecipe`. #105614

ElvisWang123 · 2024-08-22T04:53:34Z

In this patch, we add the computeCost() function for VPWidenMemoryRecipe.

llvmbot · 2024-08-22T04:54:03Z

@llvm/pr-subscribers-llvm-transforms

Author: Elvis Wang (ElvisWang123)

Changes

In this patch, we add the computeCost() function for VPWidenMemoryRecipe.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/VPlan.h (+7)
(modified) llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp (+31)

diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index a99f3882092c2c..1f6de54822ebcf 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -2529,6 +2529,13 @@ class VPWidenMemoryRecipe : public VPRecipeBase {
     llvm_unreachable("VPWidenMemoryRecipe should not be instantiated.");
   }
 
+  /// Get element Type
+  Type *getElementType() const { return getLoadStoreType(&Ingredient); }
+
+  /// Return the cost of this VPWidenMemoryRecipe.
+  InstructionCost computeCost(ElementCount VF,
+                              VPCostContext &Ctx) const override;
+
   Instruction &getIngredient() const { return Ingredient; }
 };
 
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index c9d603612aecea..d3b36b145d470e 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -2084,6 +2084,37 @@ void VPPredInstPHIRecipe::print(raw_ostream &O, const Twine &Indent,
 }
 #endif
 
+InstructionCost VPWidenMemoryRecipe::computeCost(ElementCount VF,
+                                                 VPCostContext &Ctx) const {
+  Instruction *I = getInstructionForCost(this);
+  Type *Ty = ToVectorTy(getElementType(), VF);
+  const Align Alignment = getLoadStoreAlignment(const_cast<Instruction *>(I));
+  const Value *Ptr = getLoadStorePointerOperand(I);
+  unsigned AS = getLoadStoreAddressSpace(const_cast<Instruction *>(I));
+  TTI::TargetCostKind CostKind = TTI::TCK_RecipThroughput;
+
+  if (Consecutive) {
+    InstructionCost Cost = 0;
+    if (IsMasked) {
+      Cost += Ctx.TTI.getMaskedMemoryOpCost(I->getOpcode(), Ty, Alignment, AS,
+                                            CostKind);
+    } else {
+      TTI::OperandValueInfo OpInfo = Ctx.TTI.getOperandInfo(I->getOperand(0));
+      Cost += Ctx.TTI.getMemoryOpCost(I->getOpcode(), Ty, Alignment, AS,
+                                      CostKind, OpInfo, I);
+    }
+    if (Reverse)
+      Cost += Ctx.TTI.getShuffleCost(TargetTransformInfo::SK_Reverse,
+                                     cast<VectorType>(Ty), std::nullopt,
+                                     CostKind, 0);
+
+    return Cost;
+  }
+  return Ctx.TTI.getAddressComputationCost(Ty) +
+         Ctx.TTI.getGatherScatterOpCost(I->getOpcode(), Ty, Ptr, IsMasked,
+                                        Alignment, CostKind, I);
+}
+
 void VPWidenLoadRecipe::execute(VPTransformState &State) {
   auto *LI = cast<LoadInst>(&Ingredient);

fhahn

This should be effectively NFC, might be good to tag as such

fhahn · 2024-08-26T09:11:20Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp


+InstructionCost VPWidenMemoryRecipe::computeCost(ElementCount VF,
+                                                 VPCostContext &Ctx) const {
+  Instruction *I = getInstructionForCost(this);


This should probably use Ingredient

fhahn · 2024-08-26T09:15:47Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  Instruction *I = getInstructionForCost(this);
+  Type *Ty = ToVectorTy(getElementType(), VF);
+  const Align Alignment = getLoadStoreAlignment(const_cast<Instruction *>(I));
+  const Value *Ptr = getLoadStorePointerOperand(I);


Sink closer to use? Is this required? Using the original IR pointer here won't be accurate I think. If needed, please leave a TODO behind to refactor reliance on underlying IR here and explain why it is currently needed.

Only ArmTTI is using the underlying IR to return different gather/scatter cost currently.

fhahn · 2024-08-26T09:16:10Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+
+    return Cost;
+  }
+  return Ctx.TTI.getAddressComputationCost(Ty) +


maybe handle the simpler case (!Consecutive) first with an early return?

fhahn · 2024-08-26T09:17:05Z

llvm/lib/Transforms/Vectorize/VPlan.h

  }

+  /// Get element Type
+  Type *getElementType() const { return getLoadStoreType(&Ingredient); }


Simpler to just use getLoadStoreType directly in ::computeCost, like other getLoadStoreXXX helpers there?

Thanks, I will directly use the getLoadStoreType.

fhahn

With this change, you should also be able to remove VPWidenMemoryRecipe from getInstructionForCost in VPlan.cpp

fhahn

With the change it should also be possible to remove VPWidenMemoryRecipe from getInstructionForCost in VPlan.cpp

fhahn · 2024-08-27T08:21:32Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  if (Reverse)
+    Cost +=
+        Ctx.TTI.getShuffleCost(TargetTransformInfo::SK_Reverse,
+                               cast<VectorType>(Ty), std::nullopt, CostKind, 0);
+
+  return Cost;


nit: handling the non-reverse case with an early exit may be slightly more compact

Suggested change

if (Reverse)

Cost +=

Ctx.TTI.getShuffleCost(TargetTransformInfo::SK_Reverse,

cast<VectorType>(Ty), std::nullopt, CostKind, 0);

return Cost;

if (!Reverse)

return Cost;

return

Cost +

Ctx.TTI.getShuffleCost(TargetTransformInfo::SK_Reverse,

cast<VectorType>(Ty), std::nullopt, CostKind, 0);

Updated, thanks!

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

fhahn · 2024-08-27T10:57:38Z

With the change it should also be possible to remove VPWidenMemoryRecipe from getInstructionForCost in VPlan.cpp

Still missing?

ElvisWang123 · 2024-08-27T11:07:19Z

When removing VPWidenMemoryRecipe from the getInstructionForCost() will cause test fails.
Because in the VPRecipeBase::cost() , there is a check for override the instruction cost by option needs the return from getInstructionForCost() .

if (UI && ForceTargetInstructionCost.getNumOccurrences() > 0 &&
      RecipeCost.isValid())
    RecipeCost = InstructionCost(ForceTargetInstructionCost);

Should we just remove the check of UI ?

arcbbb · 2024-08-28T05:44:53Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

not sure if this is still NFC but we can skip the shuffle cost if it is a store and the store value is uniform

ElvisWang123 · 2024-08-28T08:03:17Z

With the change it should also be possible to remove VPWidenMemoryRecipe from getInstructionForCost in VPlan.cpp

Still missing?

Remove VPWidenMemoryRecipe from getInstructionForCost will cause a test fail due to different VF between legacy model.
Maybe leave a TODO behind to revisit it when we don't need to compare to the legacy model?

fhahn · 2024-08-30T12:48:18Z

With the change it should also be possible to remove VPWidenMemoryRecipe from getInstructionForCost in VPlan.cpp

Still missing?

Remove VPWidenMemoryRecipe from getInstructionForCost will cause a test fail due to different VF between legacy model. Maybe leave a TODO behind to revisit it when we don't need to compare to the legacy model?

yes that would be great, together with an explanation that VPWidenMemoryRecipe case in getInstructionForCost still being needed for force-target-instruction handling.

ElvisWang123 · 2024-09-02T00:12:27Z

With the change it should also be possible to remove VPWidenMemoryRecipe from getInstructionForCost in VPlan.cpp

Still missing?

Remove VPWidenMemoryRecipe from getInstructionForCost will cause a test fail due to different VF between legacy model. Maybe leave a TODO behind to revisit it when we don't need to compare to the legacy model?

yes that would be great, together with an explanation that VPWidenMemoryRecipe case in getInstructionForCost still being needed for force-target-instruction handling.

Done, thanks!

In this patch, we add the `computeCost()` function for `VPWidenMemoryRecipe`.

fhahn

LGTM,thanks!

fhahn · 2024-09-03T20:09:06Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

Suggested change

// the latecy model.

// the legacy model.

?

ElvisWang123 requested review from fhahn and arcbbb August 22, 2024 04:53

llvmbot added vectorizers llvm:transforms labels Aug 22, 2024

fhahn requested a review from ayalz August 26, 2024 09:17

fhahn reviewed Aug 26, 2024

View reviewed changes

ElvisWang123 changed the title ~~[VPlan] Add cost for VPWidenMemoryRecipe~~ [VPlan][NFC] Add cost for VPWidenMemoryRecipe. Aug 26, 2024

fhahn reviewed Aug 26, 2024

View reviewed changes

ElvisWang123 force-pushed the vp-widen-ls-cost branch from d11c570 to 53972d5 Compare August 27, 2024 02:25

fhahn reviewed Aug 27, 2024

View reviewed changes

arcbbb reviewed Aug 28, 2024

View reviewed changes

ElvisWang123 added 4 commits September 2, 2024 18:26

[VPlan] Add cost for VPWidenMemoryRecipe

2478741

In this patch, we add the `computeCost()` function for `VPWidenMemoryRecipe`.

Address comments

920c948

Address comments

b1180dc

Add TODO for getInstrForCost()

627c7ee

fhahn approved these changes Sep 3, 2024

View reviewed changes

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp Outdated

Copy link

Contributor

fhahn Sep 3, 2024

Choose a reason for hiding this comment

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

Suggested change

// the latecy model.

// the legacy model.

?

Fix typo

6204144

ElvisWang123 force-pushed the vp-widen-ls-cost branch from 8e7c55c to 6204144 Compare September 3, 2024 21:30

ElvisWang123 merged commit ed220e1 into llvm:main Sep 4, 2024
8 checks passed

[VPlan][NFC] Add cost for VPWidenMemoryRecipe. #105614

[VPlan][NFC] Add cost for VPWidenMemoryRecipe. #105614

Uh oh!

Conversation

ElvisWang123 commented Aug 22, 2024

Uh oh!

llvmbot commented Aug 22, 2024

Uh oh!

fhahn left a comment

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!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

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!

Uh oh!

fhahn commented Aug 27, 2024

Uh oh!

ElvisWang123 commented Aug 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElvisWang123 commented Aug 28, 2024

Uh oh!

fhahn commented Aug 30, 2024

Uh oh!

ElvisWang123 commented Sep 2, 2024

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[VPlan][NFC] Add cost for `VPWidenMemoryRecipe`. #105614

[VPlan][NFC] Add cost for `VPWidenMemoryRecipe`. #105614