Page MenuHomePhabricator

[Clang][LLVMGold] Passing LLVM arguments to gold plugin
Needs ReviewPublic

Authored by bunty2020 on May 19 2016, 5:04 AM.

Details

Reviewers
tejohnson
Summary

Currently CLANG does not pass LLVM options to gold plugin.
Ex: -mllvm <some-option> is not passed on to gold plugin.
This patch enables passing LLVM arguments to gold plugin

Diff Detail

Event Timeline

bunty2020 updated this revision to Diff 57764.May 19 2016, 5:04 AM
bunty2020 retitled this revision from to Passing LLVM arguments to gold plugin.
bunty2020 updated this object.
bunty2020 added a reviewer: tejohnson.
bunty2020 retitled this revision from Passing LLVM arguments to gold plugin to [Clang][LLVMGold] Passing LLVM arguments to gold plugin.May 19 2016, 5:08 AM
tejohnson edited edge metadata.May 19 2016, 6:01 AM
tejohnson added a subscriber: tejohnson.

Please add llvm-commits as a subscriber for all patches, so that the patch
and review gets posted to the mailing list. I've cc'ed llvm-commits here,
which might be enough to get llvm-commits added.

tejohnson edited edge metadata.May 19 2016, 6:02 AM
tejohnson added a subscriber: llvm-commits.
tejohnson added inline comments.May 19 2016, 6:11 AM
lib/Driver/Tools.cpp
8371

Should be moved to AddGoldPlugin called just above.

Seems reasonable to pass these along to the plugin, rather than require the user to know to pass them via -Wl,-plugin-opt.

bunty2020 updated this revision to Diff 57813.May 19 2016, 9:53 AM

Moved changes to AddGoldPlugin function

bunty2020 marked an inline comment as done.May 19 2016, 10:03 AM
This comment was removed by bunty2020.
bunty2020 added inline comments.May 19 2016, 10:04 AM
lib/Driver/Tools.cpp
8377

Moved to AddGoldPlugin

Please add a test to test/Driver/gold-lto.c.

pcc added a subscriber: pcc.May 19 2016, 10:37 AM

Plugin-opt (and mllvm) aren't really meant for users, I'm not sure that we
need to be making them easier to specify like that.

Peter
bunty2020 updated this revision to Diff 57813.
bunty2020 added a comment.

Moved changes to AddGoldPlugin function

http://reviews.llvm.org/D20423

Files:

lib/Driver/Tools.cpp

Index: lib/Driver/Tools.cpp

  • lib/Driver/Tools.cpp

+++ lib/Driver/Tools.cpp
@@ -1923,6 +1923,10 @@

  else
    CmdArgs.push_back("-plugin-opt=-debugger-tune=gdb");
}

+ for (const Arg *A : Args.filtered(options::OPT_mllvm)) {
+ CmdArgs.push_back(Args.MakeArgString("-plugin-opt="
+ + StringRef(A->getValue(0))));
+ }
}

/// This is a helper function for validating the optional refinement step


llvm-commits mailing list
llvm-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

In D20423#434665, @pcc wrote:

Plugin-opt (and mllvm) aren't really meant for users, I'm not sure that we
need to be making them easier to specify like that.

True but from a developer's standpoint this increases usability quite a bit. We're passing one developer option to another to make the handling of the -mllvm developer option uniform with and without -flto.

pcc added a comment.May 19 2016, 11:26 AM
In D20423#434665, @pcc wrote:

Plugin-opt (and mllvm) aren't really meant for users, I'm not sure that we
need to be making them easier to specify like that.

True but from a developer's standpoint this increases usability quite a bit. We're passing one developer option to another to make the handling of the -mllvm developer option uniform with and without -flto.

Okay, but It isn't really uniform though because the flag only works with gold (and not ld64, lld etc).

That said I don't feel too strongly about it, so while it would be nice if the flag worked with those linkers, that doesn't need to happen in this patch.

bunty2020 updated this revision to Diff 57899.May 19 2016, 11:06 PM

Added test in test/Driver/gold-lto.c

These are developer options, doing -Wl, explicitly seems appropriate to me.

In D20423#435268, @joker.eph wrote:

These are developer options, doing -Wl, explicitly seems appropriate to me.

As a developer it is easier if I don't need to change the way I pass the developer options between a regular -O2 and a "-O2 -flto" build. Also, it would be nice not to have change the way I pass the developer option between the -flto compile and link steps e.g. the "-flto -c foo.c" and "-flto foo.o" steps. Why not provide the added convenience for developers?

(The patch currently LGTM, but I would like to wait until there is agreement with joker.eph before I accept)

I'm don't have a strong opinion, let me ask what Duncan think of that.

rafael edited subscribers, added: cfe-commits; removed: llvm-commits.May 24 2016, 6:50 AM

This seems strange to me. For example, it breaks the otherwise very convenient:

$ clang -flto t.c -mllvm -some-internal-option-for-cc1

I don't understand how it's better.

Clang's help shows the following description:
-mllvm <value> Additional arguments to forward to LLVM's option processing

If -mllvm <some-option> sets some internal option for cc1 then its strange for front-end parser using some option which is supposed to be LLVM's option.

Clang's help shows the following description:
-mllvm <value> Additional arguments to forward to LLVM's option processing

If -mllvm <some-option> sets some internal option for cc1 then its strange for front-end parser using some option which is supposed to be LLVM's option.

cc1 is not just the front-end, it is the front-end, the optimizer, and the codegen (unless emitting bitcode).