Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Differential D45289

Disable -fmerge-all-constants as default.
ClosedPublic
Actions

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

Details

Reviewers
rjmccall
rsmith
chandlerc
Commits
rG3bc2d0a6e096: Merging r329300:
rG4b3eefa5e844: Disable -fmerge-all-constants as default.
rL333564: Merging r329300:
rL329300: Disable -fmerge-all-constants as default.
rC329300: Disable -fmerge-all-constants as default.
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

Event Timeline

manojgupta created this revision.Apr 4 2018, 2:24 PM
Harbormaster completed remote builds in B16735: Diff 141057.Apr 4 2018, 2:25 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

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.

manojgupta added a comment.Apr 4 2018, 3:05 PM

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?

rjmccall added a comment.Apr 4 2018, 3:25 PM

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

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

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
Closed by commit rC329300: Disable -fmerge-all-constants as default. (authored by manojgupta). · Explain WhyApr 5 2018, 8:35 AM
Closed by commit rL329300: Disable -fmerge-all-constants as default. (authored by manojgupta). · Explain Why
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: llvm-commits. · View Herald TranscriptApr 5 2018, 8:35 AM

Revision Contents
Changeset List

PathSize
include/
clang/
Driver/
5 lines
lib/
AST/
3 lines
Driver/
ToolChains/
6 lines
Frontend/
2 lines
test/
CodeGen/
11 lines
2 lines
CodeGenCXX/
2 lines
2 lines
4 lines
CodeGenObjCXX/
2 lines
Driver/
8 lines

Diff 141081

View Options

include/clang/Driver/Options.td

Loading...
View Options

lib/AST/ExprConstant.cpp

Loading...
View Options

lib/Driver/ToolChains/Clang.cpp

Loading...
View Options

lib/Frontend/CompilerInvocation.cpp

Loading...
View Options

test/CodeGen/array-init.c

Loading...
View Options

test/CodeGen/decl.c

Loading...
View Options

test/CodeGenCXX/const-init-cxx11.cpp

Loading...
View Options

test/CodeGenCXX/cxx0x-initializer-references.cpp

Loading...
View Options

test/CodeGenCXX/cxx0x-initializer-stdinitializerlist.cpp

Loading...
View Options

test/CodeGenObjCXX/arc-cxx11-init-list.mm

Loading...
View Options

test/Driver/clang_f_opts.c

Loading...