Page MenuHomePhabricator

Flang OpenMP Report Plugin
ClosedPublic

Authored by ijan1 on Sep 16 2021, 8:23 AM.

Details

Summary

This plugin parses Fortran files and creates a
YAML report with all the OpenMP constructs and
clauses seen in the file.

The following tests have been modified to be
compatible for testing the plugin, hence why
they are not reused from another directory:

  • omp-atomic.f90
  • omp-declarative-directive.f90
  • omp-device-constructs.f90

The plugin outputs a single file in the same
directory as the source file in the following format:
<source-file-name>.yaml

Building the plugin:
ninja flangOmpReport

Running the plugin:
./bin/flang-new -fc1 -load lib/flangOmpReport.so -plugin flang-omp-report -fopenmp <source_file.f90>

Co-authored-by: Kiran Chandramohan <kiran.chandramohan@arm.com>
Co-authored-by: Stuart Ellis <stuart.ellis@arm.com>

Diff Detail

Event Timeline

ijan1 created this revision.Sep 16 2021, 8:23 AM
ijan1 requested review of this revision.Sep 16 2021, 8:23 AM
Herald added a project: Restricted Project. · View Herald Transcript
ijan1 changed the visibility from "Public (No Login Required)" to "No One".Sep 16 2021, 8:23 AM
ijan1 changed the visibility from "No One" to "All Users".Sep 16 2021, 11:24 AM

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?

