This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - LTO: Try to be option compatible with the gold plugin.
ClosedPublic

Authored by grimar on Aug 2 2017, 9:34 AM.

Details

Summary

This is relative to PR30720.

Previously we ignored all --plugin-opt arguments.
Patch adds support for them.

Patch does not add any new LTO options,
and just implements mapping from --plugin-opt to existent ones.

I am not sure if we want to support any other --plugin-opt options.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Aug 2 2017, 9:34 AM
ruiu added inline comments.Aug 2 2017, 8:24 PM
ELF/Driver.cpp
621 ↗(On Diff #109363)

Rename getLTOValue.

623 ↗(On Diff #109363)

This function is unnecessarily too complicated. Don't mix Key with other plugin options. Handle Key first, and then if Key does not exist, handle others. I.e.

if (Args.hasArg(Key))
  return getInteger(Args, Key, Default);
...
davide requested changes to this revision.Aug 2 2017, 9:33 PM
davide added a subscriber: davide.

This patch lies in the limbo between fully supported and completely unsupported.
As such, I'm not sure I like it.
Previously, we just ignored every possible argument to --plugin-opt (or plugin-opt entirely).
I guess we should at least error out in case the argument can't be translated (or add a test for that).

This revision now requires changes to proceed.Aug 2 2017, 9:33 PM
grimar added a comment.Aug 3 2017, 2:19 AM

This patch lies in the limbo between fully supported and completely unsupported.
As such, I'm not sure I like it.

I was not sure how to support other ones and if we want to support it.
For example gold-plugin (which is my reference, https://github.com/llvm-mirror/llvm/blob/master/tools/gold/gold-plugin.cpp#L190)
supports --plugin-opt=mcpu=FOO, which then assigned to lto::Config::CPU field.
But LLD does not have separate command line option for setting that and it is not clear for me
if it is desirable. It is unclear also if we need separate option or can live with --option-opt=mcpu (in case
if we want to support it). In latter case I would move initialization of lto::Config struct fields to Driver.cpp
probably, but that is story for different patch.

Another exaple is "thinlto-index-only=" option, looking on implementation it does not set any Config field,
but do something else and I am not yet familar with gold plugin logic and so would prefer to implement it separatelly.

Because of all above I started from supporting things that we already have (and which are clear in part of implementation for me).
I think patch supports all LTO options that we have atm in command line and do that "at once for all", what is probably
good for start.

Previously, we just ignored every possible argument to --plugin-opt (or plugin-opt entirely).
I guess we should at least error out in case the argument can't be translated (or add a test for that).

Ok, I agree. Not sure about error, I think warning for start can be enough probably. And we can change it to error later
when it be clear which options we support/ignore and which not.

grimar added inline comments.Aug 3 2017, 4:30 AM
ELF/Driver.cpp
623 ↗(On Diff #109363)

If I do that then mix of options will work incorrectly, example:

ld.lld --lto-O0 --plugin-opt=O6 ...

will set optimization level 0 instead of 6

ruiu edited edge metadata.Aug 4 2017, 2:25 AM

No, you don't have to worry too much about the case where users use both gold plugin options and lld options.

grimar updated this revision to Diff 110158.EditedAug 8 2017, 2:52 AM
grimar edited edge metadata.

Reimplemented:

  • Ignore some options passed by gcc.
  • Error out on unsupported --plugin-opt= options.
  • Don't mix regular commandline and --plugin-opt commandline handling.
ruiu added inline comments.Aug 10 2017, 11:17 AM
ELF/Driver.cpp
614 ↗(On Diff #110158)

Please do not use function overloading unless you really need it. We already have the function with the same name, and that function is different from this. When you write a new function with a different meaning/semantics, you usually have to give it a new name.

You are not using Arg except for generating an error message. Please change the signature to

static int parseInt(StringRef S, opt::Arg *Arg)
635–637 ↗(On Diff #110158)

Negate the condition and remove continue.

725 ↗(On Diff #110158)

Inline this function.

ELF/Options.td
363 ↗(On Diff #110158)

80 cols

grimar updated this revision to Diff 110677.Aug 11 2017, 2:23 AM

*All review comments addressed.

ruiu added inline comments.Aug 11 2017, 7:43 AM
ELF/Driver.cpp
703 ↗(On Diff #110677)

plugin_opt_eq is an alias for plugin_opt. Do you have to pass both?

716 ↗(On Diff #110677)

What is '/'?

grimar added inline comments.Aug 11 2017, 7:59 AM
ELF/Driver.cpp
703 ↗(On Diff #110677)

I'll recheck.

716 ↗(On Diff #110677)

Line Rafael mentioned earlier:

"Note that gcc has be bad habit of always passing some options, so we
pretty much have to ignore those:
-plugin-opt=/usr/libexec/gcc/x86_64-redhat-linux/7/lto-wrapper
...
"

It is LTO wrapper which we should ignore:
https://stackoverflow.com/questions/19807107/what-is-gcc-lto-wrapper

grimar added inline comments.Aug 11 2017, 8:52 AM
ELF/Driver.cpp
703 ↗(On Diff #110677)

plugin_opt_eq is an alias for plugin_opt. Do you have to pass both?

Yes, so if you have --plugin-opt=aaa --plugin-opt bbb, then
Args.filtered(OPT_plugin_opt) will return only bbb and
Args.filtered(OPT_plugin_opt_eq) will return only aaa.

Not sure if it is a bug or by design of filtered(), but atm we need both it seems.

ruiu added inline comments.Aug 11 2017, 9:08 AM
ELF/Driver.cpp
703 ↗(On Diff #110677)

OK, maybe it cannot handle aliases correctly. It is OK for now.

710 ↗(On Diff #110677)

drop_front(N) is the same as substr(N), right? Please use substr.

grimar updated this revision to Diff 110838.Aug 12 2017, 7:57 AM
  • Used substr() instead of drop_front().
ruiu accepted this revision.Aug 12 2017, 3:16 PM

LGTM

This revision was automatically updated to reflect the committed changes.