[PATCH] D147965: [LV] Use VPValue to get expanded value for SCEV step expressions.
Ayal Zaks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 8 16:23:30 PDT 2023
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
More information about the llvm-commits
mailing list