Page MenuHomePhabricator

Add -fpass-plugin option to Flang
Needs ReviewPublic

Authored by tarunprabhu on Jul 5 2022, 11:06 AM.

Details

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.

Diff Detail

Unit TestsFailed

TimeTest
160 msx64 debian > Flang.Driver::pass-plugin.f90
Script: -- : 'RUN: at line 7'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/flang-new -S /var/lib/buildkite-agent/builds/llvm-project/flang/test/Driver/pass-plugin.f90 -fpass-plugin=/var/lib/buildkite-agent/builds/llvm-project/build/lib/Bye.so -Xflang -fdebug-pass-manager -o /dev/null 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/flang/test/Driver/pass-plugin.f90 --check-prefix=CHECK-PASS-PLUGIN

Event Timeline

tarunprabhu created this revision.Jul 5 2022, 11:06 AM
Herald added a project: Restricted Project. · View Herald Transcript
tarunprabhu requested review of this revision.Jul 5 2022, 11:06 AM

Updating the patch because the earlier version was missing some files.

Hello! Thanks for implementing this :)

I'm traveling this week, so only had a chance to skim through. I'm away next week, but will try to review properly before that. Btw, have you considered re-using LLVM's Hello pass for testing?

Btw, have you considered re-using LLVM's Hello pass for testing?

I wanted to also propose a similar option to write out-of-tree FIR (MLIR) passes. I thought that it might be instructive to have the same/similar functionality in an example plugin, an MLIR pass and an LLVM pass.

Finally had a chance to take a look. My comments below and inline. Again, thanks for working on this!

Btw, have you considered re-using LLVM's Hello pass for testing?

I wanted to also propose a similar option to write out-of-tree FIR (MLIR) passes. I thought that it might be instructive to have the same/similar functionality in an example plugin, an MLIR pass and an LLVM pass.

OK, but then is this patch about adding support for -fpass-plugin or is it for comparing "hello-world" passes in MLIR and LLVM? From the driver perspective, -fpass-plugin is merely a flag that tweaks the behavior of the middle-end. And the driver/Flang should only really care about whether the pass that was loaded dynamically was actually run or not. This can be tested with -fdebug-pass-manager. But for that you only need a "dummy" pass, right? Or, an example from LLVM.

And btw, at what optimisation level will this plugin be run? Also, note that what you included here is an in-tree plugin. A few other points below.

  1. "flang/test/Examples/print-fns-definitions.f90" was introduced to test "PrintFunctionNames" parse-tree plugin (i.e. a Flang plugin). LLVM plugins are very different (e.g. run at a completely different stage of the compilation pipeline). IMO, we shouldn't be testing such different things in one file.
  2. In your summary, could you indicate that similar option already exists in Clang and that you are (or perhaps not?) making sure that the semantics of this option in Flang are consistent with Clang. I think that it is important that we maintain compatibility with Clang and that we document any divergence from that. Also, by stating that the semantics are identical to those in Clang, you save yourself the need to document them. Otherwise, how are people going to know how to use this option?
  3. Could you add a relevant entry in https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/frontend-forwarding.f90?
  4. Could you update the release notes and driver documentation? This could be something brief.
  5. Will this work on all platforms? (e.g. Windows?)
clang/lib/Driver/ToolChains/Flang.cpp
55

This is not really something within the scope of this patch, but AddOtherOptions should be renamed as e.g. ForwardToFC1 or something similar.

