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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:313
   /// vectorization re-using plans for both the main and epilogue vector loops.
   /// It should be removed once the re-use issue has been fixed.
+  VPTransformState executePlan(ElementCount VF, unsigned UF, VPlan &BestPlan,
----------------
Ayal wrote:
> Return value should be documented. Suffice that it be a Map or function?
Added documentation. Also changed to return just the map. This in turn lead to changing the functions to just pass the map and have a static `getExpandedSCEV` helper function :) 


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:501
+  /// complex control flow around the loops.  \p State it is used to look up
+  /// SCEV expansions for expressions needed during skeleton creation.
+  virtual std::pair<BasicBlock *, Value *> createVectorizedLoopSkeleton(
----------------
Ayal wrote:
> \p State was replaced by a function.
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:563
   /// bypass block, the \p AdditionalBypass pair provides information about the
   /// bypass block and the end value on the edge from bypass to this loop.
   PHINode *createInductionResumeValue(
----------------
Ayal wrote:
> \p Step should also be documented.
Documented, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7789
+EpilogueVectorizerMainLoop::createEpilogueVectorizedLoopSkeleton(
+    function_ref<Value *(const SCEV *)> GetExpandedSCEV) {
   createVectorLoopSkeleton("");
----------------
Ayal wrote:
> nit: this instance of ILV doesn't need GetExpandedSCEV.
I think for now this is needed, as `createEpilogueVectorizedLoopSkeleton` is defined as virtual.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10435
             ResumeV = MainILV.createInductionResumeValue(
-                IndPhi, *ID, {EPI.MainLoopIterationCountCheck});
+                IndPhi, *ID, State.getExpandedSCEV(ID->getStep()),
+                {EPI.MainLoopIterationCountCheck});
----------------
Ayal wrote:
> This code belongs anywhere but in LoopVectorizePass::processLoop()...
Agreed, let me move it as follow-up.


================
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:
> fhahn wrote:
> > Ayal wrote:
> > > fhahn wrote:
> > > > 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. 
> > > Passing the map complemented with moving `VPTransformState::getExpandedSCEV(const SCEV*)` to `vputils::getExpandedSCEV(const SCEV2ValueTy&, const SCEV*)` to handle expanding constants/SCEVUnkown?
> > Hm, I am not sure about moving the functionality to `vputils` as there's no recipes/other VPlan components involved, only mapping from SCEV -> Value and it is only temporarily needed. Happy to move it if you still prefer to move it, 
> Agreed, given a SCEV2ValueTy map this becomes a static function that is VPlan-context free.
> 
> One alternative is to have it as another static "helper function" of ILV, possibly also folding getStep() into it, say
> `static Value* getExpandedStep(const SCEV2ValueTy &Map, const InductionDescriptor *ID);`
> 
> Another option is to also populate Constant and Unknown SCEV steps into the Map, so that it has all SCEVs of interest.
> 
> In any case, worth defining a type to be passed around, either as map or function?
Updated to just return the map from `executePlan` and have a static function to get the expanded value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147965