This is an archive of the discontinued LLVM Phabricator instance.

[flang][flang-omp-report] Add flang-omp-report summarising script
ClosedPublic

Authored by josh.mottley.arm on Oct 4 2021, 1:54 AM.

Details

Summary

The flang plugin `flang-omp-report` takes one fortran file in and returns a
YAML report file of the input file. This becomes an issue when you want to
analyse an entire project into one final report.
The purpose of this Python script is to generate a final YAML
report from all of the files generated by `flang-omp-report`. The report can
have (currently) 2 formats; summary and log. Summary focuses on "summarizing"
all constructs and there clauses from all YAML files with a corresponding "count"
for each. Log instead combines the generated YAML files into one report in a
"cleaner" format. (Pseudo) Examples can be found for both formats at the top of
the script.

Co-Authored by: Ivan Zhechev <ivan.zhechev@arm.com>

Diff Detail

Event Timeline

ijan1 created this revision.Oct 4 2021, 1:54 AM
Herald added a project: Restricted Project. · View Herald Transcript
ijan1 requested review of this revision.Oct 4 2021, 1:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2021, 1:54 AM
josh.mottley.arm commandeered this revision.Oct 26 2021, 7:03 AM
josh.mottley.arm added a reviewer: ijan1.
josh.mottley.arm added a subscriber: josh.mottley.arm.

I will now be taking over this patch from Ivan.

Refactored code.

Hey @josh.mottley.arm , thanks for taking over! I like how you have refactoring this and split into functions - makes a lot of sense! Also, great job documenting them!

Currently this script is somewhere between a "Python Script" and a "Python Module". Take a look idiomatic usage for python scripts. Basically, in a script everything should be inside functions. Also, things inside the main block (i.e. if __name__ == "__main__") should be kept to the bare minimum. Could you update accordingly? This shouldn't be too much work.

flang/examples/flang-omp-report-plugin/yaml_summarizer.py
58

When ruamel.yaml is not present, the script will still run all the way down here and fail with:

Traceback (most recent call last):
  File "flang/examples/flang-omp-report-plugin/yaml_summarizer.py", line 58, in <module>
    yaml = YAML()
NameError: name 'YAML' is not defined
220–224

[nit] IMHO, we don't need a function for this.

josh.mottley.arm marked 2 inline comments as done.
  • Removed try...except check for importing ruamel.yaml. This also includes removing the yaml_module_not_found function.
  • Added requirements.txt.
  • Refactored code to include functions; main, parse_arguments, create_arg_parser.
awarzynski added inline comments.Oct 28 2021, 2:22 AM
flang/examples/flang-omp-report-plugin/yaml_summarizer.py
4–5

[nit] Could you add a note explaining why one can't simply use the plugin here?

6
21

IMHO, you can safely remove This option (it is clear that it's an option ;-) ). On the other hand, it would be helpful to emphasis the difference between -l and the default behavior. See my suggestion inline.

42

You don't need the search pattern outside this method. And to me, find_yaml_files should only care about the directory in which to search and whether to do secretively. The search pattern itself that you pass to glob.iglob is just an implementation detail.

But your current approach is correct and this comment is just a nit.

51–55

This an important comment, but provides a solution a bit preemptively. And, by differentiating between the source and build dirs, it's become rather long. Also, I'm concerned that it might be quite hard to find a good solution and perhaps not worth it?

How about this:

# @TODO: Currently *all* yaml files are read - regardless of whether they have been generated with  'flang-omp-report' or not. This might result in the script reading files that it should ignore.
70–81

Wouldn't the output with -l only really differ when at least 2 input files were used?

96
119–136

Was this generated using /../llvm-project/flang/test/Examples/omp-device-constructs.f90? (like for process_log?)

163

[nit] That's clear from the function definition.

josh.mottley.arm marked 7 inline comments as done.
  • Fixed issues reported from review comments.
  • Added type hints where appropriate to arguments of functions.
  • Removed the default arguments from functions where it was not neccesary, and added default arguments to parsed arguments.
flang/examples/flang-omp-report-plugin/yaml_summarizer.py
70–81

The only difference when using 1 input file to original yaml file would be that the file path is stated only once at the top, whereas the original yaml file states the file with each construct.

119–136

My bad. It was generated using all the omp tests. Changed example code to just use /../llvm-project/flang/test/Examples/omp-device-constructs.yaml now since its a bit more interesting as an example. Also made both "process" functions use the same ".yaml" file for the example, so you can see the clear difference between the two methods.

awarzynski added inline comments.Nov 1 2021, 5:07 AM
flang/examples/flang-omp-report-plugin/yaml_summarizer.py
34
70–81

It looks like -l will change the output in two cases:

  • when only one file is read, the filename will only appear once at the top (as opposed to multiple times within the file)
  • when multiple files are read, then the filename for each file is only printed once before the input for the corresponding file is dumped

Is this correct? Could you document it? Perhaps that's something that should appear in the module doc-string rather than here?

Also, I think that you could simplify this documentation by including a pseudo-example:

$ python3 yaml_summarizer.py -l file_1.yaml file_2.yaml
file_1.yaml
<OMP clauses and constructs from file_1.yaml>
file_2.yaml
<OMP clauses and constructs from file_2.yaml>

$ python3 yaml_summarizer.py file_1.yaml file_2.yaml
<OMP clauses and constructs from file_1.yaml and file_2.yaml>

Or something similar. This way, you make the behavior controlled with -l more obvious. Also, pasting the output for omp-device-constructs.f90 takes a lot of space, but does not really clarify much. I would just remove it. Similar comment for process_summary.

237–238

Why is this needed?

josh.mottley.arm marked 4 inline comments as done.
  • Fixed issues reported from review comments.
  • Extended '-d' argument to be able to take a list of semicolon seperated directory paths to search for '.yaml' files in.
awarzynski accepted this revision.Nov 4 2021, 6:06 AM

Many thanks for working on this, LGTM!

This revision is now accepted and ready to land.Nov 4 2021, 6:06 AM
josh.mottley.arm edited the summary of this revision. (Show Details)Nov 4 2021, 9:14 AM
josh.mottley.arm retitled this revision from flang-omp-report summarising script to [flang][flang-omp-report] Add flang-omp-report summarising script.Nov 4 2021, 10:09 AM
josh.mottley.arm edited the summary of this revision. (Show Details)Nov 4 2021, 10:16 AM