flang/examples/PrintFunctionNamesLLVMPass/PrintFunctionNamesLLVMPass.cpp
11 ↗(On Diff #442367)

"an out-of-tree LLVM pass"

This is an "in-tree" pass.

flang/include/flang/Frontend/CodeGenOptions.h
48–58

DELETEME

flang/lib/Frontend/FrontendActions.cpp
687

IMO, it's a bit weird to report an error and to continue anyway. Apparently that's what Clang does 🤔 . Do you know why?

OK, but then is this patch about adding support for -fpass-plugin or is it for comparing "hello-world" passes in MLIR and LLVM? From the driver perspective, -fpass-plugin is merely a flag that tweaks the behavior of the middle-end.

That's a good point. I think something like that could be useful to newcomers to help decide what is most appropriate, but it should be separate from this patch.

And btw, at what optimisation level will this plugin be run? Also, note that what you included here is an in-tree plugin. A few other points below.

  1. "flang/test/Examples/print-fns-definitions.f90" was introduced to test "PrintFunctionNames" parse-tree plugin (i.e. a Flang plugin). LLVM plugins are very different (e.g. run at a completely different stage of the compilation pipeline). IMO, we shouldn't be testing such different things in one file.
  1. In your summary, could you indicate that similar option already exists in Clang and that you are (or perhaps not?) 3. 4.

Ok, I'll update the patch with these changes.

  1. Will this work on all platforms? (e.g. Windows?)

I don't know. Unfortunately, I don't have access to a Windows machine on which to test this. I imagine that since this largely uses LLVM's machinery, if it works with clang, it should work for us too. However, it would still need to be tested. Do you have any suggestions?

clang/lib/Driver/ToolChains/Flang.cpp
55

I don't think that I should change that here, unless you want me to. As you say, it probably belongs to a different patch.

flang/examples/PrintFunctionNamesLLVMPass/PrintFunctionNamesLLVMPass.cpp
11 ↗(On Diff #442367)

When I update this patch, I'll remove this pass and update the tests to use LLVM's Hello pass instead. I called this an out-of-tree pass because it can be used as is out-of-tree as well (that's how I tested it before adding it to the tree).

The registerPass function will need to be changed if the pass is to run at a specific optimization level. Since that OptimizationLevel argument is ignored, it will run even on -O0.

flang/include/flang/Frontend/CodeGenOptions.h
48–58

Do you want me to delete the second "public:" label?

flang/lib/Frontend/FrontendActions.cpp
687

I think the idea is to display as many errors as reasonably possible instead of just failing on the first error that was encountered. I imagine that the Diagnostics object was intended to be used when parsing where this approach is helpful. But when using it elsewhere, you end up with more ... peculiar behavior.

  1. Will this work on all platforms? (e.g. Windows?)

I don't know. Unfortunately, I don't have access to a Windows machine on which to test this. I imagine that since this largely uses LLVM's machinery, if it works with clang, it should work for us too. However, it would still need to be tested. Do you have any suggestions?

"Officially", plugins in LLVM are not supported on Windows :) I'm not aware of one single "source of truth" for this, but there are plenty of references on Discourse and Phabricator:

Going over https://reviews.llvm.org/D56935 should also help understand the implementation in Clang (and the logic behind the semantics of this option). That should help writing the documentation for Flang too.

clang/lib/Driver/ToolChains/Flang.cpp
55

I agree, this would be an "unrelated change". Hopefully one of us will remember to rename this in the future ;-)

flang/include/flang/Frontend/CodeGenOptions.h
48–58

it is not needed, is it?

flang/lib/Frontend/FrontendActions.cpp
687

So, an "error" indicates that something went fundamentally wrong and indeed, flang-new will fail (that's why you had to use not in tests). If the driver does not immediately return here, could you add a comment to hint "why"? I also went over the discussion in https://reviews.llvm.org/D56935 and the rationale in Clang is still unclear to me. How about you?

If you are simply trying to reproduce the Clang semantics here, then that would be a good comment for me.

tarunprabhu added inline comments.Jul 8 2022, 12:59 PM
flang/include/flang/Frontend/CodeGenOptions.h
48–58

Strictly speaking, no. I tend to group data members and member functions with the same access specifier and explicitly add the access specifier for each group. If the class ever grows, it just helps to keep things a bit better organized. I didn't look at the rest of the flang code to see if it is done differently elsewhere.

flang/lib/Frontend/FrontendActions.cpp
687

I think it's just deferring failure until later because that can be more helpful. It lets you inform the user about multiple potential problems at the same time. If you look at clang/lib/Frontend/CompilerInvocation.cpp, you will see a pattern where the DiagnosticsEngine::getNumErrors() is called at the start and end of each function, to indicate whether that particular functionality was completed without errors. Even there, the error is recorded, the but the failure is deferred.

But, I haven't been involved with clang development, so I am hardly an authority on the matter and I could be completely wrong about this.

Nevertheless, I'll add a comment saying that the code is intended to mimic clang.

It looks like LLVM's HelloWorld pass with the new pass manager is not built as a separate shared object. LLVMHello.so uses the old pass manager and cannot be loaded using -fpass-plugin.

A quick search for "-fpass-plugin" in clang/test didn't seem to yield anything so I don't know if this is actually tested there.

I would like to make a case for keeping the PrintFunctionNamesLLVMPass example for the following reasons:

  • It can be used to explicitly test -fpass-plugin.
  • It is a full working example of how to write an out-of-tree pass that can be loaded with -fpass-plugin (on further consideration, I disagree with your claim that it is an in-tree pass. It can be built independently of Flang/LLVM. Shouldn't that make it out-of-tree by definition? If not, even the PrintFunctionNames plugin would then become "in-tree").

The counter-arguments to this would be that the pass is not strictly related to flang, and probably ought to belong elsewhere in the LLVM tree. We could add it to flang now and move it after consultation with the broader community. As far as I have been able to tell, there is no full example of building a complete out-of-tree LLVM pass in the official documentation.

The other consideration is that there needs to be a way to ensure that the -fpass-plugin test is marked as "expected-to-fail" on Windows. I don't know how to do that but hopefully someone else knows.

It looks like LLVM's HelloWorld pass with the new pass manager is not built as a separate shared object. LLVMHello.so uses the old pass manager and cannot be loaded using -fpass-plugin.

  1. Good point, HelloWorldPass was refactored in https://reviews.llvm.org/D95907 not to be built as a standalone shared object. Might be worth bringing this up on Discourse - perhaps it would be OK to change this? If not, we'll know that a separate example is indeed needed.
  2. With the switch to the new PM, LLVM's HelloWorld pass should probably be removed. It would be good to bring this up on Discourse.
  3. Have you tried the Bye pass?

A quick search for "-fpass-plugin" in clang/test didn't seem to yield anything so I don't know if this is actually tested there.

From what I can see, it is not. If you do want to test this flag with a proper pass, then it would be very desirable to extend this patch to Clang and add a test there too. This way your work would benefit both sub-projects,

I would like to make a case for keeping the PrintFunctionNamesLLVMPass example for the following reasons:

  • It can be used to explicitly test -fpass-plugin.

This is a fair point, but do we need an LLVM pass in Flang for this? I don't believe we do. And if PrintFunctionNamesLLVMPass were to be included, why can't the run method be as simple as return PreservedAnalyses::all();?

  • It is a full working example of how to write an out-of-tree pass that can be loaded with -fpass-plugin

But this patch is not about writing out-of-tree LLVM passes, is it? If it is, then it should be a contribution made to LLVM instead.

(on further consideration, I disagree with your claim that it is an in-tree pass. It can be built independently of Flang/LLVM. Shouldn't that make it out-of-tree by definition? If not, even the PrintFunctionNames plugin would then become "in-tree").

This is my understanding of "in-tree" vs "out-of-tree":

You are proposing to add PrintFunctionNamesLLVMPass inside "llvm-project", so it's an "in-tree" example. In particular, if I copy "PrintFunctionNamesLLVMPass.cpp" to a directory outside "llvm-project", then I will no longer be able to build your example without extra CMake logic.

The counter-arguments to this would be that the pass is not strictly related to flang, and probably ought to belong elsewhere in the LLVM tree.

Indeed. In particular, an example like this inside LLVM (instead of Flang) could be used by both Flang and Clang for testing -fpass-plugin.

We could add it to flang now and move it after consultation with the broader community.

Why delay this? Also, you might be able to use the Bye pass in the meantime.

As far as I have been able to tell, there is no full example of building a complete out-of-tree LLVM pass in the official documentation.

Yup. I've tried to fill this gap by creating llvm-tutor (apologies for shameless self-promotion). I'm not sure whether there would be much appetite for this sort of content "in-tree" (is "in-tree" documentation the right place to document "out-of-tree" development?). In any case, is this patch about documenting out-of-tree LLVM pass development or about adding support for -fpass-plugin in Flang?

The other consideration is that there needs to be a way to ensure that the -fpass-plugin test is marked as "expected-to-fail" on Windows. I don't know how to do that but hopefully someone else knows.

You want to use ! UNSUPPORTED: system-windows (see e.g. save-temps.f90).

flang/include/flang/Frontend/CodeGenOptions.h
48–58

To me, with so few fields in this class, it's just a noise.

flang/lib/Frontend/FrontendActions.cpp
687

If you look at clang/lib/Frontend/CompilerInvocation.cpp, you will see a pattern where the DiagnosticsEngine::getNumErrors() is called at the start and end of each function, to indicate whether that particular functionality was completed without errors.

Similar pattern has been used in Flang, see e.g. here.

Even there, the error is recorded, the but the failure is deferred.

CompilerInvocation.cpp deals with "option parsing" and "compiler configuration". FrontendActions.cpp deals with "running" the compiler. These are very different stages in the compiler executation.

  1. Have you tried the Bye pass?

Ah, good point. I hadn't. I'll see if I can use that instead.

From what I can see, it is not. If you do want to test this flag with a proper pass, then it would be very desirable to extend this patch to Clang and add a test there too. This way your work would benefit both sub-projects,

Ok. It shouldn't be too difficult to create a patch for that. I suppose I
can add one and see what the clang community has to say about it.

Yup. I've tried to fill this gap by creating llvm-tutor (apologies for shameless self-promotion).

In the spirit of shameless self-promotion, I have done something similar for clang
plugins too :-) .

CompilerInvocation.cpp deals with "option parsing" and "compiler configuration". FrontendActions.cpp deals with "running" the compiler. These are very different stages in the compiler executation.

True, but even with actions, there may be those that would benefit from not
failing immediately. For instance, the ParseArgs() method in a
PluginASTAction may also report errors during option parsing that may
benefit from deferred failure (although I must confess that I don't know
exactly when a PluginASTAction's ParseArgs() is run during the
compilation process -- in particular, whether it runs during or after the
other arguments have been parsed. I think I did trace the path at one
point, but I don't remember now).

[EDIT: Fixed formatting]

  1. Have you tried the Bye pass?

Ah, good point. I hadn't. I'll see if I can use that instead.

It seems as if the Bye pass will only print a message when the -wave-goodbye flag is passed to it. But I don't know of a way to pass that flag to the pass when loading the pass using -fpass-plugin. If not, is there a way, with the new pass manager, to print the list of passes that were executed?

It seems as if the Bye pass will only print a message when the -wave-goodbye flag is passed to it. But I don't know of a way to pass that flag to the pass when loading the pass using -fpass-plugin. If not, is there a way, with the new pass manager, to print the list of passes that were executed?

You can use -fdebug-pass-manager, see e.g. here.

Ok. It shouldn't be too difficult to create a patch for that. I suppose I
can add one and see what the clang community has to say about it.

You can do it in this patch. It's going to be a small change.

True, but even with actions, there may be those that would benefit from not failing immediately.

We haven't identified a specific case in which not failing immediately would be desirable. I'm fine with the current approach, but would appreciate if consistency with Clang was emphasized (either in the commit message or as a comment, preferably both).

tarunprabhu edited the summary of this revision. (Show Details)

Removed the pass to test the option. Instead, the Bye plugin provided in LLVM is used to test that the option works. It is not clear if loading passes dynamically is supported on Windows and the test is designed to fail on that platform.

Addressed comments from @awarzynski.

Added documentation to the ReleaseNotes. It was put under "Major changes" but it can be moved to compiler options if that is a more appropriate place for this.

Added some text to the frontend driver documentation.

Added a test to checked that the -fpass-plugin option is correctly passed to the fc1 driver.

tarunprabhu marked an inline comment as done.Wed, Sep 21, 3:25 PM

Thanks for the updates!

It is not clear if loading passes dynamically is supported on Windows and the test is designed to fail on that platform.

It isn't in your implementation, so it is safe to document it as unsupported on Windows :) In general, plugins in LLVM are not officially supported on Windows.

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

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

Addressed other comments and issues.

tarunprabhu marked 13 inline comments as done.Thu, Sep 29, 3:22 PM

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

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 marked 4 inline comments as done.Fri, Sep 30, 2:14 PM