flang/tools/CMakeLists.txt
9 ↗(On Diff #372954)

This is not a tool and hence should not be located in flang/tools. IMO, flang/examples is a better place. Note that PrintFlangFunctionNames is already there (the other plugin example inside LLVM Flang).

flang/tools/flang-omp-report-plugin/CMakeLists.txt
4 ↗(On Diff #372954)

[nit] IMO, -plugin at the end of the *.cpp filename can be skipped (i.e. rename the actual file)

flang/tools/flang-omp-report-plugin/count-openmp.cpp
82 ↗(On Diff #372954)

This is a very generic name. Could it be a bit more specific?

83–89 ↗(On Diff #372954)

Please use CreateDefaultOutputFile instead. Btw, it takes care of the diagnostics for you.

flang/tools/flang-omp-report-plugin/count-openmp.h
100 ↗(On Diff #372954)

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.

flang/tools/flang-omp-report-plugin/flang-omp-report-plugin.cpp
1 ↗(On Diff #372954)
11 ↗(On Diff #372954)

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
13–14 ↗(On Diff #372954)

[nit] Both OpenMPStatisticsParseTree and SummarizeResults are rather straightforward and I would be temped to inline them here.

19 ↗(On Diff #372954)

[nit] It's not a tool, it's a plugin.

ijan1 updated this revision to Diff 373837.Sep 21 2021, 3:39 AM
ijan1 marked 7 inline comments as done.

Thanks for the feedback! I am still working on some of the things you've pointed out, however I have managed to add tests.
Unfortunately, since the plugin outputs a file with the same name every time, this causes some of the tests to fail because they're ran in parallel.
I will have a patch which adds the source file's name to the one generated by the plugin so that there\' a unique file each time.

I will have a patch which adds the source file's name to the one generated by the plugin so that there\' a unique file each time.

You should get that for free by switching to CreateDefaultOutputFile for file generation.

ijan1 changed the visibility from "All Users" to "Public (No Login Required)".Sep 21 2021, 7:08 AM

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.
flang/examples/flang-omp-report-plugin/flang-omp-report-support.cpp
367 ↗(On Diff #373837)

Since we don't have a text report, I think this operator overload can be removed.

434 ↗(On Diff #373837)

Remove the Mapping Traits for SummaryRecord, ClauseSummary and ConstructSummary above. I think it is not required.

flang/examples/flang-omp-report-plugin/flang-omp-report-support.h
2 ↗(On Diff #373837)

Address the clang-tidy warning.

33 ↗(On Diff #373837)

I think this can be removed since we don't dump anything other than logs.

43 ↗(On Diff #373837)

I think this can be removed since we don't dump anything other than logs.

53 ↗(On Diff #373837)

I think this can be removed since we don't dump anything other than logs.

138 ↗(On Diff #373837)

This is probably not required, if we are not emitting the number of constructs and clauses?

140–142 ↗(On Diff #373837)

Are these three maps (ompConstructCounter, ompClauseCounter, constructClauseCount) required if the information collected by these is not emitted?

150–151 ↗(On Diff #373837)

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.

154 ↗(On Diff #373837)

I think this can also be removed.

ijan1 updated this revision to Diff 374183.Sep 22 2021, 4:40 AM
ijan1 marked 10 inline comments as done.

Thanks for the feedback. Updated accordingly.

ijan1 edited the summary of this revision. (Show Details)Sep 22 2021, 4:51 AM
ijan1 added inline comments.
flang/examples/flang-omp-report-plugin/flang-omp-report-support.h
140–142 ↗(On Diff #373837)

constructClauseCount is required. I've removed the others.

150–151 ↗(On Diff #373837)

Removing curLoopLogRecord leaves the "clauses" part in the yaml file empty. loopLogRecordStack was not needed and removed.

flang/tools/flang-omp-report-plugin/count-openmp.cpp
82 ↗(On Diff #372954)

Removed.

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 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 :)

flang/examples/flang-omp-report-plugin/flang-omp-report-support.cpp
253 ↗(On Diff #374183)

This function is not needed. Please move this code inside FlangOmpReport::ExecuteAction. That's the only place where it's used.

flang/examples/flang-omp-report-plugin/flang-omp-report-support.h
30 ↗(On Diff #374183)

Hm, everything is public here. Do we need friend? Some in other places.

98–103 ↗(On Diff #374183)

Please reformat the comment so that it's above the declaration that it documents.

111 ↗(On Diff #374183)

Can be deleted once the contents of this function is inlined inside ExecuteAction.

2 ↗(On Diff #373837)

There are still some clang-tidy warnings left. Please fix.

flang/examples/flang-omp-report-plugin/flang-omp-report.cpp
26

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

flang/examples/flang-omp-report-plugin/flang-omp-report.h
1 ↗(On Diff #374183)

Please fix clang-tidy checks.

Apologies, I forgot to mention this one before.

In FlangOmpReport::ExecuteAction, you do some filename manipulation in order to prevent the plugin from generating output files next to the input. You can avoid that and leverage the "existing" mechanism, e.g.:

  • flang-new -o <output-filename> <input-filename> will use <output-filename> as the location and name of the output file

In particular,flang-new -o - <intput-filename> will print to stdout. If you use this, then all you have to do is write your RUN lines in tests like this:

! RUNL %flang_fc1 -load %llvmshlibdir/flangOmpReport.so -plugin flang-omp-report -fopenmp %S/Inputs/omp-declarative-directive.f90 -o - | FileCheck %s

So, less code in tests and in ExecuteAction :)

ijan1 updated this revision to Diff 374488.Sep 23 2021, 2:45 AM
ijan1 marked 6 inline comments as done.

Thanks a lot for the feedback! I've changed accordingly to use -o now. Hopefully I got the header guards right this time.

ijan1 added inline comments.Sep 23 2021, 3:05 AM
flang/examples/flang-omp-report-plugin/flang-omp-report-support.h
30 ↗(On Diff #374183)

To my understanding, friend is used because class methods take an implicit this parameter. So, when you're overloading operator methods, there's 3 parameters being passed, which is illegal. I've moved the definitions out of the class.

ijan1 edited the summary of this revision. (Show Details)Sep 23 2021, 6:18 AM

From your summary:

The plugin outputs a single file in the following format: summary-<source-file>.yaml

This is no longer accurate. Similarly here:

Running the plugin:
./bin/flang-new -fc1 -load lib/flangOmpReport.so -plugin fomp-report-yaml -fopenmp <source_file.f90>

The plugin name is flang-omp-report.

In order to fix the clang-tidy check, try updating the header guard to: FORTRAN_FLANG_OMP_REPORT_VISITOR_H

flang/examples/flang-omp-report-plugin/flang-omp-report-visitor.h
22

We should avoid using using namespace <namespace> in header files. Otherwise, all files that include this file will have their local namespaces polluted with Fortran::parser. That might be undesired.

flang/examples/flang-omp-report-plugin/flang-omp-report.cpp
12

This will depend on how the plugin is invoked. For example,

./bin/flang-new -fc1 -load lib/flangOmpReport.so -plugin fomp-report-yaml -o - <source_file.f90>

prints to stdout. I suggest deleting this sentence and using ^^^ for your example (so that people see the output immediately when they run the plugin for the first time).

16

-fopen?

58–74

Small suggestion as how to split this into "functional" blocks.

Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2021, 12:50 AM

LGTM.
Please wait for @awarzynski's approval.

This revision is now accepted and ready to land.Sep 24 2021, 2:50 AM
ijan1 edited the summary of this revision. (Show Details)Sep 24 2021, 5:35 AM
ijan1 updated this revision to Diff 374814.Sep 24 2021, 5:38 AM
ijan1 marked 4 inline comments as done.

Thanks a lot for the feedback, it's easy to miss such silly mistakes.

awarzynski accepted this revision.Sep 24 2021, 5:52 AM

Great work, thank you @ijan1 for addressing my comments! LGTM

Many thanks to @stuartellis for preparing the first draft of this patch "downstream" and to @kiranchandramohan for designing and implementing the tool in the first place.

(Btw, we didn't really decided whether to switch to LLVM ADT. We can do it in a separate patch._

This revision was landed with ongoing or failed builds.Sep 28 2021, 2:57 PM
Closed by commit rG38c42d42eb3f: Flang OpenMP Report Plugin (authored by stuartellis, committed by ijan1). · Explain Why
This revision was automatically updated to reflect the committed changes.

Some buildbots are unhappy with the tests added in this patch. Uploaded fix here: https://reviews.llvm.org/D110682