[PATCH] D155845: [VPlan] Fix in-loop reduction chains using VPlan def-use chains (NFCI)
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 2 04:26:58 PDT 2023
fhahn added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9165-9172
+ SetVector<VPRecipeBase *> Worklist;
+ Worklist.insert(PhiR);
+
+ RecurKind Kind = RdxDesc.getRecurrenceKind();
+ assert(!RecurrenceDescriptor::isSelectCmpRecurrenceKind(Kind) &&
+ "select/cmp reductions are not allowed for in-loop reductions");
+
----------------
Ayal wrote:
>
Reordered, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9181
+ "recipes must define exactly one result value");
+ ;
+ Worklist.insert(UserRecipe);
----------------
Ayal wrote:
> ?
Removed, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9187
+ // Visit operation "Links" along the reduction chain top-down starting from
+ // the phi until LoopExitValue. We keep a track of the previous item
+ // (PreviousLink) to tell which of the two operands of a Link will remain
----------------
Ayal wrote:
>
Fixed, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9189
+ // (PreviousLink) to tell which of the two operands of a Link will remain
+ // scalar and which will be reduced. For minmax, Link will be the select
+ // instructions.
----------------
Ayal wrote:
>
added, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9207
+ if (IsFMulAdd) {
+ assert((MinVF.isScalar() || isa<VPWidenCallRecipe>(CurrentLink)) &&
+ CurrentLink->getOperand(2) == PreviousLinkV &&
----------------
Ayal wrote:
> If VF=1 CurrentLink should be a Replicate instead of a Widen(Call) recipe?
>
> Better place the above assert(RecurrenceDescriptor::isFMulAddIntrinsic(CurrentLinkI)) here?
Moved assert and refined condition as suggested, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9233
// If the instruction is a call to the llvm.fmuladd intrinsic then we
// need to create an fmul recipe to use as the vector operand for the
// fadd reduction.
----------------
Ayal wrote:
>
Adjusted, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9244
+ VPRecipeBase *CompareRecipe =
+ CurrentLink->getOperand(0)->getDefiningRecipe();
+
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Could we use PreviousLink serve as/instead of CompareRecipe?
> > Unfortunately I don't think so, but we can let dead-recipe removal take care of this removal. I removed the code.
> If PreviousLink is set to Cmp before early-continuing above?
>
> OTOH, dead-recipe removal could also reclaim CurrentLink.
Yep, updated, thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155845/new/
https://reviews.llvm.org/D155845
More information about the llvm-commits
mailing list