This is an archive of the discontinued LLVM Phabricator instance.

Enable new passmanager plugin support for LTO.
ClosedPublic

Authored by efriedma on Mar 26 2020, 10:18 AM.

Details

Summary

This should make both static and dynamic NewPM plugins work with LTO. And as a bonus, it makes static linking of OldPM plugins more reliable for plugins with both an OldPM and NewPM interface.

I only implemented the command-line flag to specify NewPM plugins in llvm-lto2, to show it works. Support can be added for other tools later.

I'm not confident the dynamic plugin bits work the way they should, but it's at least enough to make the included test pass.

Diff Detail

Event Timeline

efriedma created this revision.Mar 26 2020, 10:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2020, 10:18 AM

I'd love a test similar to llvm/test/Feature/load_extension.ll in llvm/test/tools/llvm-lto !

The current version only supports static plugins. Supporting dynamic plugins is a more difficult puzzle because I'd have to figure out how to thread the option through... I'll give it a shot, I guess.

efriedma updated this revision to Diff 254662.Apr 2 2020, 6:12 PM
efriedma retitled this revision from Enable plugin support for LTO. to Enable new passmanager plugin support for LTO..
efriedma edited the summary of this revision. (Show Details)

Added support/tests for dynamically loaded NewPM LTO plugins. (Statically linked plugins can't be tested in regression tests because there are none by default.)

Depending on the review responses, I might split the parts necessary for static plugins into a separate patch, since dynamic plugins are a lot more complicated.

Statically linked plugins can't be tested in regression tests because there are none by default

Yeah, if you add

REQUIRES: plugins, examples

you have the guarantee that the `Bye plugin will be compiled. But by default it's compiled dynamically, and you need to set -DLLVM_BYE_LINK_INTO_TOOLS=ON` to have the static test. Your approach looks good.

llvm/test/Feature/load_extension.ll
1 ↗(On Diff #254662)

why?

llvm/test/lit.cfg.py
212 ↗(On Diff #254662)

typo: `%loadbyenewpm != %loadnewpmbye`

efriedma marked an inline comment as done.Apr 3 2020, 11:55 AM
efriedma added inline comments.
llvm/test/Feature/load_extension.ll
1 ↗(On Diff #254662)

Can't run LTO without specifying a target.

Maybe there's some way to get around that if we don't actually generate code; I can experiment a bit.

efriedma updated this revision to Diff 254881.Apr 3 2020, 12:09 PM

Fix review comment and lint

efriedma marked an inline comment as done.Apr 3 2020, 12:15 PM
efriedma added inline comments.
llvm/test/Feature/load_extension.ll
1 ↗(On Diff #254662)

Looked at this more. There isn't any easy solution here, but I guess there are a few options:

  1. Add a special mode to LTO where it doesn't require a target machine, and just spits out IR.
  2. Force the LLVM default target triple. But it's not clear we can reliably generate an object file with that.
  3. Force the LLVM default target triple, and add a mode to LTO where it doesn't run codegen.
llvm/examples/Bye/Bye.cpp
48 ↗(On Diff #254881)

Why did you change the extension point? Not that it's particularly important for that pass, just being curious.

llvm/lib/LTO/LTOBackend.cpp
268

That's the exact same lines as above. Maybe it's worth putting that in a function?

llvm/test/Feature/load_extension.ll
1 ↗(On Diff #254662)

ok, make sense if that's a LTO requirement. I don't think it significantly decreases the coverage of the test.

efriedma marked 2 inline comments as done.Apr 13 2020, 10:17 AM
efriedma added inline comments.
llvm/examples/Bye/Bye.cpp
48 ↗(On Diff #254881)

EP_EarlyAsPossible doesn't run for LTO.

llvm/lib/LTO/LTOBackend.cpp
268

Yes, probably; I'll factor it out.

ddcc added a subscriber: ddcc.Apr 13 2020, 12:26 PM
efriedma updated this revision to Diff 257072.Apr 13 2020, 12:46 PM

Address review comment

dblaikie added a subscriber: dblaikie.

Thanks @efriedma for the updates. This LGTM, we should probably be waiting for @chandlerc and/or @asbirlea and/or @dblaikie to povide additional opinion though?

This revision is now accepted and ready to land.Apr 13 2020, 9:56 PM

Thanks @efriedma for the updates. This LGTM, we should probably be waiting for @chandlerc and/or @asbirlea and/or @dblaikie to povide additional opinion though?

Was more just meant as an "FYI" to those folks. I thought/think Chandler might have some thoughts on plugin infrastructure for the NPM, but probably best to... plug on in the absence of that feedback & can come back to it if he wants to chime in at some point.

This revision was automatically updated to reflect the committed changes.

IIUC, this adds statically linked plugins as a link dependency to LLVMLTO. In https://bugs.llvm.org/show_bug.cgi?id=44870 we are currently discussing the downstream issues of these plugins to be added as dependency of clangCodeGen. Are similar problems to be expected for LLVMLTO? Maybe D78192 resolves both.

That sort of looks related, but I don't think it's precisely the same issue.

I think what's happening here is that when LLVM_LINK_LLVM_DYLIB is enabled, the static plugin somehow acquires a dependency on libLLVM.so. And that's obviously broken: if we expect static plugins to be used as part of LTO, they have to be linked into libLLVM.so, so they can't depend on it.

If we're building libLLVM.so, it probably makes sense to link static plugins into it. I'll have to mess with the CMake a bit to figure out how to make that work.