Add -fpass-plugin option to Flang

[tarunprabhu]

Summary

This patch adds the -fpass-plugin option to flang which dynamically loads LLVM passes from the shared object passed as the argument to the flag. The behavior of the option is designed to replicate that of the same option in clang and thus has the same capabilities and limitations.

Features:

• Multiple instances of -fpass-plugin=path-to-file can be specified and each of the files will be loaded in that order.

• The flag can be passed to both flang-new and flang-new -fc1.

• The flag will be listed when the -help flag is passed to both flang-new and flang-new -fc1. It will also be listed when the --help-hidden flag is passed.

[awarzynski]

flang/docs/FlangDriver.md

513–514

Pass plugins are an extension to the frontend drive that make it possible to run out-of-tree LLVM passes on the LLVM-IR.

This isn't quite true.

1. These plugins can be loaded from both the compiler and frontend drivers.
2. Both out-of-tree and in-tree passes are supported.
3. This is not an extension to any of the drivers. The drivers that implement -fpass-plugin simply provide an interface to forward the specified plugin to LLVM (i.e. the middle-end).

515

516–517

See the documentation for llvm::PassBuilder for details

Please, can you provide a link?

519–520

The capitalization is inconsistent. Please use either "Flang" and "Clang" or "flang" and "clang". Usually, "Clang"/"Flang" refers to the sub-project and "flang"/"clang" to the binaries. Note that tools in this document are surrounded with backticks (i.e. "`").

flang/include/flang/Frontend/CodeGenOptions.h

48–51

DELETEME

49

DOCUMENTME :)

flang/test/Driver/frontend-forwarding.f90

4

This will really limit this test. I'd rather have it run on all supported platforms (with plugins enabled *and* disabled) than not.

flang/test/Driver/pass-plugin.f90

8–9

[tarunprabhu]

Added a sentence in the driver documentation stating that LLVM plugins are not officially supported on Windows.

Addressed other comments and issues.

[awarzynski]

Thanks for the update! Looks like per-commit CI is failing.

flang/docs/FlangDriver.md

515

It's probably worth specifying that this is the LLVM middle-end and that these passes are run by the optimisation rather than code-gen/backend pipelines (there are two).

521

[tarunprabhu]

Made it clear that dynamically loading passes from a pass plugin would add the pass to the optimization pipeline.

Addressed other issues.

The test is not failing for me, but I will look into it.

[tarunprabhu]

Potential fix to failing test. It seems that on some platforms, the output is `{anonymous}::Bye`` while on others it is `{anonymous namespace}::Bye``. The test has been fixed to allow for both. I was not able to test this locally, so will see if the buildbot is happy with it.

[tarunprabhu]

Potential fix to failing test.

It seems that on some platforms, the output is `{anonymous}::Bye`` while on others it is `{anonymous namespace}::Bye``. Hopefully, the buildbot will be happy with this change.

[tarunprabhu]

Used wrong bracket type in previous revision.

Fixed that test and use regular expressions to match the alternatives instead.

[tarunprabhu]

Potentially fixed regex to correctly match round brackets.

[awarzynski]

clang/lib/Driver/ToolChains/Flang.cpp

58–59

CLANGFORMATME

flang/test/Driver/pass-plugin.f90

11

[tarunprabhu]

Ran clang-format.

Fixed test to allow for any namespace prefix.

[tarunprabhu]

I noticed that clang-format changed several other lines in clang/lib/Driver/ToolChains/Flang.cpp. I undid those and only left in the line relevant to this patch. Do you think that those lines should be run on that file as a separate patch?

[awarzynski]

LGTM, many thanks for addressing all my comments and for seeing this through!

In D129156#3833501, @tarunprabhu wrote:

I noticed that clang-format changed several other lines in clang/lib/Driver/ToolChains/Flang.cpp. I undid those and only left in the line relevant to this patch. Do you think that those lines should be run on that file as a separate patch?

Up to you :) In general, it's good to keep the noise to a minimum. For things like this I would just push a separate patch without a review. Such patches implement non-functional-changes, are rather straightforward and very desirable (they help to maintain the code in good health).

Let me know if you need somebody to merge this for you.

[tarunprabhu]

Thanks for the review and the comments. :-)

[clementval]

I get a failure since this commit. Could not load library By.so ... cannot open shared object file: No such file or directory. Maybe a missing dependency?

[tarunprabhu]

I think there is a typo somehow. It should be "Bye.so". Not sure how that happened. I'm looking into this.

[clementval]

In D129156#3836768, @tarunprabhu wrote:

I think there is a typo somehow. It should be "Bye.so". Not sure how that happened. I'm looking into this.

