Page MenuHomePhabricator

AMDGPU: Move SIFixSGPRLiveRanges to be a regalloc pass
ClosedPublic

Authored by arsenm on Sep 26 2015, 6:49 PM.

Details

Reviewers
tstellarAMD
Summary

Replace LiveInterval usage with LiveVariables. LiveIntervals
computes far more information than is needed for this pass
which just needs to find if an SGPR is live out of the
defining block.

LiveIntervals are not usually available that early, requiring
computing them twice which is very expensive. The extra run of
LiveIntervals/LiveVariables/SlotIndexes was costing in total
about 5% of compile time.

Continuing to use LiveIntervals is problematic. It seems
there is an option (early-live-intervals) to run the analysis
about where it should go to avoid recomputing LiveVariables,
but it seems to be completely broken with subreg liveness
enabled. There are also problems from trying to recompute
LiveIntervals since this seems to undo LiveVariables
and clearing kill flags, causing TwoAddressInstructions
to make bad decisions.

Insert the pass right after live variables and preserve it.
The tricky case to worry about might be phis since
LiveVariables doesn't count a register as live out if
in the successor block it is only used in a phi,
but I don't think this is a concern right now
because SIFixSGPRCopies replaces SGPR phis.

Diff Detail

Event Timeline

arsenm updated this revision to Diff 35815.Sep 26 2015, 6:49 PM
arsenm retitled this revision from to AMDGPU: Move SIFixSGPRLiveRanges to be a regalloc pass.
arsenm updated this object.
arsenm added a reviewer: tstellarAMD.
arsenm added a subscriber: llvm-commits.

Note that long-term we plan to remove the LiveVariables analysis and just keep LiveIntervals around; though I don't see this happening soon as I don't see an incentive to actually do the work of fixing TwoAddressInstructions and PhiElimination to use LiveIntervals...

Note that long-term we plan to remove the LiveVariables analysis and just keep LiveIntervals around; though I don't see this happening soon as I don't see an incentive to actually do the work of fixing TwoAddressInstructions and PhiElimination to use LiveIntervals...

I'm kind of confused what the relationship between LiveVariables and LiveIntervals is, but I've seen the comments about removing LiveVariables. LiveIntervals seems to require LiveVariables, but then the calculation of LiveIntervals seems to clear kill flags?

The fact that LiveIntervals requires LiveVariables appears to be hack. From LiveIntervalAnalaysis.cpp:

// LiveVariables isn't really required by this analysis, it is only required
// here to make sure it is live during TwoAddressInstructionPass and
// PHIElimination. This is temporary.

This is probably not the right place for an in-depth discussion but kill flags is another feature that we hope to remove long term... Yes the current state of TwoAddressInstruction and PhiElimination prohibits those changes and I don't see this getting fixed in the near future.

tstellarAMD accepted this revision.Oct 1 2015, 1:40 PM
tstellarAMD edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Oct 1 2015, 1:40 PM
arsenm closed this revision.Oct 1 2015, 3:12 PM

r249087 with the asserts removed about physical register defs because it is not actually useful and should be a verifier error