[PATCH] D109890: Flang OpenMP Report Plugin

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Thanks @ijan1 for this plugin.

A few comments mostly relating to code not required due to either,

1. We only emit the log report and not the others (construct/clause summary, total constructs seen)
2. Only yaml is emitted (not text report)
3. We run plugin after semantics so no need for hacks to work after parsing.



================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.cpp:367
+
+std::istream &operator>>(std::istream &is, std::deque<LogRecord> &vlr) {
+  std::string ignore_line;
----------------
Since we don't have a text report, I think this operator overload can be removed.


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.cpp:434
+  }
+};
+template <> struct MappingTraits<ClauseInfo> {
----------------
Remove the Mapping Traits for SummaryRecord, ClauseSummary and ConstructSummary above. I think it is not required.


================
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.
----------------
Address the clang-tidy warning.


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.h:33
+using namespace Fortran::parser;
+struct SummaryRecord {
+  int numConstructs;
----------------
I think this can be removed since we don't dump anything other than logs.


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.h:43
+};
+struct ClauseSummary {
+  std::string clause;
----------------
I think this can be removed since we don't dump anything other than logs.


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.h:53
+};
+struct ConstructSummary {
+  std::string construct;
----------------
I think this can be removed since we don't dump anything other than logs.


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.h:138
+
+  int clauseCounter{0}, constructCounter{0};
+  std::string clauseDetails{""};
----------------
This is probably not required, if we are not emitting the number of constructs and clauses?


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.h:140-142
+  std::map<std::string, int> ompConstructCounter;
+  std::map<std::string, int> ompClauseCounter;
+  std::map<std::pair<std::string, std::string>, int> constructClauseCount;
----------------
Are these three maps (ompConstructCounter, ompClauseCounter, constructClauseCount) required if the information collected by these is not emitted?


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.h:150-151
+                        // datastructure which can be accessed through indices.
+  LogRecord *curLoopLogRecord{nullptr};
+  std::vector<LogRecord *> loopLogRecordStack;
+  std::vector<OmpWrapperType *> ompWrapperStack;
----------------
These two (curLoopLogRecord, loopLogRecordStack) were required to maintain which loop we are currently processing. This was required since the standalone tool was operating before semantic checks (which corrects some loop information). Since the plugin tool runs after semantics, it might not be required now. Please check.


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-support.h:154
+  std::map<OmpWrapperType *, std::vector<ClauseInfo>> clauseStrings;
+  bool isEndLoopDirective{false};
+  Parsing *parsing{nullptr};
----------------
I think this can also be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109890