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

[Andrzej Warzynski via Phabricator via flang-commits]

awarzynski added a comment.

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

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

>> Btw, have you considered re-using LLVM's Hello <https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Hello/Hello.cpp> 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 <https://github.com/llvm/llvm-project/blob/f80a4321ef1bafcd8041884bcb85d9ba24335adb/clang/include/clang/Driver/Options.td#L6275-L6278>. 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 <https://github.com/llvm/llvm-project/blob/f80a4321ef1bafcd8041884bcb85d9ba24335adb/flang/docs/ReleaseNotes.md> and driver documentation <https://github.com/llvm/llvm-project/blob/f80a4321ef1bafcd8041884bcb85d9ba24335adb/flang/docs/FlangDriver.md>? This could be something brief.
5. Will this work on all platforms? (e.g. Windows?)



================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:55
 void Flang::AddOtherOptions(const ArgList &Args, ArgStringList &CmdArgs) const {
   Args.AddAllArgs(CmdArgs,
                   {options::OPT_module_dir, options::OPT_fdebug_module_writer,
----------------
This is not really something within the scope of this patch, but `AddOtherOptions` should be renamed as e.g. `ForwardToFC1` or something similar.


================
Comment at: flang/examples/PrintFunctionNamesLLVMPass/PrintFunctionNamesLLVMPass.cpp:11
+// and external (library) functions in the LLVM IR. The intention is simply to
+// demonstrate how to create an out-of-tree LLVM pass and then load it using
+// -fpass-plugin. This will also allow the -fpass-plugin option to be tested in
----------------
"an out-of-tree LLVM pass"

This is an "in-tree" pass.


================
Comment at: flang/include/flang/Frontend/CodeGenOptions.h:49
+
 public:
   CodeGenOptions();
----------------
DELETEME


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:674
+    } else {
+      diags.Report(clang::diag::err_fe_unable_to_load_plugin)
+          << pluginFile << passPlugin.takeError();
----------------
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?


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

https://reviews.llvm.org/D129156