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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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)) {
----------------
Ayal wrote:
> "expandSCEVExpr" - rename? This wraps the SCEVExpr in a VPValue (or recipe - introduced in preheader, if not present there already), but does not expand it.
Good point, I changed it to `getOrCreateVPValueForSCEVExpr`, which also more closely matches the documentation of the function.


================
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));
 
----------------
Ayal wrote:
> getOperand(1) >> getStepValue()? Analogous to setStartValue().
Thanks, updated to use getStepValue().


================
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.
----------------
Ayal wrote:
> Who's \I ?
Updated to Phi, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1762
+  }
+
+  VPBasicBlock *Preheader = Plan.getEntry()->getEntryBasicBlock();
----------------
Ayal wrote:
> 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?
Updated to cache scev expressions.


================
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]]>
----------------
Ayal wrote:
> Should this change appear in additional tests?
I think this is the only place where a expanded scev operand is printed. I updated the other tests that take an ir constant as step when printing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122096