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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal 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,
----------------
Return value should be documented. Suffice that it be a Map or 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(
----------------
\p State was replaced by a function.


================
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(
----------------
\p Step should also be documented.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7789
+EpilogueVectorizerMainLoop::createEpilogueVectorizedLoopSkeleton(
+    function_ref<Value *(const SCEV *)> GetExpandedSCEV) {
   createVectorLoopSkeleton("");
----------------
nit: this instance of ILV doesn't need GetExpandedSCEV.


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:418
+  /// Map SCEVs to their expanded values. Populated when executing
+  /// VPExpandSCEVRecipes.
+  DenseMap<const SCEV *, Value *> ExpandedSCEVs;
----------------
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?


================
Comment at: llvm/test/Transforms/LoopVectorize/pr58811-scev-expansion.ll:285
+  br label %invoke.cont99.2
+
+invoke.cont120.2:                                 ; No predecessors!
----------------
fhahn wrote:
> 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.
OK, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147965