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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:207
+  auto *RHS = Exit->getOperand(1);
+  IsOrdered = IsOrdered && ((LHS == Phi) || (RHS == Phi));
+
----------------
IsOrdered &= (LHS == Phi) || (RHS == Phi);


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:204
+  // If this comes from a Phi node, look through it
+  if (auto *EIP = dyn_cast<PHINode>(Exit)) {
+    if (EIP->getNumIncomingValues() != 2)
----------------
kmclaughlin wrote:
> dmgreen wrote:
> > I think this may need to check all branches of the phi, if I'm understanding what this is doing exactly. They could have different instructions down each path with different flags (although that would be quite rare I suspect).
> > 
> > There might also be selects, if this is matching on if-block phis. I'm not sure if that is handled in AddReductionVar though.
> I realised whilst trying to add a better test than `@fadd_conditional_rdx` for conditional reductions that with the tests I have written this part of the function is redundant.
> I've removed it from this patch for now since I don't think I can write any tests for it yet - I believe in addition to looking through Phi nodes here and checking all branches, I would also need to change `getReductionOpChain` to look through Phis and Selects in order to vectorize with inloop reductions, is this correct?
Oh yeah, that sounds like it might be correct. We handle predicated reductions, but were more interested in tail-folding for MVE. Predication through a select/if phi might well need more work.


================
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);
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4361
   {
-    // Floating-point operations should have some FMF to enable the reduction.
-    IRBuilderBase::FastMathFlagGuard FMFG(Builder);
-    Builder.setFastMathFlags(RdxDesc.getFastMathFlags());
-    for (unsigned Part = 1; Part < UF; ++Part) {
-      Value *RdxPart = State.get(LoopExitInstDef, Part);
-      if (Op != Instruction::ICmp && Op != Instruction::FCmp) {
-        ReducedPartRdx = Builder.CreateBinOp(
-            (Instruction::BinaryOps)Op, RdxPart, ReducedPartRdx, "bin.rdx");
-      } else {
-        ReducedPartRdx = createMinMaxOp(Builder, RK, ReducedPartRdx, RdxPart);
+    if (Cost->isInLoopReduction(Phi) && useOrderedReductions(RdxDesc))
+      ReducedPartRdx = State.get(LoopExitInstDef, UF - 1);
----------------
kmclaughlin wrote:
> dmgreen wrote:
> > Cost->isInLoopReduction(Phi) -> IsInLoopReductionPhi
> > 
> > Does this work in-order for UF > 1? I feel like the order of the adds will be changed, so that it's no-longer in-order.
> > 
> > If not, is this code needed, if it can only be using UF == 1 and ReducedPartRdx is set to State.get(LoopExitInstDef, 0) above.
> The changes I made to `VPReductionRecipe::execute` should ensure that ordering is enforced when the UF > 1, through the way the chain is constructed for each unrolled part. I was missing a change to fixReduction to correctly set the incoming values of Phi for each unrolled part in my first patch, however I believe this is fixed now and the test for unrolling (`@fadd_strict_unroll`) has been updated accordingly.
Yeah, looks OK I think.  All the back-to-back operations might be slow, forced down a critical path, but the order seems good.


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

https://reviews.llvm.org/D98435