This is an archive of the discontinued LLVM Phabricator instance.

[LV] Use VPValue to get expanded value for SCEV step expressions.
ClosedPublic

Authored by fhahn on Apr 10 2023, 12:52 PM.

Details

Summary

Update skeleton creation logic to use SCEV expansion results from
expanding the pre-header. This avoids another set of SCEV expansions
that may happen after the CFG has been modified.

Fixes #58811.

Depends on D147964.

Diff Detail

Event Timeline

fhahn created this revision.Apr 10 2023, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2023, 12:52 PM
fhahn requested review of this revision.Apr 10 2023, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2023, 12:52 PM
Ayal added inline comments.May 3 2023, 2:53 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3224

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.)

Ayal added inline comments.May 4 2023, 8:00 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2437–2438

Becomes dead and should be removed?

fhahn updated this revision to Diff 520095.May 6 2023, 8:34 AM

Rebase and address comments, thanks!

fhahn marked 2 inline comments as done.May 6 2023, 8:34 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2437–2438

Removed, thanks!

3224

Adjusted the arguments, thanks!

Ayal added inline comments.May 6 2023, 2:37 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
506

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...

3224

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.

8914

Deserves a comment. All SCEV-expands in preheader are executed before building skeleton and resume values?

nit: redundant {}

fhahn updated this revision to Diff 520196.May 7 2023, 10:23 AM
fhahn marked 3 inline comments as done.

Address latest comments, thanks! Updated to only pass const VPTransformState & and cache SCEV expansion mapping in state directly, use that.

fhahn marked 2 inline comments as done.May 7 2023, 10:25 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
506

Updated to just pass const VPTransformState.

3224

Updated to use mapping for now as it is simpler. Using recipes should be sufficient when the full creation is moved to VPlan.

8914

Added comment & removed {}, thanks!

Ayal added inline comments.May 7 2023, 1:10 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8914

Thanks!

Why must all InductionVars be traversed (here) to build SCEVExpr's for their steps - instead of waiting for tryToCreateWidenRecipe() below to call createWidenInductionRecipes() on each header induction phi and presumably do so? (These phi's are surely not in DeadInstructions.)

llvm/lib/Transforms/Vectorize/VPlan.cpp
297 ↗(On Diff #520196)

Worth asserting that Expr has been expanded?

llvm/lib/Transforms/Vectorize/VPlan.h
416

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);

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
1138

Worth asserting (or checking?) that Expr has yet to be recorded?

llvm/test/Transforms/LoopVectorize/pr58811-scev-expansion.ll
285

Can the test be simplified and more robust? Below is an unreachable block, above is an endless loop w/o exit.

fhahn marked 7 inline comments as done.May 8 2023, 4:45 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8914

It's not needed in the latest version, removed!

llvm/lib/Transforms/Vectorize/VPlan.cpp
297 ↗(On Diff #520196)

Added assert, thanks!

llvm/lib/Transforms/Vectorize/VPlan.h
416

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.

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
1138

Added assert, thanks!

llvm/test/Transforms/LoopVectorize/pr58811-scev-expansion.ll
285

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.

fhahn updated this revision to Diff 520332.May 8 2023, 4:46 AM
fhahn marked 5 inline comments as done.

Address latest comments, thanks!

fhahn retitled this revision from [LV] Use VPValue to get expanded value for SCEV step expressions (WIP). to [LV] Use VPValue to get expanded value for SCEV step expressions..May 8 2023, 4:47 AM
fhahn edited the summary of this revision. (Show Details)
Ayal added inline comments.May 8 2023, 5:29 AM
llvm/lib/Transforms/Vectorize/VPlan.h
416

Passing the map complemented with moving VPTransformState::getExpandedSCEV(const SCEV*) to vputils::getExpandedSCEV(const SCEV2ValueTy&, const SCEV*) to handle expanding constants/SCEVUnkown?

fhahn marked an inline comment as done.May 8 2023, 9:37 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
416

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,

Ayal added inline comments.May 8 2023, 4:23 PM
llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
313–314

Return value should be documented. Suffice that it be a Map or function?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
503

\p State was replaced by a function.

562–566

\p Step should also be documented.

7812

nit: this instance of ILV doesn't need GetExpandedSCEV.

10457–10458

This code belongs anywhere but in LoopVectorizePass::processLoop()...

llvm/lib/Transforms/Vectorize/VPlan.h
416

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?

llvm/test/Transforms/LoopVectorize/pr58811-scev-expansion.ll
285

OK, thanks!

fhahn updated this revision to Diff 520663.May 9 2023, 5:22 AM
fhahn marked 7 inline comments as done.

Address latest comments, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
313–314

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 :)

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
503

Updated, thanks!

562–566

Documented, thanks!

7812

I think for now this is needed, as createEpilogueVectorizedLoopSkeleton is defined as virtual.

10457–10458

Agreed, let me move it as follow-up.

llvm/lib/Transforms/Vectorize/VPlan.h
416

Updated to just return the map from executePlan and have a static function to get the expanded value.

Ayal accepted this revision.May 9 2023, 6:00 AM

Looks good to me, thanks! Minor last nits.

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
316

Typo above.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
502–505
563
7812

Right, but if defined as a pointer or optional caller could pass in null. Just observing.

8914

nit: redundant empty line?

llvm/lib/Transforms/Vectorize/VPlan.h
357

Uncalled for now?

This revision is now accepted and ready to land.May 9 2023, 6:00 AM
This revision was landed with ongoing or failed builds.May 11 2023, 8:50 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked 6 inline comments as done.
fhahn added a comment.May 15 2023, 5:31 AM

Push unsubmitted comments

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
316

Fixed in the committed version, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
502–505

Fixed in the committed version, thanks!

563

Fixed in the committed version, thanks!

8914

Fixed in the committed version, thanks!

llvm/lib/Transforms/Vectorize/VPlan.h
357

Removed in the committed version, thanks!