[PATCH] D98435: [LoopVectorize] Add strict in-order reduction support for fixed-width vectorization
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 8 04:37:00 PDT 2021
fhahn added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:333
+cl::opt<bool> EnableStrictReductions(
+ "enable-strict-reductions", cl::init(false), cl::Hidden,
----------------
What's the plan for this option going forward? Would it be safe to remove it and let the cost model deal with deciding whether it is worth to vectorize with strict reductions only?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4260
+ if (IsInLoopReductionPhi && useOrderedReductions(RdxDesc) &&
+ State.VF.getKnownMinValue() > 1)
+ Val = State.get(State.Plan->getVPValue(LoopVal), UF - 1);
----------------
kmclaughlin wrote:
> david-arm wrote:
> > dmgreen wrote:
> > > Can this use isScalar(), or is it to handle scalable single items too?
> > >
> > > We generate multiple phi's, but only use the first one? The others get DCE'd?
> > Yes, I think in this case we can just use
> >
> > State.VF.isVector()
> >
> > since that also covers the case when VF=(1,scalable)
> Yes, we generate multiple phis and the unused phis are removed by InstCombine.
>
> Changed this to use `State.VF.isVector()` as suggested above.
I think there's test coverage missing for this code. All tests pass if this change gets removed. @kmclaughlin could you add a test?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98435/new/
https://reviews.llvm.org/D98435
More information about the llvm-commits
mailing list