-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[VPlan] Add VPExpressionRecipe, replacing extended reduction recipes. #144281
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
Changes from 17 commits
736357e
3c3f9e4
1b7bf4b
e8dc289
9688a79
741ba42
ac1a2dc
3be870c
d87843c
86b1216
285ac34
1d717d2
201c5d7
1bae34e
42359e5
6b85231
3908cd6
c70bba4
4bfef50
4ed3f43
a661de8
8af8f33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -525,14 +525,13 @@ class VPSingleDefRecipe : public VPRecipeBase, public VPValue { | |||||||||||
|
|
||||||||||||
| static inline bool classof(const VPRecipeBase *R) { | ||||||||||||
| switch (R->getVPDefID()) { | ||||||||||||
| case VPRecipeBase::VPBundleSC: | ||||||||||||
| case VPRecipeBase::VPDerivedIVSC: | ||||||||||||
| case VPRecipeBase::VPEVLBasedIVPHISC: | ||||||||||||
| case VPRecipeBase::VPExpandSCEVSC: | ||||||||||||
| case VPRecipeBase::VPInstructionSC: | ||||||||||||
| case VPRecipeBase::VPReductionEVLSC: | ||||||||||||
| case VPRecipeBase::VPReductionSC: | ||||||||||||
| case VPRecipeBase::VPMulAccumulateReductionSC: | ||||||||||||
| case VPRecipeBase::VPExtendedReductionSC: | ||||||||||||
| case VPRecipeBase::VPReplicateSC: | ||||||||||||
| case VPRecipeBase::VPScalarIVStepsSC: | ||||||||||||
| case VPRecipeBase::VPVectorPointerSC: | ||||||||||||
|
|
@@ -852,9 +851,7 @@ struct VPRecipeWithIRFlags : public VPSingleDefRecipe, public VPIRFlags { | |||||||||||
| R->getVPDefID() == VPRecipeBase::VPReductionEVLSC || | ||||||||||||
| R->getVPDefID() == VPRecipeBase::VPReplicateSC || | ||||||||||||
| R->getVPDefID() == VPRecipeBase::VPVectorEndPointerSC || | ||||||||||||
| R->getVPDefID() == VPRecipeBase::VPVectorPointerSC || | ||||||||||||
| R->getVPDefID() == VPRecipeBase::VPExtendedReductionSC || | ||||||||||||
| R->getVPDefID() == VPRecipeBase::VPMulAccumulateReductionSC; | ||||||||||||
| R->getVPDefID() == VPRecipeBase::VPVectorPointerSC; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| static inline bool classof(const VPUser *U) { | ||||||||||||
|
|
@@ -2440,28 +2437,6 @@ class VPReductionRecipe : public VPRecipeWithIRFlags { | |||||||||||
| setUnderlyingValue(I); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// For VPExtendedReductionRecipe. | ||||||||||||
| /// Note that the debug location is from the extend. | ||||||||||||
| VPReductionRecipe(const unsigned char SC, const RecurKind RdxKind, | ||||||||||||
| ArrayRef<VPValue *> Operands, VPValue *CondOp, | ||||||||||||
| bool IsOrdered, DebugLoc DL) | ||||||||||||
| : VPRecipeWithIRFlags(SC, Operands, DL), RdxKind(RdxKind), | ||||||||||||
| IsOrdered(IsOrdered), IsConditional(CondOp) { | ||||||||||||
| if (CondOp) | ||||||||||||
| addOperand(CondOp); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// For VPMulAccumulateReductionRecipe. | ||||||||||||
| /// Note that the NUW/NSW flags and the debug location are from the Mul. | ||||||||||||
| VPReductionRecipe(const unsigned char SC, const RecurKind RdxKind, | ||||||||||||
| ArrayRef<VPValue *> Operands, VPValue *CondOp, | ||||||||||||
| bool IsOrdered, WrapFlagsTy WrapFlags, DebugLoc DL) | ||||||||||||
| : VPRecipeWithIRFlags(SC, Operands, WrapFlags, DL), RdxKind(RdxKind), | ||||||||||||
| IsOrdered(IsOrdered), IsConditional(CondOp) { | ||||||||||||
| if (CondOp) | ||||||||||||
| addOperand(CondOp); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| public: | ||||||||||||
| VPReductionRecipe(RecurKind RdxKind, FastMathFlags FMFs, Instruction *I, | ||||||||||||
| VPValue *ChainOp, VPValue *VecOp, VPValue *CondOp, | ||||||||||||
|
|
@@ -2487,9 +2462,7 @@ class VPReductionRecipe : public VPRecipeWithIRFlags { | |||||||||||
|
|
||||||||||||
| static inline bool classof(const VPRecipeBase *R) { | ||||||||||||
| return R->getVPDefID() == VPRecipeBase::VPReductionSC || | ||||||||||||
| R->getVPDefID() == VPRecipeBase::VPReductionEVLSC || | ||||||||||||
| R->getVPDefID() == VPRecipeBase::VPExtendedReductionSC || | ||||||||||||
| R->getVPDefID() == VPRecipeBase::VPMulAccumulateReductionSC; | ||||||||||||
| R->getVPDefID() == VPRecipeBase::VPReductionEVLSC; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| static inline bool classof(const VPUser *U) { | ||||||||||||
|
|
@@ -2628,190 +2601,6 @@ class VPReductionEVLRecipe : public VPReductionRecipe { | |||||||||||
| } | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| /// A recipe to represent inloop extended reduction operations, performing a | ||||||||||||
| /// reduction on a extended vector operand into a scalar value, and adding the | ||||||||||||
| /// result to a chain. This recipe is abstract and needs to be lowered to | ||||||||||||
| /// concrete recipes before codegen. The operands are {ChainOp, VecOp, | ||||||||||||
| /// [Condition]}. | ||||||||||||
| class VPExtendedReductionRecipe : public VPReductionRecipe { | ||||||||||||
| /// Opcode of the extend for VecOp. | ||||||||||||
| Instruction::CastOps ExtOp; | ||||||||||||
|
|
||||||||||||
| /// The scalar type after extending. | ||||||||||||
| Type *ResultTy; | ||||||||||||
|
|
||||||||||||
| /// For cloning VPExtendedReductionRecipe. | ||||||||||||
| VPExtendedReductionRecipe(VPExtendedReductionRecipe *ExtRed) | ||||||||||||
| : VPReductionRecipe( | ||||||||||||
| VPDef::VPExtendedReductionSC, ExtRed->getRecurrenceKind(), | ||||||||||||
| {ExtRed->getChainOp(), ExtRed->getVecOp()}, ExtRed->getCondOp(), | ||||||||||||
| ExtRed->isOrdered(), ExtRed->getDebugLoc()), | ||||||||||||
| ExtOp(ExtRed->getExtOpcode()), ResultTy(ExtRed->getResultType()) { | ||||||||||||
| transferFlags(*ExtRed); | ||||||||||||
| setUnderlyingValue(ExtRed->getUnderlyingValue()); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| public: | ||||||||||||
| VPExtendedReductionRecipe(VPReductionRecipe *R, VPWidenCastRecipe *Ext) | ||||||||||||
| : VPReductionRecipe(VPDef::VPExtendedReductionSC, R->getRecurrenceKind(), | ||||||||||||
| {R->getChainOp(), Ext->getOperand(0)}, R->getCondOp(), | ||||||||||||
| R->isOrdered(), Ext->getDebugLoc()), | ||||||||||||
| ExtOp(Ext->getOpcode()), ResultTy(Ext->getResultType()) { | ||||||||||||
| assert((ExtOp == Instruction::CastOps::ZExt || | ||||||||||||
| ExtOp == Instruction::CastOps::SExt) && | ||||||||||||
| "VPExtendedReductionRecipe only supports zext and sext."); | ||||||||||||
|
|
||||||||||||
| transferFlags(*Ext); | ||||||||||||
| setUnderlyingValue(R->getUnderlyingValue()); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| ~VPExtendedReductionRecipe() override = default; | ||||||||||||
|
|
||||||||||||
| VPExtendedReductionRecipe *clone() override { | ||||||||||||
| return new VPExtendedReductionRecipe(this); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| VP_CLASSOF_IMPL(VPDef::VPExtendedReductionSC); | ||||||||||||
|
|
||||||||||||
| void execute(VPTransformState &State) override { | ||||||||||||
| llvm_unreachable("VPExtendedReductionRecipe should be transform to " | ||||||||||||
| "VPExtendedRecipe + VPReductionRecipe before execution."); | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| /// Return the cost of VPExtendedReductionRecipe. | ||||||||||||
| 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, | ||||||||||||
| VPSlotTracker &SlotTracker) const override; | ||||||||||||
| #endif | ||||||||||||
|
|
||||||||||||
| /// The scalar type after extending. | ||||||||||||
| Type *getResultType() const { return ResultTy; } | ||||||||||||
|
|
||||||||||||
| /// Is the extend ZExt? | ||||||||||||
| bool isZExt() const { return getExtOpcode() == Instruction::ZExt; } | ||||||||||||
|
|
||||||||||||
| /// Get the opcode of the extend for VecOp. | ||||||||||||
| Instruction::CastOps getExtOpcode() const { return ExtOp; } | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| /// A recipe to represent inloop MulAccumulateReduction operations, multiplying | ||||||||||||
| /// the vector operands (which may be extended), performing a reduction.add on | ||||||||||||
| /// the result, and adding the scalar result to a chain. This recipe is abstract | ||||||||||||
| /// and needs to be lowered to concrete recipes before codegen. The operands are | ||||||||||||
| /// {ChainOp, VecOp1, VecOp2, [Condition]}. | ||||||||||||
| class VPMulAccumulateReductionRecipe : public VPReductionRecipe { | ||||||||||||
| /// Opcode of the extend for VecOp1 and VecOp2. | ||||||||||||
| Instruction::CastOps ExtOp; | ||||||||||||
|
|
||||||||||||
| /// Non-neg flag of the extend recipe. | ||||||||||||
| bool IsNonNeg = false; | ||||||||||||
|
|
||||||||||||
| /// The scalar type after extending. | ||||||||||||
| Type *ResultTy = nullptr; | ||||||||||||
|
|
||||||||||||
| /// For cloning VPMulAccumulateReductionRecipe. | ||||||||||||
| VPMulAccumulateReductionRecipe(VPMulAccumulateReductionRecipe *MulAcc) | ||||||||||||
| : VPReductionRecipe( | ||||||||||||
| VPDef::VPMulAccumulateReductionSC, MulAcc->getRecurrenceKind(), | ||||||||||||
| {MulAcc->getChainOp(), MulAcc->getVecOp0(), MulAcc->getVecOp1()}, | ||||||||||||
| MulAcc->getCondOp(), MulAcc->isOrdered(), | ||||||||||||
| WrapFlagsTy(MulAcc->hasNoUnsignedWrap(), MulAcc->hasNoSignedWrap()), | ||||||||||||
| MulAcc->getDebugLoc()), | ||||||||||||
| ExtOp(MulAcc->getExtOpcode()), IsNonNeg(MulAcc->isNonNeg()), | ||||||||||||
| ResultTy(MulAcc->getResultType()) { | ||||||||||||
| transferFlags(*MulAcc); | ||||||||||||
| setUnderlyingValue(MulAcc->getUnderlyingValue()); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| public: | ||||||||||||
| VPMulAccumulateReductionRecipe(VPReductionRecipe *R, VPWidenRecipe *Mul, | ||||||||||||
| VPWidenCastRecipe *Ext0, | ||||||||||||
| VPWidenCastRecipe *Ext1, Type *ResultTy) | ||||||||||||
| : VPReductionRecipe( | ||||||||||||
| VPDef::VPMulAccumulateReductionSC, R->getRecurrenceKind(), | ||||||||||||
| {R->getChainOp(), Ext0->getOperand(0), Ext1->getOperand(0)}, | ||||||||||||
| R->getCondOp(), R->isOrdered(), | ||||||||||||
| WrapFlagsTy(Mul->hasNoUnsignedWrap(), Mul->hasNoSignedWrap()), | ||||||||||||
| R->getDebugLoc()), | ||||||||||||
| ExtOp(Ext0->getOpcode()), ResultTy(ResultTy) { | ||||||||||||
| assert(RecurrenceDescriptor::getOpcode(getRecurrenceKind()) == | ||||||||||||
| Instruction::Add && | ||||||||||||
| "The reduction instruction in MulAccumulateteReductionRecipe must " | ||||||||||||
| "be Add"); | ||||||||||||
| assert((ExtOp == Instruction::CastOps::ZExt || | ||||||||||||
| ExtOp == Instruction::CastOps::SExt) && | ||||||||||||
| "VPMulAccumulateReductionRecipe only supports zext and sext."); | ||||||||||||
| setUnderlyingValue(R->getUnderlyingValue()); | ||||||||||||
| // Only set the non-negative flag if the original recipe contains. | ||||||||||||
| if (Ext0->hasNonNegFlag()) | ||||||||||||
| IsNonNeg = Ext0->isNonNeg(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| VPMulAccumulateReductionRecipe(VPReductionRecipe *R, VPWidenRecipe *Mul, | ||||||||||||
| Type *ResultTy) | ||||||||||||
| : VPReductionRecipe( | ||||||||||||
| VPDef::VPMulAccumulateReductionSC, R->getRecurrenceKind(), | ||||||||||||
| {R->getChainOp(), Mul->getOperand(0), Mul->getOperand(1)}, | ||||||||||||
| R->getCondOp(), R->isOrdered(), | ||||||||||||
| WrapFlagsTy(Mul->hasNoUnsignedWrap(), Mul->hasNoSignedWrap()), | ||||||||||||
| R->getDebugLoc()), | ||||||||||||
| ExtOp(Instruction::CastOps::CastOpsEnd), ResultTy(ResultTy) { | ||||||||||||
| assert(RecurrenceDescriptor::getOpcode(getRecurrenceKind()) == | ||||||||||||
| Instruction::Add && | ||||||||||||
| "The reduction instruction in MulAccumulateReductionRecipe must be " | ||||||||||||
| "Add"); | ||||||||||||
| setUnderlyingValue(R->getUnderlyingValue()); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| ~VPMulAccumulateReductionRecipe() override = default; | ||||||||||||
|
|
||||||||||||
| VPMulAccumulateReductionRecipe *clone() override { | ||||||||||||
| return new VPMulAccumulateReductionRecipe(this); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| VP_CLASSOF_IMPL(VPDef::VPMulAccumulateReductionSC); | ||||||||||||
|
|
||||||||||||
| void execute(VPTransformState &State) override { | ||||||||||||
| llvm_unreachable("VPMulAccumulateReductionRecipe should transform to " | ||||||||||||
| "VPWidenCastRecipe + " | ||||||||||||
| "VPWidenRecipe + VPReductionRecipe before execution"); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// Return the cost of VPMulAccumulateReductionRecipe. | ||||||||||||
| 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, | ||||||||||||
| VPSlotTracker &SlotTracker) const override; | ||||||||||||
| #endif | ||||||||||||
|
|
||||||||||||
| Type *getResultType() const { return ResultTy; } | ||||||||||||
|
|
||||||||||||
| /// The first vector value to be extended and reduced. | ||||||||||||
| VPValue *getVecOp0() const { return getOperand(1); } | ||||||||||||
|
|
||||||||||||
| /// The second vector value to be extended and reduced. | ||||||||||||
| VPValue *getVecOp1() const { return getOperand(2); } | ||||||||||||
|
|
||||||||||||
| /// Return true if this recipe contains extended operands. | ||||||||||||
| bool isExtended() const { return ExtOp != Instruction::CastOps::CastOpsEnd; } | ||||||||||||
|
|
||||||||||||
| /// Return the opcode of the extends for the operands. | ||||||||||||
| Instruction::CastOps getExtOpcode() const { return ExtOp; } | ||||||||||||
|
|
||||||||||||
| /// Return if the operands are zero-extended. | ||||||||||||
| bool isZExt() const { return ExtOp == Instruction::CastOps::ZExt; } | ||||||||||||
|
|
||||||||||||
| /// Return true if the operand extends have the non-negative flag. | ||||||||||||
| bool isNonNeg() const { return IsNonNeg; } | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| /// VPReplicateRecipe replicates a given instruction producing multiple scalar | ||||||||||||
| /// copies of the original scalar type, one per lane, instead of producing a | ||||||||||||
| /// single copy of widened type for all lanes. If the instruction is known to be | ||||||||||||
|
|
@@ -2930,6 +2719,122 @@ class VPBranchOnMaskRecipe : public VPRecipeBase { | |||||||||||
| } | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| /// A recipe to combine multiple recipes into a 'bundle' recipe, which should be | ||||||||||||
|
||||||||||||
| /// A recipe to combine multiple recipes into a 'bundle' recipe, which should be | |
| /// A recipe to combine multiple 'bundled' recipes into one 'bundle' recipe, which should be |
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.
The recipes aren't bundled until they're in the recipe so I don't think saying "multiple 'bundled' recipes" is correct. The original comment is correct in that the recipes are separate until they're bundled into this class.
Outdated
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.
| /// Recipes bundled together in this VPSingleDefBundleRecipe. | |
| /// Recipes bundled together in this VPSingleDefBundleRecipe. | |
| /// Note that recipes may appear multiple times, e.g., when both extends of a ExtMulAccumulateReduction are the same. |
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.
They are unique in the latest version.
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.
Ok, good, worth noting?
Outdated
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.
One could bundle multi-def recipes, perhaps excluding the last one if the bundle recipe is to have a single def.
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.
Yep, this could be extended in the future. I left it as VPSingleDefRecipe for now initially, as it simplifies some of the bundle/unbundle logic
Outdated
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.
These are placeholder VPValues defined temporarily by the bundle recipe itself to feed its bundled recipes. Note that the VPBundleRecipe, being a single def VPValue, feeds out-of bundle recipes itself.
Outdated
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.
| /// reduction on a extended vector operand into a scalar value, and adding | |
| /// reduction on an extended vector operand into a scalar value, and adding |
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.
That should be fixed in the latest/landed version I think
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.
So should this be
| ExtendedReduction, | |
| ExtAccReduction, |
?
Outdated
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.
| ExtMulAccumulateReduction, | |
| ExtMulAccReduction, |
(also aligning with TTI API)
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.
Done thanks
Outdated
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.
| MulAccumulateReduction, | |
| MulAccReduction, |
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.
Done thanks
Outdated
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.
Better placed next to the enum definition above, documenting each?
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.
Moved the definition down here, thanks
Outdated
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.
Worth documenting, especially \p Operands - perhaps better called CloneOperands, perhaps defined to be optional, passed only to save time? Does it really save anything.
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.
Operands is gone now, moved logic from bundle() directly to the constructor, thanks
Outdated
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.
Worth asserting that Red is an add reduction, aka accumulation?
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.
Currently VPReductionRecipe will always accumulate/add the result to a reduction chain.
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.
Then perhaps better rename it VPAccumulationRecipe, or VPInLoopAccRecipe, independently.
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.
There will be a use case soon that won't be an in-loop reduction so it's probably worth keeping the more generally name.
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.
Ok, if VPReductionRecipe will still be confined to add reductions, in loop or not, better rename it VPAccumulationRecipe? And/or add asserts that expressions named and documented as doing in-loop accumulations use suitable reduction recipes.
Outdated
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.
Worth asserting that Red is an add reduction, aka accumulation?
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.
Currently VPReductionRecipe will always accumulate/add the result to a reduction chain.
Outdated
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.
Same recipe may be bundled twice?
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.
Yes, one example would be a MulAccumulateReduction with both extends are the same
Outdated
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.
Should the recipe unbundle() itself now, upon destruction, leaving its bundled recipes to be destructed by their basic block, or simply assert that this has already taken place?
| SmallPtrSet<VPRecipeBase *, 4> Seen; | |
| for (auto *R : reverse(BundledRecipes)) | |
| if (Seen.insert(R).second) | |
| delete R; | |
| assert(BundledRecipes.empty() && "must unbundle before destruction"); |
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.
Unfortunately the recipe won't get unbundled in all VPlans that won't get selected/executed and unbundle will insert the recipes before the current one, so won't get removed. I left it as-is for now.
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.
The latest version doesn't need to use a set, as the recipes are unique'd
Outdated
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.
| VPValue *getTypeVPValue() const { | |
| VPValue *getOperandOfResultType() const { |
?
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.
Updated, thanks
Outdated
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.
Last operand unless it's a mask in which case penultimate operand? Should the bundled recipe itself support isConditional()?
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.
I am not sure, as this is the only place where it is needed. Left as-is for now.
Outdated
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.
Docs needed.
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.
Added thanks.
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.
to be consistent, and place below following lex order.
But would
VPExpressionRecipebe better - conveying the notion that (a) it produces a single value (b) out of multiple simpler components - which are potentially side-effect-free(?) and potentially (sub)expressions themselves(?). I.e., a bundle recipe can potentially be bundled inside another bundle recipe, unbundle() can be recursive. Worth commenting that this may be possible but that currently bundle recipes are restricted to flat/single-level bundling.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.
Another option is
VPIdiomRecipeThere 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, updated to VPExpressionRecipe, which is also slightly more compact. Tried to update various places refering to bundle as appropriate, just keeping
unbundle.