[PATCH] D108283: [flang][driver] Add documentation for Plugins

Fri Aug 20 06:17:08 PDT 2021

awarzynski added a subscriber: Meinersbur.
awarzynski added a comment.

Thank you for addressing my comments @stuartellis ! I really like the structure of this document. I've left a few more suggestions inline. I've noticed that you spell certain things using CamelCase (e.g. Plugin, Frontend Action, Frontend Driver, Parse Tree), but that's inconsistent with the convention here and other documents within Flang. Could you update this?

Adding @Meinersbur to the list of reviewers. He's got extensive experience with plugins in LLVM and might have some suggestions for you too.

I will be away for 2 weeks, so won't be able to return to this earlier. If this is approved by other reviewers and my comments are addressed, please go ahead and merge.

Many thanks for working on this!

================
Comment at: flang/docs/FlangDriver.md:277
+# Frontend Driver Plugins
+Plugins are an extension to the Frontend Driver that make it possible to run
+extra user defined actions, in the form of a `FrontendAction`, during a
----------------
The convention in this document is  `frontend driver` rather than `Frontend Driver`.

================
Comment at: flang/docs/FlangDriver.md:278
+Plugins are an extension to the Frontend Driver that make it possible to run
+extra user defined actions, in the form of a `FrontendAction`, during a
+compilation, after semantic checks. Similarly to Clang, Flang leverages
----------------
I think that this would be clearer:
```
(...) extra user defined frontend actions, in the form of a specialization of a `PluginParseTreeAction`. These actions are run during compilation, after semantic checks.
```

================
Comment at: flang/docs/FlangDriver.md:281
+`LoadLibraryPermanently` from LLVM's `llvm::sys::DynamicLibrary` to load dynamic
+objects that implement Plugins. The process for using Plugins includes:
+* Creating a Plugin Shared Object library (i.e. implementation)
----------------
Why `Plugin` rather than `plugin`?

================
Comment at: flang/docs/FlangDriver.md:282-283
+objects that implement Plugins. The process for using Plugins includes:
+* Creating a Plugin Shared Object library (i.e. implementation)
+* Loading and running a Plugin Shared Object (i.e. usage)
+
----------------
Apologies, I didn't mean my suggestion literally when making the comment earlier :) I think that now this is a bit too verbose. How about:

```
* Creating a Plugin
*  Loading and Running a Plugin
```
Could you make these lit items link to the corresponding sections?

================
Comment at: flang/docs/FlangDriver.md:289-292
+1. A class that wraps everything together and represents the Frontend Action
+   corresponding to your Plugin, which inherits from `PluginParseTreeAction`
+1. An `ExecuteAction` function that implements what the Plugin actually does
+1. A line to register the Plugin
----------------
Also, could make these items link to the corresponding sections in the document? The descriptions that I am deleting here could be moved to the corresponding sections.

================
Comment at: flang/docs/FlangDriver.md:292
+1. An `ExecuteAction` function that implements what the Plugin actually does
+1. A line to register the Plugin
+
----------------
>1. A line to register the Plugin
In your example it's more than just one line.

================
Comment at: flang/docs/FlangDriver.md:295
+There is an example Plugin located in `flang/example/PrintFlangFunctionNames`
+that demonstrates these points by using the Parse Tree to print out function
+names.
----------------
Could you be more specific here? What do you mean by `Parse Tree`? The `ParseTree API`?

Also, `function _and_ subroutine names declared in the input file`.

================
Comment at: flang/docs/FlangDriver.md:301
+(defined in `flang/include/flang/FrontendActions.h`), in order to have access to
+the Parse Tree post semantic checks, and also so that it can be registerd, e.g.
+```cpp
----------------

================
Comment at: flang/docs/FlangDriver.md:307
+### Implementation of `ExecuteAction`
+Like a normal `FrontendAction`, your Plugin class will need to implement the
+`ExecuteAction` method. This method will contain what the Plugin actually does,
----------------
> your Plugin class will need to implement 

No, that's not required. But if users don't implement this, the [[ https://github.com/llvm/llvm-project/blob/main/flang/lib/Frontend/FrontendActions.cpp#L368 | default ]] `ExectueAction` is run and that doesn't do anything. Could you clarify this?

================
Comment at: flang/docs/FlangDriver.md:337
+```
+The different types of nodes and also what each node structure contain are in
+`flang/include/flang/Parser/parse-tree.h`. In the example, there is a `Post`
----------------

================
Comment at: flang/docs/FlangDriver.md:372
+
+### The `-load` option
+This loads the Plugin shared object library, with the path given to the option,
----------------
This way you can refer to `<dsopath>` in your description and also, IMHO, the option itself becomes clearer.

================
Comment at: flang/docs/FlangDriver.md:378
+
+### The `-plugin` option
+This sets `frontend::ActionKind programAction` in `FrontendOptions` to
----------------
This way you can refer to `<name>` in your description and also, IMHO, the option itself becomes clearer.

================
Comment at: flang/docs/FlangDriver.md:383-384
+otherwise it reports an error diagnostic and returns `nullptr`.
+
+## Plugin CMake Flag
+There is a CMake flag `FLANG_BUILD_EXAMPLES`, which when enabled, builds the
----------------
This whole section is only relevant for in-tree plugins (i.e. plugins that are developed inside llvm-project/flang). Otherwise, these CMake variables don't really exist. It would be good to clarify this and also to mention that it is possible to develop plugins out-of-tree. Feel free to document the process for the latter or just to "leave it for later".

Btw, this is related to the question from @kiranchandramohan on the location of plugins. You could expand this section to answer his question here. Also, I would rename this section as e.g. `Enabling In Tree Plugins`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108283