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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:578
 
-  /// Create a new induction variable inside L.
-  PHINode *createInductionVariable(Loop *L, Value *Start, Value *End,
-                                   Value *Step, Instruction *DL);
+  /// Connect the header to the header (backedge) and exit blocks of \p L.
+  void createHeaderBranch(Loop *L);
----------------
Ayal wrote:
> More accurately, something like:
> /// Introduce a conditional branch (on true, condition to be set later) at the end of the header=latch connecting it to itself (across the backedge) and to the exit block of \p L.
Adjusted wording, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8916
+// VPInstruction to increment it by VF * UF to the header block.
+static void addCanonicalIVPHIRecipes(VPlan &Plan, Type *IdxTy, DebugLoc DL,
+                                     bool HasNUW, bool IsVPlanNative) {
----------------
Ayal wrote:
> addCanonicalIVPHIRecipes >> addCanonicalIVRecipes
> 
> > ... to the header block.
> The phi is added to the header block, the increment is added to the latch block.
Adjusted wording ,thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:780
+  case VPInstruction::CanonicalIVIncrement:
+    O << "canonical induction increment";
+    break;
----------------
Ayal wrote:
> How about printing something like " VF*UF + " instead?
That is much better, updated!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:927
+    auto *PhiR = dyn_cast<VPHeaderPHIRecipe>(&R);
+    if (!PhiR || isa<VPWidenPHIRecipe>(&R))
+      continue;
----------------
Ayal wrote:
> `if (isa_and_nonnull<VPWidenPHIRecipe>(PhiR))` ?
> 
> Use PhiR instead of &R below?
There are some phi-like recipes that do not inherit from VPHeaderPHIRecipe, like VPWidenIntOrFpInductionRecipe. I think it would be better to adjust this separately, once they *only* model the phi.

I updated the code the explicitly skip unwanted recipes and then use `cast<VPHeaderPHIRecipe>`.


> Use PhiR instead of &R below?

Done!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:948
+  auto *CanonicalIV = cast<VPCanonicalIVPHIRecipe>(&*Header->begin());
+  // TODO: Model compare and branch explicitly in VPlan as recipes.
+  auto *Next = State->get(CanonicalIV->getBackedgeValue(), 0);
----------------
Ayal wrote:
> call getCanonicalIV(), or call getCanonicalIVIncrement() which should take care of looking up the backedge value of the canonical IV - or retrieving the last recipe in the latch directly?
updated to use `getCanonicalIV`.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:178
   }
 
   for (const VPRegionBlock *Region :
----------------
Ayal wrote:
> Verify that the last recipe in Exit is a CanonicalIVIncrement VPInstruction?
I am not sure this is something we should enforce in the verifier, as it is easy to retrieve from the canonical IV.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113223