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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

Looks good to me, thanks! Minor last nits.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:316
+  /// a temporary workaround needed due to the current epilogue
+  /// handling.workaround needed due to the current epilogue handling.
+  DenseMap<const SCEV *, Value *> executePlan(ElementCount VF, unsigned UF,
----------------
Typo above.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:502
   /// epilogue vectorization, this function is overriden to handle the more
-  /// complex control flow around the loops.
-  virtual std::pair<BasicBlock *, Value *> createVectorizedLoopSkeleton();
+  /// complex control flow around the loops.  \p ExpandedSCEVs it is used to
+  /// look up SCEV expansions for expressions needed during skeleton creation.
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:563
+  /// left off. \p Step is the SCEV-expanded induction step to use. In cases
+  /// where the loop skeleton is more complicated (eg. epilogue vectorization)
+  /// and the resume values can come from an additional bypass block, the \p
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8914
       *PSE.getSE());
+
   VPBasicBlock *HeaderVPBB = new VPBasicBlock("vector.body");
----------------
nit: redundant empty line?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7789
+EpilogueVectorizerMainLoop::createEpilogueVectorizedLoopSkeleton(
+    function_ref<Value *(const SCEV *)> GetExpandedSCEV) {
   createVectorLoopSkeleton("");
----------------
fhahn wrote:
> Ayal wrote:
> > nit: this instance of ILV doesn't need GetExpandedSCEV.
> I think for now this is needed, as `createEpilogueVectorizedLoopSkeleton` is defined as virtual.
Right, but if defined as a pointer or optional caller could pass in null. Just observing.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:357
+  /// Returns the value corresponding to the expansion of \p Expr.
+  Value *getExpandedSCEV(const SCEV *Expr) const;
+
----------------
Uncalled for now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147965