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

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 3 02:19:32 PDT 2017


grimar added a comment.

In https://reviews.llvm.org/D36227#830068, @davide wrote:

> 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.


https://reviews.llvm.org/D36227





More information about the llvm-commits mailing list