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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 7 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8895
       *PSE.getSE());
+
+  for (const auto &[_, II] : Legal->getInductionVars()) {
----------------
Ayal wrote:
> 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.)
It's not needed in the latest version, removed!


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:418
+  /// Map SCEVs to their expanded values. Populated when executing
+  /// VPExpandSCEVRecipes.
+  DenseMap<const SCEV *, Value *> ExpandedSCEVs;
----------------
Ayal wrote:
> 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);`
Updated to pass a function_ref to get the expanded SCEV. Just passing the map isn't enough (without more changes to ILV), as we need to also handle expanding constants/SCEVUnknown. 


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


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

It is also needed for the simpler crash that was fixed earlier in `test1_pr58811`

I cleaned up the names used in the new tests though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147965