[PATCH] D98435: [LoopVectorize] Add strict in-order reduction support for fixed-width vectorization

[Florian Hahn via Phabricator via llvm-commits]

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