This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Only create extracts for recurrence exits if there are live-outs.
ClosedPublic

Authored by fhahn on Apr 4 2023, 1:23 PM.

Details

Summary

Move the code to collect live-out earlier and only generate extracts for
exit values if there are any live-outs that use them.

Depends on D147472.

Diff Detail

Event Timeline

fhahn created this revision.Apr 4 2023, 1:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2023, 1:23 PM
fhahn requested review of this revision.Apr 4 2023, 1:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2023, 1:23 PM
Ayal accepted this revision.Apr 8 2023, 3:20 PM

LGTM, adding minor nits.

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

nit: define ExtractForScalar closer to its use below?

Set insert point, and possibly RuntimeVF, early for both Extract's.

3869

nit: use Incoming instead of ExtractForScalar, similar to ExtractForPhiUsedOutsideLoop.

3871–3872

nit: define ExtractForPhiUsedOutsideLoop inside the if below?

Names of both ExtractFor's could be better aligned (independent of this patch), perhaps ExtractForPhiInScalarLoop?

3883

nit: should this else if (UF > 1) be an else { assert(UF > 1 && "VF and UF are both 1?"); ...}?

This revision is now accepted and ready to land.Apr 8 2023, 3:20 PM
fhahn marked 4 inline comments as done.Apr 10 2023, 1:01 PM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3862

Will do separately.

3869

Will do separately.

3871–3872

Moved inside the loop. I'll address the suggestions independent to the patch as follow-up.

3883

Looks like it, I'll turn it into an assert separately.

fhahn updated this revision to Diff 512232.Apr 10 2023, 1:01 PM
fhahn marked 4 inline comments as done.

Rebase before landing, thanks for taking a look!

This revision was landed with ongoing or failed builds.Apr 10 2023, 1:08 PM
This revision was automatically updated to reflect the committed changes.
fhahn added inline comments.Apr 10 2023, 1:29 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3862

Should be done in f9d0b35d2238

3869

Should be done in f9d0b35d2238

3883

Should be done in 954befe2a745

Ayal added inline comments.Apr 10 2023, 3:44 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3862

nit: define ExtractForScalar closer to its use below?

post-commit: the method currently defines ExtractForScalar, then defines and uses ExtractForPhiUsedOutsideLoop, and finally uses ExtractForScalar. Better fully handle scalar loop Phi and Start first, and then take care of exit block live-outs - potentially early-exiting if there are no live-outs? May need to reset Builder again.

3903

post-commit nit: better assert once, early - inside if (!LiveOuts.empty())?