This is an archive of the discontinued LLVM Phabricator instance.

Add support for LTO options
ClosedPublic

Authored by rdhindsa on Apr 4 2018, 11:17 AM.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

rdhindsa created this revision.Apr 4 2018, 11:17 AM
ruiu added a comment.Apr 4 2018, 11:23 AM

Generally looking good.

lld/ELF/Driver.cpp
660–663

Please sort alphabetically.

lld/ELF/LTO.cpp
108

Do you need this if? I thought you could copy a string even if it is empty.

ruiu added a comment.Apr 4 2018, 11:30 AM

By the way, is there any way to write a test for the new flags?

pcc added a reviewer: pcc.Apr 4 2018, 12:16 PM
pcc added a comment.Apr 4 2018, 1:12 PM

By the way, is there any way to write a test for the new flags?

It should be possible. For new-pass-manger see llvm/test/tools/llvm-lto2/X86/pipeline.ll and for sample-profile see llvm/test/tools/gold/X86/thinlto_afdo.ll

rdhindsa updated this revision to Diff 141209.Apr 5 2018, 1:23 PM

Added tests for options: sample-profile and new-pass-manager.

rdhindsa marked 2 inline comments as done.Apr 5 2018, 1:24 PM
ruiu added inline comments.Apr 5 2018, 1:36 PM
lld/ELF/Config.h
135

Remove the default value because it is always overwritten.

lld/ELF/Driver.cpp
661

hasFlag is for a boolean flag such as --foo and --no-foo, and you are supposed to use this as

Args.hasFlag(OPT_foo, OPT_no_foo, default-value-which-is-true-or-false)

So this is a misuse of the function. The first and the second should be different. You should use Args.hasArg instead.

lld/ELF/LTO.cpp
110

I wouldn't add this comment because this is obvious from the code.

lld/ELF/Options.td
398

HelpText is displayed as part of the --help message,

$ ld.lld --help
...
--as-needed             Only set DT_NEEDED for shared libraries if used
--auxiliary <value>     Set DT_AUXILIARY field to the specified name
--Bdynamic              Link against shared libraries
--Bshareable            Build a shared object
...

"New pass manager" doesn't start with a verb, and it is not appropriate as a help message. "Use new pass manager" is better.

lld/test/ELF/lto/Inputs/sample.prof
1 ↗(On Diff #141209)

What is this file? Is there any way to avoid checking in a binary file?

rdhindsa updated this revision to Diff 141218.Apr 5 2018, 1:56 PM
rdhindsa marked 3 inline comments as done.Apr 5 2018, 2:00 PM
rdhindsa added inline comments.
lld/test/ELF/lto/Inputs/sample.prof
1 ↗(On Diff #141209)

This is the file for input profile to be used with sample-profile option. Here, it is an input to sample-profile.ll test. For now, it is empty as we just want to test the option.

ruiu added inline comments.Apr 5 2018, 2:02 PM
lld/test/ELF/lto/Inputs/sample.prof
1 ↗(On Diff #141209)

If an empty file suffices, please use /dev/null instead of a real empty file.

rdhindsa updated this revision to Diff 141222.Apr 5 2018, 2:21 PM
rdhindsa marked an inline comment as done.
ruiu added a comment.Apr 5 2018, 3:28 PM

Peter, are you fine with this change?

lld/test/ELF/lto/new-pass-manager.ll
4–5 ↗(On Diff #141222)

Don't break this line.

pcc added inline comments.Apr 5 2018, 3:38 PM
lld/test/ELF/lto/new-pass-manager.ll
7 ↗(On Diff #141222)

You don't actually need to pass --plugin-opt=thinlto here. The flag has no effect.

I would also do something like D45293 to make sure that we are actually using the new pass manager.

rdhindsa updated this revision to Diff 141414.Apr 6 2018, 1:30 PM
rdhindsa edited the summary of this revision. (Show Details)
rdhindsa set the repository for this revision to rLLD LLVM Linker.
rdhindsa added inline comments.
lld/test/ELF/lto/new-pass-manager.ll
7 ↗(On Diff #141222)

Added new-pass-manager option to check if Module Pass manager is run.

pcc accepted this revision.Apr 6 2018, 2:39 PM

LGTM

lld/ELF/LTO.cpp
108–110

Nit: I'd move these lines to above line 104 where the rest of the Conf.foo = bar; lines are.

This revision is now accepted and ready to land.Apr 6 2018, 2:39 PM
This revision was automatically updated to reflect the committed changes.