I probably didn't copy the error correctly .. this is the correct libs that is not found llvm-project/build/lib/Bye.so

[tarunprabhu]

In D129156#3836838, @clementval wrote:

I probably didn't copy the error correctly .. this is the correct libs that is not found llvm-project/build/lib/Bye.so

I need some help with this.

The failing build is building flang out-of-tree. It looks like %llvmshlibdir/ does not get set correctly in this case. Bye.so is located in %llvmshlibdir which is within the LLVM build directory.

Do you know if there is a way to tell the test-suite about %llvmshlibdir?

[MatsPetersson]

In D129156#3836895, @tarunprabhu wrote:

In D129156#3836838, @clementval wrote:

I probably didn't copy the error correctly .. this is the correct libs that is not found llvm-project/build/lib/Bye.so

I need some help with this.

The failing build is building flang out-of-tree. It looks like %llvmshlibdir/ does not get set correctly in this case. Bye.so is located in %llvmshlibdir which is within the LLVM build directory.

Do you know if there is a way to tell the test-suite about %llvmshlibdir?

I'm fairly sure it is not a "out of tree" build, I'm seeing the same problem building in tree.
As far as I can see, there's nothing creatubg a "Bye.so" plugin. Maybe I'm missing something tho'

[DavidTruby]

This is also causing phabricator's pre-commit CI builds to fail, for example this build for my patch: https://buildkite.com/llvm-project/premerge-checks/builds/115370#0183a8a5-6684-4602-952f-81bde55220ea
Could we revert this until it is fixed? I think the CI passing is quite important.

[awarzynski]

I suspect that this fails when running ninja check-flang, right?

Most likely Bye needs to be added as a dependency for Flang tests, something akin to this. Alternatively, try adding examples to ! REQUIRES: in pass-plugin.f90.

[MatsPetersson]

In D129156#3837709, @awarzynski wrote:

I suspect that this fails when running ninja check-flang, right?

Most likely Bye needs to be added as a dependency for Flang tests, something akin to this. Alternatively, try adding examples to ! REQUIRES: in pass-plugin.f90.

As this seems to be breaking the testing on the public servers, as well as locally on my machine, I've reverted the commit for now. Please feel free to submit an updated version, this is not a criticism of the work as such, just trying to keep the "build & test" green.

[clementval]

As @awarzynski mentioned, it is likely a missing dependency on the test.

[tarunprabhu]

Added examples to REQUIRES in `test/Driver/pass-plugin.f90.

I still cannot reproduce the build failure on my end. @MatsPetersson tested this patch and the tests passed. Could someone else test this on their build and let me know if it works - especially if the previous version failed for you.

[tarunprabhu]

I tried an out-of-tree flang build. check-flang passes in that case too.

[awarzynski]

In D129156#3847396, @tarunprabhu wrote:

Added examples to REQUIRES in `test/Driver/pass-plugin.f90.

Thanks for the update!

I still cannot reproduce the build failure on my end. @MatsPetersson tested this patch and the tests passed.

@MatsPetersson & @clementval , could you share you build command so that the failure can be reproduced before this re-lands?

Could someone else test this on their build and let me know if it works - especially if the previous version failed for you.

I can try as soon as folks share their build commands. In general, I'm concerned that this fix is not enough. It merely makes sure that "pass-plugin.f90" is only run when LLVM_BUILD_EXAMPLES is set (or whatever other flag that sets examples in LIT). However, that's not sufficient to make sure that libBye.so is built when running ninja check-flang. I might wrong though - have you checked this?

[clementval]

In D129156#3848692, @awarzynski wrote:

In D129156#3847396, @tarunprabhu wrote:

