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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn 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);
----------------
Ayal wrote:
> Is this needed? We're not inserting anything at VecPhi, right?
Nope, I removed it.


================
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);
----------------
Ayal wrote:
> 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);
>   }
> ```
> ?
Sounds good, updated.


================
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());
----------------
Ayal wrote:
> 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.
Oh I see. I put up D106244 as follow-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105008