This is an archive of the discontinued LLVM Phabricator instance.

[lld] add options for context-sensitive PGO.
ClosedPublic

Authored by xur on Jan 14 2019, 10:33 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

xur created this revision.Jan 14 2019, 10:33 AM
ruiu added a subscriber: ruiu.Jan 14 2019, 10:42 AM

You can submit this as long as the main patch is approved and submitted.

ruiu added inline comments.Jan 14 2019, 10:45 AM
ELF/Options.td
472–475 ↗(On Diff #181590)

Actually, do you have to add these options? -plugin-opt=foo is an option for backward compatibility with gold. Since lld has a native LTO support, "plugin opt" doesn't make sense anymore. If they are new options, you shouldn't add -plugin-opt aliases.

xur marked an inline comment as done.Mar 5 2019, 10:56 AM
xur added inline comments.
ELF/Options.td
472–475 ↗(On Diff #181590)

Sorry I missed this comment.

I actually do use -plugin-opt options. If I remove these two options, I got errors in my test.
ld.lld: error: --plugin-opt: ld.lld: Unknown command line argument 'cs-profile-generate'.

The command line options are " .. -fcs-profile-generate=pass2 -flto=thin -fuse-ld=lld".

xur marked an inline comment as done.Mar 5 2019, 11:45 AM
xur added inline comments.
ELF/Options.td
472–475 ↗(On Diff #181590)

CSPGO options are handled in tools::AddGoldPlugin() (in lib/Driver/ToolChains/CommonArgs.cpp) for LTO/ThinLTO, and we add -plugin-opt there.
tools::AddGoldPlugin() is called regardless of what linker is used.
So I think lld needs to handle these -plugin-ops.

ruiu added a comment.Mar 11 2019, 2:44 PM

Sorry for the belated response. (I was on vacation last week.)

ELF/Driver.cpp
801–802 ↗(On Diff #181590)

Please sort ASCIIbetically.

ELF/Options.td
440–443 ↗(On Diff #181590)

Are these new options, or is there a linker that already have these options? If they are new, is there any reason to choose a new name lto-cs-pgo-gen instead of lto-cs-profile-generate?

xur updated this revision to Diff 190169.Mar 11 2019, 3:31 PM
xur marked 3 inline comments as done.

Integrated Ruiu's review comments.

Thanks,

-Rong

ruiu accepted this revision.Mar 11 2019, 3:36 PM

LGTM

ELF/Config.h
155 ↗(On Diff #190169)

Remove = false as the default value would always be overwritten.

This revision is now accepted and ready to land.Mar 11 2019, 3:36 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2019, 3:53 PM
dmajor added a subscriber: dmajor.Mar 12 2019, 10:18 AM

Are there any bugs/reviews/etc. that I can subscribe to for the implementation of this in COFF?

xur added a comment.Mar 12 2019, 10:50 AM

Are there any bugs/reviews/etc. that I can subscribe to for the implementation of this in COFF?

Feature of the main implementation is reviewed here:
https://reviews.llvm.org/D54175
The change has been committed.

I'm not familiar with COFF. But I think you can just do similar things as in ELF: it just passes the plugin-opts to backend compilation.

ELF/Options.td
440–443 ↗(On Diff #181590)

They are new options. I will change the name to -lto-cs-profile-generate.