Added examples to REQUIRES in `test/Driver/pass-plugin.f90.

Thanks for the update!

I still cannot reproduce the build failure on my end. @MatsPetersson tested this patch and the tests passed.

@MatsPetersson & @clementval , could you share you build command so that the failure can be reproduced before this re-lands?

I shared it with @tarunprabhu and I think the only major difference is the use of Ninja vs. Unix Makefiles.

cmake \
  -G "Unix Makefiles" \
  -DCMAKE_BUILD_TYPE=Release \
  -DLLVM_ENABLE_ASSERTIONS=ON \
  -DLLVM_TARGETS_TO_BUILD=host \
  -DLLVM_ENABLE_PROJECTS="clang;mlir;flang" \
  -DLLVM_ENABLE_RUNTIMES="compiler-rt"

[awarzynski]

In D129156#3848704, @clementval wrote:

I still cannot reproduce the build failure on my end. @MatsPetersson tested this patch and the tests passed.

@MatsPetersson & @clementval , could you share you build command so that the failure can be reproduced before this re-lands?

I shared it with @tarunprabhu and I think the only major difference is the use of Ninja vs. Unix Makefiles.

cmake \
  -G "Unix Makefiles" \
  -DCMAKE_BUILD_TYPE=Release \
  -DLLVM_ENABLE_ASSERTIONS=ON \
  -DLLVM_TARGETS_TO_BUILD=host \
  -DLLVM_ENABLE_PROJECTS="clang;mlir;flang" \
  -DLLVM_ENABLE_RUNTIMES="compiler-rt"

Thanks Valentin! Switching between generators will definitely change the order in which libraries will built (and, AFAIK, the order is non-deterministic to begin with). I will try to experiment later today.

If anyone manages to test this in the meantime, please share here - that's part of the public review process :)

[Leporacanthicus]

@MatsPetersson & @clementval , could you share you build command so that the failure can be reproduced before this re-lands?

I can't share the same command that I used, because it's some old CMAKE command that I no longer have in my history. But I tested this patch, and it works in the same tree that I was using it last week.

I just double-checked again in my release-build directory, and it also works. So I think the latest version works...

[tarunprabhu]

In D129156#3848728, @awarzynski wrote:

Thanks Valentin! Switching between generators will definitely change the order in which libraries will built (and, AFAIK, the order is non-deterministic to begin with). I will try to experiment later today.

I ran this with Unix Makefiles as the generator and it passes there too.

What I did notice when I tried the out-of-tree build of flang is that the test did not run. The message I got was that the test was "unsupported".

I assume that is because there is no dependence on LLVM examples in an out-of-tree build. Would adding an explicit dependence on LLVM's examples cause the test to run? I imagine that we want the feature to be tested even when building flang out-of-tree.

[awarzynski]

In @clementval 's CMake invocation LLVM_BUILD_EXAMPLES is not set, which means that the examples (e.g. the Bye plugin) are not built. Adding ! REQUIRES: examples to the test should fix the failure in this case. I did verify locally.

However, the pre-commit CI that was failing did include the examples:

cmake ../llvm -D LLVM_ENABLE_PROJECTS="mlir;flang;clang;llvm" -G Ninja -D CMAKE_BUILD_TYPE=Release -D LLVM_ENABLE_ASSERTIONS=ON -D LLVM_BUILD_EXAMPLES=ON -D LLVM_LIT_ARGS="-v --xunit-xml-output test-results.xml" -D LLVM_ENABLE_LLD=ON -D CMAKE_CXX_FLAGS=-gmlt -DBOLT_CLANG_EXE=/usr/bin/clang
ninja check-flang

You can see there that libBye.so is not built. Could you try that command, pls?

In D129156#3849487, @tarunprabhu wrote:

I imagine that we want the feature to be tested even when building flang out-of-tree.

I don't built standalone LLVM Flang, so won't be needing it. Perhaps somebody else will, dunno :)

[awarzynski]

@tarunprabhu Could you confirm that with the build command above "pass-plugin.f90" is failing for you? It is for me.

In order to fix that, you will have to add this in Flang's test/CMakeLists.txt. After the change, libBye.so will be added as a dependency to Flang tests, but only when FLANG_BUILD_EXAMPLES is set. I would also add a note in "pass-plugin.f90" that it depends on the "Bye" pass from LLVM.

[tarunprabhu]

In D129156#3851843, @awarzynski wrote:

@tarunprabhu Could you confirm that with the build command above "pass-plugin.f90" is failing for you? It is for me.

@awarzynski What compiler do you use to build this? gcc doesn't seem to like -gmlt, clang-12 complains about -fuse-ld=lld (could be the clang installation on the machine on which I am building), and clang-13 causes a compile error.

[EDIT: Tag @awarzynski]

[awarzynski]

In D129156#3852553, @tarunprabhu wrote:

In D129156#3851843, @awarzynski wrote:

@tarunprabhu Could you confirm that with the build command above "pass-plugin.f90" is failing for you? It is for me.

@awarzynski What compiler do you use to build this? gcc doesn't seem to like -gmlt, clang-12 complains about -fuse-ld=lld (could be the clang installation on the machine on which I am building), and clang-13 causes a compile error.

[EDIT: Tag @awarzynski]

I doubt those particular flags matter here. You can trim that CMake command as follows:

cmake ../llvm -D LLVM_ENABLE_PROJECTS="mlir;flang;clang;llvm" -G Ninja -D CMAKE_BUILD_TYPE=Release -D LLVM_ENABLE_ASSERTIONS=ON -D LLVM_BUILD_EXAMPLES=ON 
ninja check-flang

[tarunprabhu]

In D129156#3851843, @awarzynski wrote:

@tarunprabhu Could you confirm that with the build command above "pass-plugin.f90" is failing for you? It is for me.

The tests still passed. Some relevant output from the test is below (I added the -DLLVM_LIT_ARGS="-v" to the configure line).

UNSUPPORTED: Flang :: Driver/pass-plugin.f90 (1721 of 1809)
PASS: Flang :: Driver/pass-plugin-not-found.f90 (560 of 1809)

Testing Time: 4.81s
  Unsupported      :   11
  Passed           : 1780
  Expectedly Failed:   18

I am not sure what I am doing differently. This is starting from an empty build directory.

[awarzynski]

In D129156#3853045, @tarunprabhu wrote:

The tests still passed.

No, it wasn't run. We need to understand why before proceeding.

[tarunprabhu]

Added a dependence for the flang tests on LLVM's Bye plugin. This results in the Bye plugin being built when check-flang is run, even if building examples has been explicitly disabled.

Removed the dependency on 'examples' in the pass-plugin.f90 test.

Some minor changes to function names for better consistency.

[awarzynski]

flang/test/CMakeLists.txt

66

IIUC, Bye is only needed when plugins are enabled.

[tarunprabhu]

Only add a dependence to the Bye plugin when plugins are enabled.

[tarunprabhu]

Previous revision was missing the tests.

Only add a dependence to the Bye plugin when plugins are enabled.

[awarzynski]

Please be aware of https://reviews.llvm.org/D137673 - you may need to rebase if it lands before this patch.

Please just go ahead and merge, but please keep WIN32 in the final version of "flang/test/CMakeLists.txt" (see my comment inline).

LGTM

flang/test/CMakeLists.txt

66

IIUC, WIN32 is still required. Sorry for not being clearer earlier.

[tarunprabhu]

Yes, I am aware of the other patch which also adds test/Driver/pass-plugin.f90. I will keep an eye out for it.

flang/test/CMakeLists.txt

66

Ugh! Yes, of course. I looked at llvm/CMakeLists.txt and somehow concluded that LLVM_ENABLE_PLUGINS is disabled if WIN32, but it is in fact the _default value_ of LLVM_ENABLE_PLUGINS that is set to false in that case.

[ro]

This introduced a new failure on Solaris:

FAIL: Flang :: Driver/pass-plugin-not-found.f90

Running the failing command manually shows:

error: unable to load plugin 'X.Y': 'Could not load library 'X.Y': ld.so.1: flang-new: X.Y: open failed: No such file or directory'

while the test currently expects

error: unable to load plugin 'X.Y': 'Could not load library 'X.Y': X.Y: cannot open shared object file: No such file or directory'

This expectation is unjustified, I believe: the message is produced by PassPlugin::Load -> sys::DynamicLibrary::getPermanentLibrary -> HandleSet::DLOpen -> ::dlerror. Obviously the output of the last is system-dependent; I don't think we can put any requirements on it (maybe not even the exact wording of the No such file or directory part.

[awarzynski]

In D129156#3949156, @ro wrote:

This introduced a new failure on Solaris:

FAIL: Flang :: Driver/pass-plugin-not-found.f90

Running the failing command manually shows:

error: unable to load plugin 'X.Y': 'Could not load library 'X.Y': ld.so.1: flang-new: X.Y: open failed: No such file or directory'

while the test currently expects

error: unable to load plugin 'X.Y': 'Could not load library 'X.Y': X.Y: cannot open shared object file: No such file or directory'

This expectation is unjustified, I believe: the message is produced by PassPlugin::Load -> sys::DynamicLibrary::getPermanentLibrary -> HandleSet::DLOpen -> ::dlerror. Obviously the output of the last is system-dependent; I don't think we can put any requirements on it (maybe not even the exact wording of the No such file or directory part.

Sorry that you are hitting this - things like this happen, sadly. I think that the easiest to resolve it would be to tweak the expected error so that it works on Solaris as well as other popular platforms. Given that you might be the only person able to test on Solaris, would you be able to upload something for a review? Thanks!

[ro]

In D129156#3949194, @awarzynski wrote:

Sorry that you are hitting this - things like this happen, sadly. I think that the easiest to resolve it would be to tweak the expected error so that it works on Solaris as well as other popular platforms. Given that you might be the only person able to test on Solaris, would you be able to upload something for a review? Thanks!

No worries, especially given that this seems to be the first test ever to check the Could not load library error message.

I've just submitted a patch that works on Linux and Solaris (all that I can easily test): D138663.