This is an archive of the discontinued LLVM Phabricator instance.

Fixes PR27241 by enabling the X86 call frame optimization for 64-bit targets
ClosedPublic

Authored by DavidKreitzer on Apr 28 2016, 2:09 PM.

Details

Summary

The source changes are fairly straightforward. The most interesting change is the new code at lines 489-499 that promotes a 32-bit value to 64 bits. I'd appreciate a careful review to make sure I am doing that correctly.

I would also appreciate any suggestions for improving the unit test. I modeled the test after the existing movtopush.ll that was written for 32-bit targets. I am testing all the same things, though naturally the details are different due to differences between the calling conventions.

Not surprisingly, the code size improvements are small compared to IA-32 due the in-register 64-bit calling convention. I measured 0.2% improvement across cpu2k. Performance is basically flat. There are further improvement opportunities, e.g. adding support for scheduling the pushes and relaxing the early bail-out at line 351.

Diff Detail

Repository
rL LLVM

Event Timeline

DavidKreitzer retitled this revision from to Fixes PR27241 by enabling the X86 call frame optimization for 64-bit targets.
DavidKreitzer updated this object.
DavidKreitzer added reviewers: rnk, mkuper, delena, hans.
DavidKreitzer added a subscriber: llvm-commits.
DavidKreitzer added inline comments.Apr 28 2016, 2:11 PM
lib/Target/X86/X86CallFrameOptimization.cpp
108 ↗(On Diff #55466)

In case it isn't immediately obvious, removing "const" here is necessary due to the calls to MRI->createVirtualRegister at lines 492-493.

rnk edited edge metadata.Apr 28 2016, 2:25 PM

I'm a little concerned that this will uncover broken unwinders, but we can find out.

test/CodeGen/X86/movtopush64.ll
2 ↗(On Diff #55466)

Can you also test that this is disabled on Darwin? You may need to modify the IR to add uwtable attributes, but I think that condition is worth testing.

hans edited edge metadata.Apr 28 2016, 5:19 PM

This looks great as far as I can tell! Thanks

DavidKreitzer edited edge metadata.

Thanks for the reviews, Reid and Hans!

I updated the test to check that the optimization works as expected for Darwin64, which means that it is allowed for routines that use a frame pointer and have no EH (so that the pushes don't require additional unwind info) but disallowed otherwise. The 32-bit test movtopush.ll could also use some Darwin specific testing. I can do that separately.

Reid, if you have any suggestions for how to find/fix broken unwinders prior to commit, I would be happy to try them.

DavidKreitzer added inline comments.Apr 29 2016, 7:45 AM
test/CodeGen/X86/movtopush64.ll
3 ↗(On Diff #55595)

The extra "-check-prefix=DARWIN" here was inadvertent. I'll remove it.

rnk accepted this revision.Apr 29 2016, 9:46 AM
rnk edited edge metadata.

lgtm

lib/Target/X86/X86CallFrameOptimization.cpp
494–495 ↗(On Diff #55595)

I'm not familiar enough with MI to say if this is the best way to express this, but you have tests for it and that gives me confidence that it works.

This revision is now accepted and ready to land.Apr 29 2016, 9:46 AM
mkuper edited edge metadata.Apr 29 2016, 10:01 AM

Thanks, Dave!

Re 32-bit Darwin testing, note that there's already some of that in push-cfi.ll. Perhaps it's worth just merging the tests.

Michael

This revision was automatically updated to reflect the committed changes.