This is an archive of the discontinued LLVM Phabricator instance.

[X86] Enable shrink-wrapping by default
ClosedPublic

Authored by qcolombet on Oct 28 2015, 1:11 PM.

Details

Summary

Hi,

This patch enables shrink-wrapping by default for x86.
Both Chandler and I have tested shrink-wrapping and no correctness problems came out or runtime regressions.
Although the gain may be small, the code looks nicer with shrink-wrapping enabled and the performance could just be better.

Finally, I’ve run the gdb test suite with and without shrink-wrapping enabled for an optimized compiler and the results were identical.

For the record shrink-wrapping was introduced after this review:
http://reviews.llvm.org/D9210

More information on Shrink-Wrapping:
http://lists.llvm.org/pipermail/llvm-dev/2015-May/085220.html

OK to commit?

Thanks,
-Quentin

Diff Detail

Repository
rL LLVM

Event Timeline

qcolombet updated this revision to Diff 38681.Oct 28 2015, 1:11 PM
qcolombet retitled this revision from to [X86] Enable shrink-wrapping by default.
qcolombet updated this object.
qcolombet added reviewers: chandlerc, RKSimon, spatel, nadav.
qcolombet set the repository for this revision to rL LLVM.
qcolombet added a subscriber: llvm-commits.

For the record, the changes in the tests are because shrink-wrapping moves the related code into if blocks.

Hi David,

https://llvm.org/bugs/show_bug.cgi?id=24193 is still outstanding.

Oh that’s right, I forgot about this one.
An easy fix would be to disallow that for win64 for now.
Anyway, I’ll look into it.

Thanks for reminding me!

Q.

qcolombet updated this revision to Diff 39262.Nov 4 2015, 2:47 PM

Update patch after PR24193 is fixed, i.e., now the Win64 test case did not show any changes.

Alright, PR24193 is now fixed.

Is it OK to commit?

Thanks,
-Quentin

spatel accepted this revision.Nov 11 2015, 9:55 AM
spatel edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Nov 11 2015, 9:55 AM

Thanks Sanjay and Nadav!

I am holding on the commit, it seems enabling shrink-wrapping will cause problem with EH emission and although I believe the problem may not be shrink-wrapping, we will need to fix it first.

For more details, see
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20151109/312215.html

Cheers,
-Quentin

qcolombet closed this revision.Nov 18 2015, 4:46 PM

Alright, everything fixed.

Committed revision 253528.