Page MenuHomePhabricator

ignore -flto= options recognized by GCC
ClosedPublic

Authored by doko on Mar 29 2021, 6:21 AM.

Details

Summary

as requested in https://bugs.llvm.org/show_bug.cgi?id=49553, submitting the proposed changes to just ignore the -flto= options which are recognized by GCC ("auto" and "jobserver").

GCC supports -flto=<auto|jobserver|<N> to select the parallelity for LTO builds. LLVM also has -flto-jobs=<N>, which only seems to have a meaning when used with -flto=thin?

The attached patch just ignores the values "auto" and "jobserver". that doesn't change anything in functionality. Another option would be to map these values to either "thin" or "full", maybe in presence of the -ffat-lto-objects option?

-flto=<n> could also be translated to -flto-jobs=<N>.

Diff Detail

Event Timeline

doko created this revision.Mar 29 2021, 6:21 AM
doko requested review of this revision.Mar 29 2021, 6:21 AM

Please submit a patch with context. @mehdi_amini this looks good to me, any thoughts?

doko updated this revision to Diff 334248.Mar 30 2021, 1:00 PM

I don't know what's the best practice for gcc compatibility with respect to clang command line flags, adding Teresa and Richard here.

It looks like typically Clang will warn for unsupported gcc options. See clang_ignored_gcc_optimization_f_Group and its handling in Clang.cpp. I think it should be possible to simply mark these as clang_ignored_gcc_optimization_f_Group in Options.td. I.e. have an entry specifically for each of these, like:

def : Flag<["-"], "flto=auto">, Group<clang_ignored_gcc_optimization_f_Group>;
def : Flag<["-"], "flto=jobserver">, Group<clang_ignored_gcc_optimization_f_Group>;

doko added a comment.Apr 1 2021, 9:14 AM

do you mean, just discard the current patch, and just list those as unsupported?

do you mean, just discard the current patch, and just list those as unsupported?

Yes as an alternate approach that results in these being warned about (and ignored) as we seem to do for other unsupported gcc options.

doko updated this revision to Diff 334793.Apr 1 2021, 12:21 PM

discarded the first approach, and used the proposed approach. I added this close to the ignoring of the -ffat-lto-objects option. Please let me know if there's a better location. Builds and works as expected:

$ clang -flto=auto hello.c
clang: warning: optimization flag '-flto=auto' is not supported [-Wignored-optimization-argument]
$ echo $?
0

$ clang -flto=auto -ffat-lto-objects hello.c
clang: warning: optimization flag '-flto=auto' is not supported [-Wignored-optimization-argument]
clang: warning: optimization flag '-ffat-lto-objects' is not supported [-Wignored-optimization-argument]

$ clang -flto=jobserver -ffat-lto-objects hello.c
clang: warning: optimization flag '-flto=jobserver' is not supported [-Wignored-optimization-argument]
clang: warning: optimization flag '-ffat-lto-objects' is not supported [-Wignored-optimization-argument]

This revision is now accepted and ready to land.Apr 1 2021, 12:29 PM
This revision was landed with ongoing or failed builds.Apr 5 2021, 2:55 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2021, 2:55 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Tests missing.
This somehow wasn't sent to cfe-commits list

fhahn added a subscriber: fhahn.Apr 8 2021, 2:56 AM

Tests missing.
This somehow wasn't sent to cfe-commits list

@doko could you add a test? e.g. in clang/test/Driver/clang_f_opts.c, which already contains a check for -ffat-lto-objects

doko added a comment.Apr 9 2021, 5:51 AM

here in the same place, or in a different merge request?

fhahn added a comment.Apr 9 2021, 5:58 AM

here in the same place, or in a different merge request?

This one is closed now I think, so a new review would be ideal.

Just approved the test case patch. Sorry I missed the lack of test on this!

Just approved the test case patch. Sorry I missed the lack of test on this!

Great, thanks!