This is an archive of the discontinued LLVM Phabricator instance.

Disable -fmerge-all-constants as default.
ClosedPublic

Authored by manojgupta on Apr 4 2018, 2:24 PM.

Details

Summary

"-fmerge-all-constants" is a non-conforming optimization and should not
be the default. It is also causing miscompiles when building Linux
Kernel (https://lkml.org/lkml/2018/3/20/872).

Fixes PR18538.

Diff Detail

Repository
rL LLVM

Event Timeline

manojgupta created this revision.Apr 4 2018, 2:24 PM
rsmith accepted this revision.Apr 4 2018, 2:30 PM

One minor change, then this looks good to me. Thank you!

include/clang/Driver/Options.td
1286 ↗(On Diff #141057)

Remove the Flags<[CC1Option]> here, since -cc1 no longer understands this flag.

This revision is now accepted and ready to land.Apr 4 2018, 2:30 PM
chandlerc accepted this revision.Apr 4 2018, 2:44 PM

Just wanted to explicitly say +1 to this default change (with the adjustment Richard suggested).

Also, John already said +1 to this default change on the bug.

Thanks a lot for the quick review!
There a bunch of broken tests because of different IR generated. Is it preferable to fix them individually or add the option -fmerge-all-constants to clang arguments to avoid figuring out changes needed to fix the tests?

It's not unreasonable to just add -fmerge-all-constants to the command line invocations for the individual tests, yeah. We can take those off as necessary later.

manojgupta updated this revision to Diff 141075.Apr 4 2018, 3:31 PM

Removed unused CC1 option and updated broken tests.

rjmccall added inline comments.Apr 4 2018, 3:40 PM
include/clang/Driver/Options.td
1286 ↗(On Diff #141057)

Did you mean to remove the help text here? It's still a meaningful driver option.

manojgupta updated this revision to Diff 141081.Apr 4 2018, 3:59 PM

Bring back Help Text for "-fno-merge-all-constants"

manojgupta added inline comments.Apr 4 2018, 4:02 PM
include/clang/Driver/Options.td
1286 ↗(On Diff #141057)

I agree that it is meaningful and also useful to have the help text around. I had removed it since previously "fmerge-all-constants" didn't have it.

srhines added a subscriber: srhines.Apr 4 2018, 4:16 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.