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

[Ayal Zaks via Phabricator via llvm-commits]

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


================
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()) {
----------------
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.)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9419
+      VPValue *Red = Plan->getOrAddVPValue(
+          PhiR->getRecurrenceDescriptor().getLoopExitInstr());
+      Builder.createNaryOp(Instruction::Select, {Cond, Red, PhiR});
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100102