This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Support autoloading vendor-defined plugins
Needs ReviewPublic

Authored by mgorny on Aug 5 2020, 6:30 AM.

Details

Summary

Provide a new LLVM_AUTOLOAD_PLUGINS CMake variable to specify plugins
that LLVM should attempt to load automatically. This is going
to be used to make it possible for LLVM to automatically take advantage
of installed Polly plugin without requiring it to be unconditionally
present at build or runtime.

Diff Detail

Event Timeline

mgorny created this revision.Aug 5 2020, 6:30 AM
Herald added a project: Restricted Project. · View Herald Transcript
mgorny requested review of this revision.Aug 5 2020, 6:30 AM

[DriveBy] We should test this ;)

Adding @serge-sans-paille as he designed the latest plugin loading mechanism.

The new pass manager uses a separate mechanism using -fpass-plugin/-load-pass-plugin. Maybe that should be supported as well.

I think that some community members try to reduce the number of build configurations to reduce testing effort/improve reproducibility. This would run against that.

mgorny added a comment.Aug 7 2020, 7:54 AM

I don't really know about the two plugin mechanisms. To be honest, I haven't been able to get Polly to work yet — however, one of our users has done some testing for me, and IIRC the 'pass plugin' mechanism didn't work for some reason.

If it is passes that (like Polly) using the in-tree static plugin mechanism, I recommend using the LLVM_<plugin>_LINK_INTO_TOOLS=ON flag.

For adding yet another mechanism I would request additional motivation. Maintaining that many plugin loading mechanism cross-platforms is a lot of work.

If it is passes that (like Polly) using the in-tree static plugin mechanism, I recommend using the LLVM_<plugin>_LINK_INTO_TOOLS=ON flag.

I don't see how that would work if Polly is built after LLVM, and entirely optional.

I have some difficulties to understand the exact scenario: if one knows at compile-time that polly is needed, why no just build it as a linked-in plugin, as suggested by @Meinersbur?
If one wants to specify a list of auto-loaded plugins at runtime, there's already several ways to do so, either through the C++ API or through CLI.

I have some difficulties to understand the exact scenario: if one knows at compile-time that polly is needed, why no just build it as a linked-in plugin, as suggested by @Meinersbur?

How does one know that? The whole point is to not have to know what's going to happen in the future, and not have to rebuild the whole LLVM to get Polly or get rid of it. That's pretty much the purpose of plugins, isn't it?

If one wants to specify a list of auto-loaded plugins at runtime, there's already several ways to do so, either through the C++ API or through CLI.

I don't think we have the same definition of 'auto-loaded'. The whole point is to have them autoloaded in all LLVM tools that might need plugins, *without* manual user effort, and in particular without requiring user to remember the exact paths or having to add wrappers for tools.

Meinersbur resigned from this revision.Aug 9 2020, 3:30 AM

The scenario sounds too specific for a single user requiring too much effort to maintain it across build configurations and platforms. Additionally, it makes clang's behavior dependent on the system configuration, which in the past the LLVM community was not in favor of. See http://lists.llvm.org/pipermail/cfe-dev/2016-September/050795.html.

There are a few other problems of this patch in particular:

  1. Errors silently ignored
  2. Does not handle new pass manager plugins
  3. LLVM and Polly must exactly match, otherwise unpredictable errors occur. Picking up a plugin at some locations the user is not necessarily aware of is problematic.
  4. Reproducibility is reduced: The output of clang -v doesn't even indicates that a pass is implicitly loaded.
  5. Enlarges the build configuration space.

At most it saves the system maintainer from re-compiling LLVM with(/-out) LLVM_POLLY_LINK_INTO_TOOLS=ON. Sorry, I don't see myself supporting this patch.

serge-sans-paille resigned from this revision.Mar 9 2021, 12:18 AM

I 100% agree with the above arguments