[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:4226
 
+  auto *IdxTy = Builder.getInt32Ty();
   VPValue *PreviousDef = PhiR->getBackedgeValue();
----------------
nit: can keep IdxTy in original location to reduce diff


================
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)));
----------------
above comment deserves update


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4230
   // This phi node will eventually be deleted.
   Builder.SetInsertPoint(cast<Instruction>(State.get(PhiR, 0)));
 
----------------
SetInsertPoint(VecPhi)?


================
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;
----------------
(This part of fixing middle block and beyond should remain in ILV::fixFirstOrderRecurrence(), until the scope of VPlan expands.)


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9234
+
+    VPRecipeBase*PrevRecipe = RecurPhi->getBackedgeRecipe();
+    if (auto *Region = GetReplicateRegion(PrevRecipe)) {
----------------
    VPRecipeBase[ ]*PrevRecipe


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9169
+    RecurP->replaceAllUsesWith(RecurSplice);
+    RecurSplice->setOperand(0, RecurP);
+  }
----------------
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? 


================
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)
----------------
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`?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:286
-      continue;
-
     // Move recipes to the successor region.
----------------
This check for IsImmovableRecipe is no longer needed?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:106
     VPVWidenCanonicalIVSC,
+    VPVFirstOrderRecurrencePHISC,
     VPVWidenIntOrFpInductionSC,
----------------
(try to) keep lex order within Phi-like VPValues?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:337
     VPWidenCanonicalIVSC,
+    VPFirstOrderRecurrencePHISC,
     VPWidenIntOrFpInductionSC,
----------------
ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105008