[PATCH] D105008: [VPlan] Add recipe for first-order rec phis, make splicing explicit.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4230
+  VPValue *PreviousDef = PhiR->getBackedgeValue();
+  Builder.SetInsertPoint(VecPhi);
+  Value *Incoming = State.get(PreviousDef, UF - 1);
----------------
Is this needed? We're not inserting anything at VecPhi, right?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4136
       fixReduction(cast<VPReductionPHIRecipe>(PhiR->getDef()), State);
-    } else if (Legal->isFirstOrderRecurrence(OrigPhi))
+    else if (isa<VPFirstOrderRecurrencePHIRecipe>(PhiR->getDef()))
       fixFirstOrderRecurrence(PhiR, State);
----------------
fhahn wrote:
> Ayal wrote:
> > Ayal wrote:
> > > no need to check if !PhiR, no need to getDef()
> > nit: can remove PhiR, check directly if &R is a Reduction or FOR PHIRecipe, and rename R >> PhiR
> I think this one should be addressed now.
Suffice to simply do:

```
  for (VPRecipeBase &PhiR : Header->phis()) {
    if (auto *ReductionPhi = dyn_cast<VPReductionPHIRecipe>(&PhiR))
      fixReduction(ReductionPhi, State);
    else if (isa<VPFirstOrderRecurrencePHIRecipe>(&PhiR))
      fixFirstOrderRecurrence(&PhiR, State);
  }
```
?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4220
   // Fix the latch value of the new recurrence in the vector loop.
+  Value *Incoming = UF == 0 ? VecPhi : State.get(PreviousDef, UF - 1);
   VecPhi->addIncoming(Incoming, LI->getLoopFor(LoopVectorBody)->getLoopLatch());
----------------
fhahn wrote:
> Ayal wrote:
> > fhahn wrote:
> > > Ayal wrote:
> > > > UF is at-least 1?
> > > Yes, if UF == 0, then `VecPhi` is used.
> > but when can UF == 0?
> > isn't the operand of VecPhi across the backedge always the last part of PreviousDef?
> > Can RecipeBuilder.fixHeaderPhis() hook up this operand?
> Oh right, not sure where this originally came from. I removed it now.
> 
> I don't think we can hook this up in `RecipeBuilder.fixHeaderPhis() `, because we only need to hook up the last part.
> I don't think we can hook this up in RecipeBuilder.fixHeaderPhis() , because we only need to hook up the last part.

Ah, sorry, RecipeBuilder.fixHeaderPhis() already hooks up the 2nd operand of FOR phi's. I meant to offload the code-gen fixing of VecPhi to Incoming's last part, from ILV here over to VPlan::execute(), possibly in a last stage there; leaving here only code-gen fixings of middle-block and beyond. Can also be done in a separate follow-up patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105008