[PATCH] D113223: [VPlan] Add VPCanonicalIVRecipe, partly retire createInductionVariable.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3006
   BasicBlock *Header = L->getHeader();
-  BasicBlock *Latch = L->getLoopLatch();
-  // As we're just creating this loop, it's possible no latch exists
-  // yet. If so, use the header as this will be a single block loop.
-  if (!Latch)
-    Latch = Header;
+  BasicBlock *Latch = Header;
 
----------------
If we are sure no latch exists yet as we're just creating this loop, update comment and assert(!L->getLoopLatch())?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3628
 
-  // Some loops have a single integer induction variable, while other loops
-  // don't. One example is c++ iterators that often have multiple pointer
-  // induction variables. In the code below we also support a case where we
-  // don't have a single induction variable.
-  //
-  // We try to obtain an induction variable from the original loop as hard
-  // as possible. However if we don't find one that:
-  //   - is an integer
-  //   - counts from zero, stepping by one
-  //   - is the size of the widest induction variable type
-  // then we create a new one.
   OldInduction = Legal->getPrimaryInduction();
+  createLatchTerminator(Lp);
----------------
Perhaps place the call to createLatchTerminator(Lp) above the setting of OldInduction, as the latter is not used by the former, whereas createInductionResumeValues() below does use OldInduction. Perhaps OldInduction can be eliminated altogether, following [New]Induction?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4528
+  auto *Plan = PhiR->getParent()->getPlan();
+  auto *IVR = Plan->getCanonicalIV();
+  PHINode *CanonicalIV = cast<PHINode>(State.get(IVR, 0));
----------------
Plan is used only here, right? So can fold into
`auto *IVR = PhiR->getParent()->getPlan()->getCanonicalIV();`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9666
+  auto *CanonicalIV =
+      State.get(getParent()->getPlan()->getCanonicalIV(), VPIteration(0, 0));
+  State.ILV->widenIntOrFpInduction(
----------------
State.get per-part 0?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8928
+// to the header block.
+static void addCanonicalIV(VPlan &Plan, Type *IdxTy, DebugLoc DL, bool HasNUW,
+                           bool IsVPlanNative) {
----------------
Ayal wrote:
> "addCanonicalIV" >> "addCanonicalIVRecipe"?
Ahh, on second thought, this method adds two recipes (one being a VPInstruction, but that may change..), one placed first in the header and the other last in the latch. How about "addCanonicalIVRecipes()"?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:726
+    // times the unroll factor (num of SIMD instructions).
+    Value *Step = createStepForVF(Builder, Phi->getType(), State.VF, State.UF);
+    Value *Next = Builder.CreateAdd(Phi, Step, "index.next", IsNUW, false);
----------------
If Step is fixed to be VF*UF rather than support arbitrary inductions, a more accurate name would be CanonicalIVIncrement[NUW]?

Potentially related to https://reviews.llvm.org/D116123#inline-1111668


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1350
+  Value *CanonicalIV =
+      State.get(getParent()->getPlan()->getCanonicalIV(), VPIteration(0, 0));
   Type *STy = CanonicalIV->getType();
----------------
State.get per-part 0?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:919
+      auto *ICmp = cast<Instruction>(
+          State->Builder.CreateICmpEQ(Next, State->VectorTripCount));
+      TermBr->setCondition(ICmp);
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Introducing the ICmpEQ feeding TermBr should ideally be done by an appropriate (BCT) recipe in the normal course of VPlan::execute() stage 2 above, rather than during this backedge-rewiring stage 3.
> > I added a TODO and this should be addressed soon in a follow-up patch. Or should those changes also be pulled in here?
> > I added a TODO and this should be addressed soon in a follow-up patch. Or should those changes also be pulled in here?
> 
> As you prefer. A BranchOnCount recipe could potentially take care of the Increment[NUW]-feeding-the-ICmpEQ-feeding-the-branch better, while defining the single VPValue to feed back the CanonicalIVPHI. It could be introduced here instead of Increment[NUW] et al., or replace them in the TODO follow-up patch.
If the ICmpEQ is to be generated here/now connected to InductionIncrement and branch, suggest to do so separately rather than inside a loop taking care of rewiring header phis. The InductionIncrement can be located directly at the end of the Exit, analogous to getCanonicalIV(), conceptually as part of a terminating inc-cmp-br idiom.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2180
+  /// Represents the vector trip count.
+  VPValue *VectorTripCount = nullptr;
+
----------------
Hold `VPValue VectorTripCount;` as an instance rather than a pointer?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2254
+  /// The vector trip count.
+  VPValue *getVectorTripCount() { return VectorTripCount; }
+
----------------
Can return a reference, is never null.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2360
+    }
+    return cast<VPCanonicalIVPHIRecipe>(&*EntryVPBB->phis().begin());
+  }
----------------
can drop phis().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113223