[PATCH] D100102: [VPlan] Iterate over phi recipes to detect reductions to fix.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9404
   // Adjust the recipes for any inloop reductions.
-  adjustRecipesForInLoopReductions(Plan, RecipeBuilder, Range.Start);
-
-  // Finally, if tail is folded by masking, introduce selects between the phi
-  // and the live-out instruction of each reduction, at the end of the latch.
-  if (CM.foldTailByMasking() && !Legal->getReductionVars().empty()) {
-    Builder.setInsertPoint(VPBB);
-    auto *Cond = RecipeBuilder.createBlockInMask(OrigLoop->getHeader(), Plan);
-    for (auto &Reduction : Legal->getReductionVars()) {
-      if (CM.isInLoopReduction(Reduction.first))
-        continue;
-      VPValue *Phi = Plan->getOrAddVPValue(Reduction.first);
-      VPValue *Red = Plan->getOrAddVPValue(Reduction.second.getLoopExitInstr());
-      Builder.createNaryOp(Instruction::Select, {Cond, Red, Phi});
-    }
-  }
+  adjustRecipesForInLoopReductions(VPBB, Plan, RecipeBuilder, Range.Start);
 
----------------
Ayal wrote:
> Thanks! Rename adjustRecipesForInLoopReductions() to adjustRecipesForReductions()?
> 
> Would be good to iterate over all header (reduction) phi's once, checking for inLoop or not. Note that the select handles only non-inLoop reductions, under foldTail. Deserves a separate patch?
> Thanks! Rename adjustRecipesForInLoopReductions() to adjustRecipesForReductions()?

Ah thanks, I thought I renamed it, but was wrong. should be updated now.

> Would be good to iterate over all header (reduction) phi's once, checking for inLoop or not. Note that the select handles only non-inLoop reductions, under foldTail. Deserves a separate patch?

Can update as separate patch to keep the diff cleaner.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9529
+  //  VPlan. fixReduction will introduce the appropriate selects and update the
+  //  users outside the loop without relying on the select recipe.
+  if (CM.foldTailByMasking()) {
----------------
Ayal wrote:
> Ah, ILV::fixReduction() actually looks for this select and hooks up to it, under foldTail:
> 
> 
> ```
>    Instruction *LoopExitInst = RdxDesc.getLoopExitInstr();
>    ...
>    VPValue *LoopExitInstDef = State.Plan->getVPValue(LoopExitInst);
>    ...
>    if (Cost->foldTailByMasking() && !PhiR->isInLoop()) {
>      for (unsigned Part = 0; Part < UF; ++Part) {
>        Value *VecLoopExitInst = State.get(LoopExitInstDef, Part);
>        Value *Sel = nullptr;
>        for (User *U : VecLoopExitInst->users()) {
>          if (isa<SelectInst>(U)) {
>            assert(!Sel && "Reduction exit feeding two selects");
>            Sel = U;
>          } else
>            assert(isa<PHINode>(U) && "Reduction exit must feed Phi's or select");
>        }
>        assert(Sel && "Reduction exit feeds no select");
>        State.reset(LoopExitInstDef, Sel, Part);
>   
>        // If the target can create a predicated operator for the reduction at no
>        // extra cost in the loop (for example a predicated vadd), it can be
>        // cheaper for the select to remain in the loop than be sunk out of it,
>        // and so use the select value for the phi instead of the old
>        // LoopExitValue.
>        if (PreferPredicatedReductionSelect ||
>            TTI->preferPredicatedReductionSelect(
>                RdxDesc.getOpcode(), PhiTy,
>                TargetTransformInfo::ReductionFlags())) {
>          auto *VecRdxPhi =
>              cast<PHINode>(State.get(PhiR->getVPSingleValue(), Part));
>          VecRdxPhi->setIncomingValueForBlock(
>              LI->getLoopFor(LoopVectorBody)->getLoopLatch(), Sel);
>        }
>      }
>    }         
> ```
> 
> Simply take PhiR->getBackedgeValue() instead there as well?
> 
> PreferPredicatedReductionSelect should also hack the phi recipe and be taken care of by VPlan execute? (Here or in a separate patch.)
> Simply take PhiR->getBackedgeValue() instead there as well?

Should be updated there, thanks!

> PreferPredicatedReductionSelect should also hack the phi recipe and be taken care of by VPlan execute? (Here or in a separate patch.)

I can do that in a separate one.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9419
+      VPValue *Red = Plan->getOrAddVPValue(
+          PhiR->getRecurrenceDescriptor().getLoopExitInstr());
+      Builder.createNaryOp(Instruction::Select, {Cond, Red, PhiR});
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > `VPValue *Red = PhiR->getBackedgeValue ();`?
> > > 
> > > Would be good to note that (and remember how ;-) ILV::fixReduction() later hooks up to this select, as it seems pretty useless here.
> > I updated the code as suggested, but is the loop exit value always the backedge value? I added a note that the select here is only to model the def-use chain in VPlan (AFAIU)
> > is the loop exit value always the backedge value?
> 
> It appears so, taken from IVDescriptors.cpp/RecurrenceDescriptor::AddReductionVar():
> ```
>          // The instruction used by an outside user must be the last instruction
>          // before we feed back to the reduction phi. Otherwise, we loose VF-1
>          // operations on the value.
>          if (!is_contained(Phi->operands(), Cur))
>            return false;
>   
>          ExitInstruction = Cur;
>          continue;
> ```
> Otherwise the generated the select between exit-instruction and phi may be erroneous.
> It would be clearer if getLoopExitInstr() would return Phi's operand across the backedge, instead of recording ExitInstruction.
> 
Ok great!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100102