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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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).
----------------
Ayal wrote:
> temporary value for s1 - this value is no longer temporary, but rather one that is updated here
Updated to say that we create a `phi node v1 for s1`.


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


================
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:
> UF is at-least 1?
Yes, if UF == 0, then `VecPhi` is used.


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9162
+        Builder.createNaryOp(VPInstruction::FirstOrderRecurrenceSplice,
+                             {RecurP, Prev->getVPValue()}));
+    if (auto *Region = GetReplicateRegion(Prev)) {
----------------
Ayal wrote:
> Better have a `VPValue` *Prev rather than getDef()->getVP[Single]Value()?
I moved the call to `getBackedgeValue` directly here and use `getBackedgeRecipe` below.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9163
+                             {RecurP, Prev->getVPValue()}));
+    if (auto *Region = GetReplicateRegion(Prev)) {
+      VPBasicBlock *Succ = cast<VPBasicBlock>(Region->getSingleSuccessor());
----------------
Ayal wrote:
> assert Prev is the last insn in its region?
`GetReplicateRegion` already asserts that the recipes parent block contains exactly 1 instruction.


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:782
     ActiveLaneMask,
+    FirstOrderRecurrenceSplice, // Combines the incoming and previous values of
+                                // a first-order recurrence.
----------------
Ayal wrote:
> try to maintain lexicographic order?
I moved it to the top, but left the others as is for now. I can clean up the ordering separately.


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


================
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");
----------------
Ayal wrote:
> If we expect a single value, call getVPSingleValue?
yes, removed!

I'll remove the default value from the const version in a separate commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105008