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

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4229
   // We constructed a temporary phi node in the first phase of vectorization.
   // This phi node will eventually be deleted.
   Builder.SetInsertPoint(cast<Instruction>(State.get(PhiR, 0)));
----------------
Ayal wrote:
> above comment deserves update
I removed it as it seems out of date now.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4237
   // Extract the last vector element in the middle block. This will be the
   // initial value for the recurrence when jumping to the scalar loop.
   auto *ExtractForScalar = Incoming;
----------------
Ayal wrote:
> (This part of fixing middle block and beyond should remain in ILV::fixFirstOrderRecurrence(), until the scope of VPlan expands.)
sounds good, the code will remain here for now.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9223
   }
 
+  // Introduce a recipe to combine the incoming and previous values of a first-order recurrence.
----------------
Ayal wrote:
> LVP::buildVPlanWithVPRecipes() is now reaching 300 LOC... would be good to split it, possibly in a follow-up patch.
sounds good as a follow up.


================
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:
> 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.


================
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:
> > > 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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9169
+    RecurP->replaceAllUsesWith(RecurSplice);
+    RecurSplice->setOperand(0, RecurP);
+  }
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > RecurSplice was built with RecurP as it's first operand?
> > Yes, but we replace all uses of `RecurP`, so this gets fixed up here.
> ah, right, should RAU but for RecurSplice. Worth a comment? 
yes, added a comment.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:703
+
+    // For the first part, use the recurrence phi (v1), otherwise v2.
+    Value *Incoming = Part == 0 ? State.get(getOperand(0), Part)
----------------
Ayal wrote:
> Perhaps it would be clearer to first set
> ```
>   auto *V1 = State.get(getOperand(0), 0));
> ```
> and then use it to set `Incoming`, better renamed `PartMinus1`?
updated, thanks.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:286
-      continue;
-
     // Move recipes to the successor region.
----------------
Ayal wrote:
> This check for IsImmovableRecipe is no longer needed?
no, because the shuffle mentioned in the comment is now explicit in the VPlan, and the existing def-use checks ensure that the transform does not happen in the case.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:106
     VPVWidenCanonicalIVSC,
+    VPVFirstOrderRecurrencePHISC,
     VPVWidenIntOrFpInductionSC,
----------------
Ayal wrote:
> (try to) keep lex order within Phi-like VPValues?
should be at the right place now.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:337
     VPWidenCanonicalIVSC,
+    VPFirstOrderRecurrencePHISC,
     VPWidenIntOrFpInductionSC,
----------------
Ayal wrote:
> ditto
should be at the right place now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105008