This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Add prepareForExecution to set up live-ins (NFC).
ClosedPublic

Authored by fhahn on Dec 27 2021, 1:05 PM.

Details

Summary

This patch adds a new prepareForExecution helper to set up live-ins, so
VPTransformState doesn't need to hold values like TripCount.

This also requires making the trip count operand for ActiveLaneMask
explicit in VPlan.

Diff Detail

Unit TestsFailed

Event Timeline

fhahn created this revision.Dec 27 2021, 1:05 PM
fhahn requested review of this revision.Dec 27 2021, 1:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 27 2021, 1:05 PM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal accepted this revision.Dec 28 2021, 1:31 AM

This looks good to me, adding some optional suggestions.

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

Induction/CanonicalIV is another live-in that could be handled by prepareForExecution(), but soon D113223 will replace it and pass CanonicalIVStartValue to prepareForExecution() instead?

7970

Best placed right before calling BestVPlan.execute()?

Can alternatively have execute() call (a protected) prepareForExecution(), but in any case the caller needs to pass live-in Values separately from State.

Can alternatively be called prepareToExecute().

8464

Now that the comment is removed, so can the {} be removed...
Or set VPValue *TC = Plan->getOrCreateTripCount() analogous to the ICmpULE else case below.

llvm/lib/Transforms/Vectorize/VPlan.cpp
809

Slightly more logical to first handle trip count and then backedge taken count?

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

Perhaps TripCount should appear first, before BackedgeTakenCount. In any case, may be good to mention how the two relate.

2174

Can alternatively add TripCount et al. to VPValuesToFree when constructed, placing the remember-to-deallocate next to the allocation.

2207

Slightly more logical to list TripCount first and then BackedgeTakenCount?

This revision is now accepted and ready to land.Dec 28 2021, 1:31 AM
This revision was landed with ongoing or failed builds.Dec 28 2021, 8:51 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked 6 inline comments as done.
fhahn added inline comments.Dec 28 2021, 8:51 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7965

Yes, CanonicalIV won't be needed after D113223 and I'll updated D113223 to use prepareToExecute to handle CanonicalIVStartValue

7970

moved and renamed in the committed version

8464

updated to use TC variable

llvm/lib/Transforms/Vectorize/VPlan.cpp
809

reordered

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

mentioned that BTC is TripCount -1 in the comment for BTC.

2207

reordered