[PATCH] D147965: [LV] Use VPValue to get expanded value for SCEV step expressions (WIP).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8895
       *PSE.getSE());
+
+  for (const auto &[_, II] : Legal->getInductionVars()) {
----------------
fhahn wrote:
> Ayal wrote:
> > Deserves a comment. All SCEV-expands in preheader are executed before building skeleton and resume values?
> > 
> > nit: redundant {}
> Added comment & removed {}, thanks!
Thanks!

Why must all InductionVars be traversed (here) to build SCEVExpr's for their steps - instead of waiting for tryToCreateWidenRecipe() below to call createWidenInductionRecipes() on each header induction phi and presumably do so? (These phi's are surely not in DeadInstructions.)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:297
+  if (auto *U = dyn_cast<SCEVUnknown>(Expr))
+    return U->getValue();
+  return ExpandedSCEVs.find(Expr)->second;
----------------
Worth asserting that Expr has been expanded?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:418
+  /// Map SCEVs to their expanded values. Populated when executing
+  /// VPExpandSCEVRecipes.
+  DenseMap<const SCEV *, Value *> ExpandedSCEVs;
----------------
Worth defining and passing around, say,
`  using SCEV2ValueTy = DenseMap<const SCEV *, Value *>;`
instead of the full VPTransformState?

vputils could take care of
`  getExpandedSCEV(const SCEV2ValueTy &ExpandedSCEVs, const SCEV *ToExpand);`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1154
   Value *Res = Exp.expandCodeFor(Expr, Expr->getType(),
                                  &*State.Builder.GetInsertPoint());
+  State.ExpandedSCEVs[Expr] = Res;
----------------
Worth asserting (or checking?) that Expr has yet to be recorded?


================
Comment at: llvm/test/Transforms/LoopVectorize/pr58811-scev-expansion.ll:285
+  br label %invoke.cont99.2
+
+invoke.cont120.2:                                 ; No predecessors!
----------------
Can the test be simplified and more robust? Below is an unreachable block, above is an endless loop w/o exit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147965