[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:4136
       fixReduction(cast<VPReductionPHIRecipe>(PhiR->getDef()), State);
-    } else if (Legal->isFirstOrderRecurrence(OrigPhi))
+    else if (isa<VPFirstOrderRecurrencePHIRecipe>(PhiR->getDef()))
       fixFirstOrderRecurrence(PhiR, State);
----------------
no need to check if !PhiR, no need to getDef()


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4166
   // previous iteration. In the first phase of vectorization, we created a
   // temporary value for s1. We now complete the vectorization and produce the
   // shorthand vector IR shown below (for VF = 4, UF = 1).
----------------
temporary value for s1 - this value is no longer temporary, but rather one that is updated here


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4217
+  VecPhi->setName("vector.recur");
   VecPhi->addIncoming(VectorInit, LoopVectorPreHeader);
 
----------------
Can have FORRecipe::execute() name the phi and hook it up to VectorInit (Start)?


================
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());
----------------
UF is at-least 1?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9106
     VPRecipeBase *Target = RecipeBuilder.getRecipe(Entry.second);
 
     auto *TargetRegion = GetReplicateRegion(Target);
----------------
(lambda taken out of loop, to be reused below)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9152
   }
 
+  for (VPRecipeBase &R : Plan->getEntry()->getEntryBasicBlock()->phis()) {
----------------
Add a comment what the code below does?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9154
+  for (VPRecipeBase &R : Plan->getEntry()->getEntryBasicBlock()->phis()) {
+    auto *RecurP = dyn_cast<VPFirstOrderRecurrencePHIRecipe>(R.getVPValue());
+    if (!RecurP)
----------------
drop getVPValue()?
RecurP >> RecurPHI?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9159
+    VPRecipeBase *Prev =
+        cast<VPRecipeBase>(RecurP->getBackedgeValue()->getDef());
+    auto *RecurSplice = cast<VPInstruction>(
----------------
BackedgeValue is always a recipe; maybe worth having getBackedgeRecipe()


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9162
+        Builder.createNaryOp(VPInstruction::FirstOrderRecurrenceSplice,
+                             {RecurP, Prev->getVPValue()}));
+    if (auto *Region = GetReplicateRegion(Prev)) {
----------------
Better have a `VPValue` *Prev rather than getDef()->getVP[Single]Value()?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9163
+                             {RecurP, Prev->getVPValue()}));
+    if (auto *Region = GetReplicateRegion(Prev)) {
+      VPBasicBlock *Succ = cast<VPBasicBlock>(Region->getSingleSuccessor());
----------------
assert Prev is the last insn in its region?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9165
+      VPBasicBlock *Succ = cast<VPBasicBlock>(Region->getSingleSuccessor());
+      RecurSplice->moveBefore(*Succ, Succ->begin());
+    } else
----------------
moveBefore first non-phi?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9169
+    RecurP->replaceAllUsesWith(RecurSplice);
+    RecurSplice->setOperand(0, RecurP);
+  }
----------------
RecurSplice was built with RecurP as it's first operand?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1138
+  // directly.
+  // TODO: Remove once all VPWidenPHIRecipe instances keep all relevant incoming
+  // values as VPValues.
----------------
TODO needed here?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:782
     ActiveLaneMask,
+    FirstOrderRecurrenceSplice, // Combines the incoming and previous values of
+                                // a first-order recurrence.
----------------
try to maintain lexicographic order?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1132
+                         VPFirstOrderRecurrencePHISC, Phi) {
+    addOperand(&Start);
+  }
----------------
another constructor for VPWidenPHIRecipe, receiving both Phi and Start, in addition to the two SC's?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:365
   /// Returns the VPValue with index \p I defined by the VPDef.
-  VPValue *getVPValue(unsigned I) {
+  VPValue *getVPValue(unsigned I = 0) {
     assert(DefinedValues[I] && "defined value must be non-null");
----------------
If we expect a single value, call getVPSingleValue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105008