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

[Tarun Prabhu via Phabricator via flang-commits]

tarunprabhu added a comment.

In D129156#3638935 <https://reviews.llvm.org/D129156#3638935>, @awarzynski wrote:

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



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

> 5. 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?



================
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,
----------------
awarzynski wrote:
> This is not really something within the scope of this patch, but `AddOtherOptions` should be renamed as e.g. `ForwardToFC1` or something similar.
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.


================
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
----------------
awarzynski wrote:
> "an out-of-tree LLVM pass"
> 
> This is an "in-tree" pass.
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. 


================
Comment at: flang/include/flang/Frontend/CodeGenOptions.h:49
+
 public:
   CodeGenOptions();
----------------
awarzynski wrote:
> DELETEME
Do you want me to delete the second "public:" label?


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


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

https://reviews.llvm.org/D129156