This is an archive of the discontinued LLVM Phabricator instance.

[LV] Use VPValue for SCEV expansion in fixupIVUsers.
ClosedPublic

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

Details

Summary

The step is already expanded in the VPlan. Use this expansion instead.
This is a step towards modeling fixing up IV users in VPlan.

It also fixes a crash casued by SCEV-expanding the Step expression in
fixupIVUsers, where the IR is in an incomplete state

Diff Detail

Event Timeline

fhahn created this revision.Apr 10 2023, 12:45 PM
fhahn requested review of this revision.Apr 10 2023, 12:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2023, 12:45 PM
Ayal added inline comments.Apr 16 2023, 8:34 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3399

Would it make sense to look for the IV recipe that corresponds to OrigPhi (with overridden getVPValue() or by scanning header phi recipes) and then reach its Step, instead of looking for StepR in preheader and relying on II.getStep()?
(Traversing IV recipes (or live-outs) instead of original phi's could probably follow.)

fhahn updated this revision to Diff 514238.Apr 17 2023, 7:52 AM
fhahn marked an inline comment as done.

Add comment explaining step expansion.

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

Unfortunately we cannot rely on mapping IV recipes to OrigPhi here, as an original VPWidenIntOrFpInductionRecipe ( which maps to OrigPhi) could be transformed to a VPScalarIVStepsRecipe, which doesn't store OrigPhi.

This will only be cleaned up once we completely model fixing up inductions in VPlan.

fhahn updated this revision to Diff 514248.Apr 17 2023, 8:21 AM

Added test case that shows this patch is not an NFC. It fixes a crash casued by SCEV-expanding the Step expression in fixupIVUsers, where the IR is in an incomplete state. I will adjust the title accordingly.

fhahn retitled this revision from [LV] Use VPValue for SCEV expansion in fixupIVUsers (NFCI). to [LV] Use VPValue for SCEV expansion in fixupIVUsers..Apr 17 2023, 8:21 AM
fhahn edited the summary of this revision. (Show Details)
Ayal added inline comments.Apr 17 2023, 9:19 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3399

ok, deserves a TODO note emphasizing it's a temporary solution.

Worth recording a SCEVtoExpandSCEVRecipe mapping in VPlan (with a note that it too is temporary)?

fhahn updated this revision to Diff 515677.Apr 21 2023, 4:33 AM

Update to add temporary mapping from SCEV -> VPValue expansions.

fhahn marked an inline comment as done.Apr 21 2023, 4:35 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3399

Updated the code to have a temporary mapping. I've not added a TODO, as moving IV fix up to VPlan is a general TODO :)

fhahn updated this revision to Diff 517895.Apr 28 2023, 6:26 AM
fhahn marked an inline comment as done.

rebase and ping :)

Ayal accepted this revision.May 3 2023, 7:57 AM

LGTM, with some comments, also provided in D147964.

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

nit: worth asserting that step SCEV is expected to have a VPValue?

3438–3439
This revision is now accepted and ready to land.May 3 2023, 7:57 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked 2 inline comments as done.May 4 2023, 1:32 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3437

Added an assert in the committed version, thanks!

3438–3439

Updated in the committed version, thanks!