[PATCH] D122096: [VPlan] Expand induction step in VPlan pre-header.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8165
+
+  VPValue *Step = vputils::expandSCEVExpr(Plan, IndDesc.getStep(), SE);
   if (auto *TruncI = dyn_cast<TruncInst>(PhiOrTrunc)) {
----------------
"expandSCEVExpr" - rename? This wraps the SCEVExpr in a VPValue (or recipe - introduced in preheader, if not present there already), but does not expand it.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9415
+  // Now do the actual transformations, and start with fetching the step value.
+  Value *Step = State.get(getOperand(1), VPIteration(0, 0));
 
----------------
getOperand(1) >> getStepValue()? Analogous to setStartValue().


================
Comment at: llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h:75
 
   /// Check if an induction recipe should be constructed for \I. If so build and
   /// return it. If not, return null.
----------------
Who's \I ?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1762
+  }
+
+  VPBasicBlock *Preheader = Plan.getEntry()->getEntryBasicBlock();
----------------
Add comment; searching if a recipe has already been created for Expr, and return it if so, to save redundant duplication(?)
Can alternatively cache Expr's that have recipes.

Should we also check if Constant or Unknown Expr's were assigned VPValues, and return them, instead of creating new ones - potentially leading to a leak?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1766
+    if (auto *ExpandR = dyn_cast<VPExpandSCEVRecipe>(&R)) {
+      if (ExpandR->getSCEV() == Expr)
+        return ExpandR;
----------------
nit: can fold the two if's


================
Comment at: llvm/test/Transforms/LoopVectorize/vplan-printing.ll:429
 ; CHECK-NEXT:     "  %iv = phi %iv.next, 0\l" +
-; CHECK-NEXT:     "  ir<%v2>
+; CHECK-NEXT:     "  ir<%v2>, vp<[[EXP_SCEV]]>
 ; CHECK-NEXT:     vp<[[STEPS:%.+]]> = SCALAR-STEPS vp<[[CAN_IV]]>, ir<0>, vp<[[EXP_SCEV]]>
----------------
Should this change appear in additional tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122096