[PATCH] D109890: Flang OpenMP Report Plugin

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thanks for the updates @ijan1 ! This already looks great, but I have a few more suggestions to make it a bit leaner and concise. And to clarify a few things. For example - shouldn't this plugin use LLVM's ADT <https://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task> instead of STL? @kiranchandramohan 
, what are your thoughts?

A few more suggestions:

1. I don't see much point of having `flang/test/Examples/Inputs/` (i.e. the directory). For example, `flang/test/Examples/omp-atomic.f90` and `flang/test/Examples/Inputs/omp-atomic.f90` can be merged. Same for other tests like this.
2. Files from `flang/test/Examples/Inputs/` seem to be copied from `flang/test/Semantics`. If we are not going to re-use them, then it would be good explain that in the commit message (e.g. `Files x, y, z were copied from flang/test/Semantics. We decided not to share them because abc.`
3. IMHO, there's no need for `flang-omp-report.h`. Everything from there can be moved to `flang-omp-report.cpp`. Fewer files will make this easier to read/follow.
4. I really like how you made `flang-omp-report-support.{cpp|h}` implement just one thing - the visitor that's used inside `FlangOmpReport`. That makes so much sense! I would suggest renaming the files accordingly (e.g. `flang-omp-report-visitor.{cpp|h}`)

Thanks for all the effort :)



================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.cpp:253
+
+void OpenMPStatisticsParseTree(
+    Fortran::parser::Parsing &parsing, OpenMPCounterVisitor &visitor) {
----------------
This function is not needed. Please move this code inside `FlangOmpReport::ExecuteAction`. That's the only place where it's used.


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.h:30
+      : clause{std::get<0>(p)}, clauseDetails{std::get<1>(p)} {}
+  friend bool operator<(const ClauseInfo &a, const ClauseInfo &b) {
+    return a.clause < b.clause;
----------------
Hm, everything is `public` here. Do we need `friend`? Some in other places.


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.h:98-103
+      constructClauses; // curLoopLogRecord and loopLogRecordStack store
+                        // pointers to this datastructure's entries. Hence a
+                        // vector cannot be used since pointers are invalidated
+                        // on resize. Next best option seems to be deque. Also a
+                        // list cannot be used since YAML gen requires a
+                        // datastructure which can be accessed through indices.
----------------
Please reformat the comment so that it's above the declaration that it documents.


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.h:111
+
+void OpenMPStatisticsParseTree(Parsing &parsing, OpenMPCounterVisitor &visitor);
+
----------------
Can be deleted once the contents of this function is inlined inside `ExecuteAction`.


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.h:2
+//===-- examples/flang-omp-report-plugin/flang-omp-report-support.h -------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
kiranchandramohan wrote:
> Address the clang-tidy warning.
There are still some clang-tidy warnings left. Please fix.


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report.cpp:25
+        GetCurrentFileOrBufferName()}; // We are getting the file name
+    std::size_t found = buf2.rfind("/"); // and removing the path to it
+    buf2 = buf2.substr(found + 1); // otherwise it will be stored
----------------
Note that this will only work on Linux. We should avoid solutions that work only on a particular platform. Instead, you could use e.g. [[ https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/Path.h#L310 | filename ]] or other utilities from `llvm::sys::path`.

Also, you are overriding the default behavior of the compiler here, which can be counter intuitive. For example, what should happen here:
```
flang -fc1 -load flangOmpReport.so -plugin flang-omp-report -fopenmp omp-device-constructs.f90 -o <some_dir/another_dir>/output_file_name.yaml
```
? Where should `output_file_name.yaml` be saved? Either way, it would be helpful to have this behavior documented somewhere. For example at the top of this file.



================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report.h:1
+//===-- examples/flang-omp-report-plugin/flang-omp-report.h ---------------===//
+//
----------------
Please fix clang-tidy checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109890