Page MenuHomePhabricator

TargetLowering: Add finalizeLowering() function; NFC
ClosedPublic

Authored by MatzeB on Apr 27 2017, 4:05 PM.

Details

Summary

Adds a new method finalizeLowering to TargetLoweringBase. This is in preparation for an upcoming commit.

This function is meant for target specific adjustments to MachineFrameInfo or register reservations.

Move the freezeRegisters() and the hasCopyImplyingStackAdjustment() handling into the new function to prove the concept. As an added bonus GlobalISel no longer missed the hasCopyImplyingStackAdjustment() handling with this.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB created this revision.Apr 27 2017, 4:05 PM
MatzeB updated this revision to Diff 97024.Apr 27 2017, 5:04 PM
MatzeB retitled this revision from TargetLowering: Add finalizeLowering() function to TargetLowering: Add finalizeLowering() function; NFC.

Slight change so this is actually a NFC.

arsenm added a subscriber: arsenm.Apr 27 2017, 5:07 PM
arsenm added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
595 ↗(On Diff #97024)

Can we move this to after ExpandISelPseudos at least? It's a problem that reserved registers are frozen this early

MatzeB added inline comments.Apr 27 2017, 5:20 PM
lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
595 ↗(On Diff #97024)

I think that should work, we certainly could try. Maybe rename ExpandISelPseudos to FinalizeISel then?

However I'd like to do that in a separate patch then, to avoid complications when backporting this to a production branches.

arsenm added inline comments.Apr 27 2017, 5:24 PM
lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
595 ↗(On Diff #97024)

Last time I tried to do that a few PPC tests broke, although that might have been fixed since then

MatzeB added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
595 ↗(On Diff #97024)

Just tried it https://reviews.llvm.org/D32628 and it seems to work (at least for PowerPC). So if you need this feel free to grab it and finish it up (more testing, changing tests to adjust for the changed passname, ask GlobalISel people whether is is fine to freeze registers later).

rnk accepted this revision.Apr 28 2017, 11:41 AM

lgtm

This revision is now accepted and ready to land.Apr 28 2017, 11:41 AM
rnk added a comment.Apr 28 2017, 11:43 AM

Oops, I went straight to the diff and missed Matt's comment.

rnk requested changes to this revision.Apr 28 2017, 11:43 AM
This revision now requires changes to proceed.Apr 28 2017, 11:43 AM
arsenm accepted this revision.Apr 28 2017, 11:46 AM

LGTM

In D32621#741035, @rnk wrote:

Oops, I went straight to the diff and missed Matt's comment.

Does that mean you want to see Matts comment addressed (I think we both agreed to do that in a separate patch), or just need more time for review?

rnk accepted this revision.Apr 28 2017, 12:34 PM
In D32621#741035, @rnk wrote:

Oops, I went straight to the diff and missed Matt's comment.

Does that mean you want to see Matts comment addressed (I think we both agreed to do that in a separate patch), or just need more time for review?

No, I looked at the other patch and that seems fine. Looks good!

This revision is now accepted and ready to land.Apr 28 2017, 12:34 PM
This revision was automatically updated to reflect the committed changes.