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

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:504
+  virtual std::pair<BasicBlock *, Value *>
+  createVectorizedLoopSkeleton(VPlan &Plan, VPTransformState &State);
 
----------------
Ayal wrote:
> 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...
Updated to just pass `const VPTransformState`.


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3235
     PHINode *OrigPhi = InductionEntry.first;
     const InductionDescriptor &II = InductionEntry.second;
     PHINode *BCResumeVal = createInductionResumeValue(
----------------
Ayal wrote:
> 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.
Updated to use mapping for now as it is simpler. Using recipes should be sufficient when the full creation is moved to VPlan.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147965