This is an archive of the discontinued LLVM Phabricator instance.

Add -fpass-plugin option to Flang
ClosedPublic

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

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.Sep 21 2022, 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.

awarzynski added inline comments.Sep 29 2022, 1:29 PM
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.Sep 29 2022, 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.Sep 30 2022, 2:14 PM

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.

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.

Used wrong bracket type in previous revision.

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

Potentially fixed regex to correctly match round brackets.

awarzynski added inline comments.Oct 4 2022, 6:27 AM
clang/lib/Driver/ToolChains/Flang.cpp
58

CLANGFORMATME

flang/test/Driver/pass-plugin.f90
11

Ran clang-format.

Fixed test to allow for any namespace prefix.

tarunprabhu marked 2 inline comments as done.Oct 4 2022, 7:30 AM

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 accepted this revision.Oct 4 2022, 11:22 AM

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

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.

This revision is now accepted and ready to land.Oct 4 2022, 11:22 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2022, 4:09 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks for the review and the comments. :-)

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?

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

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

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 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'

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.

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.

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.

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

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.

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

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?

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"

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 :)

@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...

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 added a comment.EditedOct 11 2022, 12:11 PM

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?

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 :)

@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 added a comment.EditedOct 12 2022, 6:42 AM

@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]

@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 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.

The tests still passed.

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

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 added inline comments.Nov 4 2022, 8:32 AM
flang/test/CMakeLists.txt
65 ↗(On Diff #472161)

IIUC, Bye is only needed when plugins are enabled.

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

Previous revision was missing the tests.

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

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
65 ↗(On Diff #472161)

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

tarunprabhu marked 2 inline comments as done.Nov 10 2022, 6:37 AM

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
65 ↗(On Diff #472161)

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 added a subscriber: ro.Nov 24 2022, 5:04 AM

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.

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 added a comment.Nov 24 2022, 5:54 AM

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.