[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:2692
 def fno_openmp_assume_teams_oversubscription : Flag<["-"], "fno-openmp-assume-teams-oversubscription">,
-  Group<f_Group>, Flags<[CC1Option, NoArgumentUnused, HelpHidden]>;
+  Group<f_Group>, Flags<[CC1Option, FC1Option, NoArgumentUnused, HelpHidden]>;
 def fno_openmp_assume_threads_oversubscription : Flag<["-"], "fno-openmp-assume-threads-oversubscription">,
----------------
agozillon wrote:
> awarzynski wrote:
> > Many of these options have identical flags. While not really needed for this change, it would still be nice to re-organise them a bit. This file could really benefit from some love :) Here's an example of what I have in mind: https://github.com/llvm/llvm-project/blob/cf60d3f1a688671c8eb7859bf0572c403c3c0cca/clang/include/clang/Driver/Options.td#L6575-L6600
> I can have a look into doing this in this patch :-)
Made an attempt at tidying these up with the recent commit, hopefully this is close to what you expected! 


================
Comment at: flang/tools/bbc/CMakeLists.txt:29
 FortranLower
+flangFrontendTool
 )
----------------
agozillon wrote:
> agozillon wrote:
> > awarzynski wrote:
> > > This a frontend driver library and so far `bbc` and `flang-new -fc1` have been entirely separate. Could this dependency be avoided?
> > I had hoped to share LangOpts so that the setOffloadModuleInterfaceAttributes function wouldn't turn into a monolithic set of arguments whenever it's invoked if more arguments are added, but the dependency isn't ideal I agree!
> > 
> > I could perhaps look into making some sort of shared data structure to be put inside of CrossToolHelpers that might remove the dependency and be similarly useable to how LangOpts works at the moment. If that doesn't work, I can revert the change to just be a regular argument list and we can revisit the topic if new options are ever added? 
> Although looking at @domada's https://reviews.llvm.org/D146612 patch it reminds me that they could just be separate functions shared across tools (I perhaps got a little fixated on the idea of it being similar to a driver function handling all the options). Please do tell me whichever you'd prefer :-)
I removed the dependency via a helper class, that will keep flang-new a little cleaner hopefully and allow bbc to utilise the same logic provided it populates an intermediate structure!

But alternatively it's possible to split into separate functions for each attribute that depends on options if that's the desired direction!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145264