-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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.
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
[VPlan] Implement VPReductionRecipe::computeCost(). NFC #107790
[VPlan] Implement VPReductionRecipe::computeCost(). NFC #107790
Conversation
@llvm/pr-subscribers-llvm-transforms Author: Elvis Wang (ElvisWang123) ChangesImplementation of Full diff: https://github.com/llvm/llvm-project/pull/107790.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index bd71dbffa929e7..68f8ea3dea0db3 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -2242,6 +2242,10 @@ class VPReductionRecipe : public VPSingleDefRecipe {
/// Generate the reduction in the loop
void execute(VPTransformState &State) override;
+ /// Return the cost of VPReductionRecipe.
+ 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,
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index 3d08e3cefbf633..12b6d2c7d06dd1 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -1897,6 +1897,30 @@ void VPReductionEVLRecipe::execute(VPTransformState &State) {
State.set(this, NewRed, 0, /*IsScalar*/ true);
}
+InstructionCost VPReductionRecipe::computeCost(ElementCount VF,
+ VPCostContext &Ctx) const {
+ RecurKind RdxKind = RdxDesc.getRecurrenceKind();
+ Type *ElementTy = RdxDesc.getRecurrenceType();
+ auto *VectorTy = dyn_cast<VectorType>(ToVectorTy(ElementTy, VF));
+ TTI::TargetCostKind CostKind = TTI::TCK_RecipThroughput;
+ unsigned Opcode = RdxDesc.getOpcode();
+
+ if (VectorTy == nullptr)
+ return InstructionCost::getInvalid();
+
+ // Cost = Reduction cost + BinOp cost
+ InstructionCost Cost =
+ Ctx.TTI.getArithmeticInstrCost(Opcode, ElementTy, CostKind);
+ if (RecurrenceDescriptor::isMinMaxRecurrenceKind(RdxKind)) {
+ Intrinsic::ID Id = getMinMaxReductionIntrinsicOp(RdxKind);
+ return Cost + Ctx.TTI.getMinMaxReductionCost(
+ Id, VectorTy, RdxDesc.getFastMathFlags(), CostKind);
+ }
+
+ return Cost + Ctx.TTI.getArithmeticReductionCost(
+ Opcode, VectorTy, RdxDesc.getFastMathFlags(), CostKind);
+}
+
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
void VPReductionRecipe::print(raw_ostream &O, const Twine &Indent,
VPSlotTracker &SlotTracker) const {
|
Gentle ping! |
a42e18f
to
c5fb73a
Compare
VPCostContext &Ctx) const { | ||
RecurKind RdxKind = RdxDesc.getRecurrenceKind(); | ||
Type *ElementTy = RdxDesc.getRecurrenceType(); | ||
auto *VectorTy = cast<VectorType>(ToVectorTy(ElementTy, VF)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
infer the type for the recipe instead and assert that the type matches RdxDesc.getRecurrenceType()
to ensure consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, thanks.
@@ -1999,6 +1999,27 @@ void VPReductionEVLRecipe::execute(VPTransformState &State) { | |||
State.set(this, NewRed, 0, /*IsScalar*/ true); | |||
} | |||
|
|||
InstructionCost VPReductionRecipe::computeCost(ElementCount VF, | |||
VPCostContext &Ctx) const { | |||
RecurKind RdxKind = RdxDesc.getRecurrenceKind(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This computes the cost for non-in loop and non-any-of reductions, correct? Would be good to add an assert an explanation why (for those the cost needs to be pre-computed at the moment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will add an assertion to check it is not an any-of reduction.
I tried to compute the cost of in-loop reductions by the vplan-based cost model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be better to add support for in-loop reductions separately, as it adds extra complexity and may introduce new divergences
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will open another PR to address in-loop reduction.
faa86e5
to
dc5b400
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Would be good to update the description to clarify that this is for non-in-loop reductions and non any-of?
// TODO: Support any-of reduction and in-loop reduction | ||
assert(!RecurrenceDescriptor::isAnyOfRecurrenceKind(RdxKind) && | ||
"Not support any-of reduction in VPlan-based cost model currently."); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also assert that it's not an in-loop reduction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a new function isInLoopReduction()
to check if the instruction is in the inLoopReduction chain.
dc5b400
to
20e90e7
Compare
ForceTargetInstructionCost.getNumOccurrences() > 0) && | ||
"Any-of reduction not implemented in VPlan-based cost model currently."); | ||
assert( | ||
(!Ctx.isInLoopReduction(getUnderlyingInstr(), VF, VectorTy) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to introduce a lookup via the legacy cost model here? IIRC the first op of the reduction recipe is the reduction PHI recipe, which has an in-loop flag. Can we check that instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks.
Implementation of `computeCost()` function for `VPReductionRecipe`.
562ae34
to
7acd294
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Spotted one more typo, please feel free to land with if fixed
"In-loop reduction not implemented in VPlan-based cost model currently."); | ||
|
||
assert(ElementTy->getTypeID() == RdxDesc.getRecurrenceType()->getTypeID() && | ||
"Infered type and recurrence type mismatch."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Infered type and recurrence type mismatch."); | |
"Inferred type and recurrence type mismatch."); |
Implementation of `computeCost()` function for `VPReductionRecipe`. Note that `in-loop` and `any-of` reductions are not supported by VPlan-based cost model currently.
Implementation of `computeCost()` function for `VPReductionRecipe`. Note that `in-loop` and `any-of` reductions are not supported by VPlan-based cost model currently.
Implementation of `computeCost()` function for `VPReductionRecipe`. Note that `in-loop` and `any-of` reductions are not supported by VPlan-based cost model currently.
Implementation of
computeCost()
function forVPReductionRecipe
.Note that
in-loop
andany-of
reductions are not supported by VPlan-based cost model currently.