[flang-commits] [PATCH] D129156: Add -fpass-plugin option to Flang

Sat Jul 9 05:26:36 PDT 2022

awarzynski added a comment.

In D129156#3640027 <https://reviews.llvm.org/D129156#3640027>, @tarunprabhu wrote:

> 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 <https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/HelloWorld.cpp>  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 <https://github.com/llvm/llvm-project/blob/main/llvm/examples/Bye/Bye.cpp> 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":

- "in-tree" --> located //inside//  llvm-project <https://github.com/llvm/llvm-project>
- "out-of-tree" --> located //outside//  llvm-project <https://github.com/llvm/llvm-project>

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 <https://github.com/llvm/llvm-project/blob/main/llvm/examples/Bye/Bye.cpp> 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 <https://github.com/banach-space/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 <https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/save-temps.f90#L4>).

================
Comment at: flang/include/flang/Frontend/CodeGenOptions.h:49
+
 public:
   CodeGenOptions();
----------------
tarunprabhu wrote:
> awarzynski wrote:
> > tarunprabhu wrote:
> > > awarzynski wrote:
> > > > DELETEME
> > > Do you want me to delete the second "public:" label?
> > it is not needed, is it?
> 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. 
To me, with so few fields in this class, it's just a noise.

================
Comment at: flang/lib/Frontend/FrontendActions.cpp:674
+    } else {
+      diags.Report(clang::diag::err_fe_unable_to_load_plugin)
+          << pluginFile << passPlugin.takeError();
----------------
tarunprabhu wrote:
> awarzynski wrote:
> > tarunprabhu wrote:
> > > awarzynski wrote:
> > > > IMO, it's a bit weird to report an error and to continue anyway. Apparently that's what Clang [[ https://github.com/llvm/llvm-project/blob/f80a4321ef1bafcd8041884bcb85d9ba24335adb/clang/lib/CodeGen/BackendUtil.cpp#L791-L800 | does ]] 🤔 . Do you know why?
> > > 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. 
> > 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.
> 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. 
> 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. [[ https://github.com/llvm/llvm-project/blob/f80a4321ef1bafcd8041884bcb85d9ba24335adb/flang/lib/Frontend/CompilerInvocation.cpp#L495 | 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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129156/new/

https://reviews.llvm.org/D129156