[PATCH] D109890: Flang OpenMP Report Plugin

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thank you for working on this! I would like to suggest organising this a bit more strictly. Currently, it's hard to identify code that's specifically for visiting the nodes (e.g. `OpenMPCounterVisitor`) and  what is for collecting/dumping the data (e.g. `Dumper`). I suggest the following files:

- //flang-omp-report.cpp// that implements `FompReportYamlAction`/`FlangOmpReport` together with `OpenMPCounterVisitor`
- //flang-omp-report.h// that declares `FompReportYamlAction`/`FlangOmpReport` together with `OpenMPCounterVisitor`
- //flang-omp-report-support.cpp// that defines classes and functions for processing the data (e.g. `Dumper`)
- //flang-omp-report-support.h// that declares classes and functions for processing the data (e.g. `Dumper`)

//flang-omp-report.{cpp|h}// will require //flang-omp-report-support.{cpp|h}// to work, but the latter should be independent of //flang-omp-report.{cpp|h}// . Do you think that that could work?



================
Comment at: flang/tools/CMakeLists.txt:9
 
+add_subdirectory(flang-omp-report-plugin)
 add_subdirectory(f18)
----------------
This is not a tool and hence should not be located in `flang/tools`. IMO, `flang/examples` is a better place. Note that [[ https://github.com/llvm/llvm-project/tree/main/flang/examples/PrintFlangFunctionNames | PrintFlangFunctionNames ]] is already there (the other plugin example inside LLVM Flang).


================
Comment at: flang/tools/flang-omp-report-plugin/CMakeLists.txt:4
+  MODULE
+  flang-omp-report-plugin.cpp
+  count-openmp.cpp
----------------
[nit] IMO, `-plugin` at the end of the `*.cpp` filename can be skipped (i.e. rename the actual file)


================
Comment at: flang/tools/flang-omp-report-plugin/count-openmp.cpp:82
+
+class Dumper {
+  void openStream(llvm::StringRef fileName) {
----------------
This is a very generic name. Could it be a bit more specific?


================
Comment at: flang/tools/flang-omp-report-plugin/count-openmp.cpp:83-89
+  void openStream(llvm::StringRef fileName) {
+    std::error_code EC;
+    OS = new llvm::raw_fd_ostream(
+        fileName, EC, llvm::sys::fs::OpenFlags::OF_Text);
+    if (EC) {
+      llvm::errs() << "Opening file = " << fileName << "\n";
+    }
----------------
Please use  [[ https://github.com/llvm/llvm-project/blob/bdafe3124c9ac7276df6092e041d4b328684c680/flang/include/flang/Frontend/CompilerInstance.h#L201-L203 | CreateDefaultOutputFile ]] instead. Btw, it takes care of the diagnostics for you.




================
Comment at: flang/tools/flang-omp-report-plugin/count-openmp.h:100
+
+struct OpenMPCounterVisitor {
+  std::string normalize_construct_name(std::string s) {
----------------
This is a very large class and it would be helpful to have it declared in one file and defined in another. Otherwise it's hard to see what the actual interface is.


================
Comment at: flang/tools/flang-omp-report-plugin/flang-omp-report-plugin.cpp:1
+#include "count-openmp.h"
+
----------------
Missing LLVM header: https://llvm.org/docs/CodingStandards.html#file-headers


================
Comment at: flang/tools/flang-omp-report-plugin/flang-omp-report-plugin.cpp:11
+
+class FompReportYamlAction : public PluginParseTreeAction {
+  void ExecuteAction() override {
----------------
I suggest simply `FlangOmpReport`. IMO that will more accurately reflect what this is, i.e. 
* we know that it's an Action based on what this class inherits from
* the fact that it generates YAML is an implementation detail that does not have to be included in the name as there is no e.g. `FompReportTxtAction`


================
Comment at: flang/tools/flang-omp-report-plugin/flang-omp-report-plugin.cpp:13-14
+  void ExecuteAction() override {
+    OpenMPStatisticsParseTree(instance().parsing());
+    SummarizeResults();
+  }
----------------
[nit] Both `OpenMPStatisticsParseTree` and `SummarizeResults` are rather straightforward and I would be temped to inline them here. 


================
Comment at: flang/tools/flang-omp-report-plugin/flang-omp-report-plugin.cpp:19
+static FrontendPluginRegistry::Add<FompReportYamlAction> X(
+    "fomp-report-yaml", "Flang omp report tool - output YAML format");
----------------
[nit] It's not a `tool`, it's a plugin.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109890