[PATCH] D120999: [flang] Update the plugin API

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
Comment at: flang/examples/FlangOmpReport/FlangOmpReport.cpp:61
 
-    // Dump the output
-    std::unique_ptr<llvm::raw_pwrite_stream> OS{ci.CreateDefaultOutputFile(
-        /*Binary=*/true, /*InFile=*/GetCurrentFileOrBufferName(),
-        /*Extension=*/".yaml")};
-    llvm::yaml::Output yout(*OS);
+    llvm::yaml::Output yout(llvm::outs());
     yout << visitor.constructClauses;
----------------
kiranchandramohan wrote:
> awarzynski wrote:
> > kiranchandramohan wrote:
> > > I was thinking that `flang-omp-report` will run on all the source files and generate a lot of YAML files whose name is derived from the name of the source file. Then the summary python script collects a summary from all these info.
> > > 
> > > This code looks like it dumps everything to output, wouldn't it affect the running of the summarize python script?
> > >I was thinking that flang-omp-report will run on all the source files and generate a lot of YAML files whose name is derived from the name of the source file. 
> > 
> > Not quite. You will have to run the plugin manually for every file. But you can automate this by "hacking" a Makefile of the project that you are interested in. Currently, you will need these extra flags:
> > 
> > ```lang=Makefile
> >   FFLAGS=-fsyntax-only -I /.../llvm-project/flang/module -Xflang -load -Xflang /.../llvm-project/build/Release/lib/flangOmpReport.so -Xflang -plugin -Xflang flang-omp-report -fopenmp
> > ```
> > With this change, you will also have to pipe the output to a file (so that it does not end up in stdout):
> > ```lang=Makefile
> >   FFLAGS=-fsyntax-only -I /.../llvm-project/flang/module -Xflang -load -Xflang /.../llvm-project/build/Release/lib/flangOmpReport.so -Xflang -plugin -Xflang flang-omp-report -fopenmp -o output.yaml
> > ```
> > 
> > This does make using the plugin on larger projects a bit more tricky, but I feel that the API should be independent of the internals of the driver. In particular, the driver depends on various TableGen files from Clang that define diagnostics. So, in general, so does this plugin because it includes "CompilerInstance.h" from the driver. I'm trying to remove this dependency.
> > 
> > As for the script, it's independent of the plugin and all it needs are some *.yaml files (i.e. it does not care how you generate them).
> >  but I feel that the API should be independent of the internals of the driver
> 
> That is a good goal for plugins. But at the same time the advantage of running flang-omp-report as a plugin is the fact that we can run it on large projects. Most drivers default to using the file name as the default. Why can't we ask the driver to provide the output stream just like you did for `Parsing`?
> That is a good goal for plugins. But at the same time the advantage of running flang-omp-report as a plugin is the fact that we can run it on large projects. 

This change does not affect this.

Recall that in order to run `FlangOmpReport`on a project (note that we changed the name from `flang-omp-report` for consistency with other libs), you need to modify the corresponding build script. This is the case _with_ and _without_ this change. However, this update does mean that you will need one additional change in the build script though. For example, for SNAP I needed this:
```lang=Makefile
@@ -187,7 +187,7 @@ translv.o : global.o plib.o geom.o sn.o data.o control.o utils.o \
        $(FORTRAN) $(FFLAGS) -c $*.f90

 %.o:   %.f90
-       $(FORTRAN) $(FFLAGS) -c $<
+       $(FORTRAN) $(FFLAGS) -c $< >> $(basename $@).yaml

 #
```
But still, I'm not removing the possibility of running this plugin on a project (i.e. for multiple files).

> Most drivers default to using the file name as the default.

Yes, that's a good point. But I felt that that's not "mission critical". First, I want to get rid of the dependency on "CompilerInstance.h". That has caused build failures for people (CC @DavidSpickett  ).

I do agree that plugins should inherit the drivers' behavior. I just didn't have the time to look into this. Anyway, let me fix that (update coming shortly).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120999