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

[Kerry McLaughlin via Phabricator via llvm-commits]

kmclaughlin added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:333
 
+cl::opt<bool> EnableStrictReductions(
+    "enable-strict-reductions", cl::init(false), cl::Hidden,
----------------
fhahn wrote:
> 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?
Hi @fhahn, I think there is more testing needed before we can be confident in removing the `-enable-strict-reductions` flag, for instance adding more tests for scalable types and running LNT with the flag enabled. After this, the plan is to remove the option (after making any necessary changes to the cost model to decide if using in-order reductions is beneficial).


================
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);
----------------
fhahn wrote:
> 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?
The `@fadd_strict_unroll` test should have been testing this part of fixReduction(), but there was a mistake in the CHECK lines I added for the vector.reduce.fadd intrinsics. I've pushed rG93f54fae9dda to address this, and also created D100570 to try and prevent multiple unused Phis from being generated to begin with.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98435/new/

https://reviews.llvm.org/D98435