[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:504
+  virtual std::pair<BasicBlock *, Value *>
+  createVectorizedLoopSkeleton(VPlan &Plan, VPTransformState &State);
 
----------------
Suffice to pass (a const?) State only, and retrieve Plan from it?
Or pass a function that maps SCEVs to Values, or cache the values of this function in a map (in State) and pass it? To prevent future potential abuse of passed Plan and State...


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

nit: redundant {}


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3235
     PHINode *OrigPhi = InductionEntry.first;
     const InductionDescriptor &II = InductionEntry.second;
     PHINode *BCResumeVal = createInductionResumeValue(
----------------
fhahn wrote:
> Ayal wrote:
> > Suffice to retrieve Step here from II, Plan, and State, and pass it to createInductionResumeValue()?
> > Getting the Value expanded from a SCEV possibly deserves wrapping in a common function.
> > (Admittedly not saving much.)
> Adjusted the arguments, thanks!
Thanks!

Would it be better to simplify obtaining a Value for II.getStep() by passing a map<const SCEV *, Value *>, filled in State by VPExpandSCEVRecipe::execute(), or directly getValue() if II.getStep() is Constant or Unknown? Or this is a step towards taking care of creating resume values by an appropriate recipe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147965