This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr
AbandonedPublic

Authored by agozillon on Mar 3 2023, 12:13 PM.

Details

Summary

This patch adds the lowering for the Module FlagsAttr to FIR (the OpenMP
dialect component) for the bbc/flang-new tool and compiler.

In addition It ports additional flags from the shared Clang compiler options to
provide this lowering to the OpenMP Dialect.

This patch enables the flags below (and any equals or inverse variants) for
Flang that exist in Clang:

-fopenmp-target-debug
-fopenmp-assume-threads-oversubscription
-fopenmp-assume-teams-oversubscription
-fopenmp-assume-no-nested-parallelism
-fopenmp-assume-no-thread-state

For the bbc tool it only utilises the primary variants to minimize additional
complexity in the tool.

The patch also provides lowering of the FlagsAttr to LLVM-IR,
each individual frontend flag (an argument to the OpenMP MLIR attribute)
is lowered to a global.

In short the patch generates an OpenMP MLIR Dialect FlagsAttr to hold
provided flag information so that it can proliferate down to the translation
to LLVM-IR that occurs in OpenMPToLLVMIRTranslation, allowing use of the
OpenMPIRBuilder to create LLVM-IR globals for the OpenMP runtime to
eventually utilise.

Diff Detail

Event Timeline

agozillon created this revision.Mar 3 2023, 12:13 PM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
agozillon requested review of this revision.Mar 3 2023, 12:13 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript

This builds on the following phabricator patches that are in review at the moment, so there are components that are used here that are added from these:

https://reviews.llvm.org/D144864 : add fopenmp-is-device and an mlir is-device attribute for the OpenMP dialect
https://reviews.llvm.org/D144896 : the RTLModuleFlagsAttr that is in use within this patch for lowering and application
https://reviews.llvm.org/D144883 : an infrastructure addition for the MLIR LLVM translation interface

For the moment, this patch is primarily to give context to the usage of the RTLModuleFlagsAttr mlir attribute and give some further understanding for a reviewer, however, please feel free to review it if you have time as I will eventually ask for some revision of it after all the components it depends on have been progressed.

agozillon edited the summary of this revision. (Show Details)Mar 3 2023, 12:29 PM
agozillon updated this revision to Diff 509487.Mar 29 2023, 3:18 PM
agozillon edited the summary of this revision. (Show Details)
  • [Flang][MLIR][OpenMP] Fix rebase inconsistencies
  • [Flang][bbc][Tools] Add OpenMP RTL flags to bbc and adjust setOffloadModuleInterfaceAttributes to use LangOpts
  • [Flang][bbc][Tools] Update rtl-flags.f90 test for bbc tests and generally fix due to rebase changes

All the mechanisms for this patch to now function are upstream, so I believe it is ready for a review if at all possible @kiranchandramohan @awarzynski and other reviewers that this patch may be of interest to.

Although if we wish for it to be split into two separate Phabricator patches, one for the lowering, the other for the flag additions and attribute creation, I can do so. However, I think this patch gives the big picture which may be useful.

As a side-note there will be a follow up commit in the near future (tomorrow if nothing steals my attention) which will do the following:

  • Add an additional mlir-translate test, as I believe I owe @kiranchandramohan a test I promised from another patch!
  • The clang/test/Driver/flang/flang-omp.f90 will be converted into a flang specific test and likely moved into the existing omp-frontend-forwarding.f90 test
agozillon updated this revision to Diff 509626.Mar 30 2023, 5:34 AM
  • [Flang][Driver][Test] Move flang-omp.f90 tests into omp-frontend-forwarding.f90
agozillon updated this revision to Diff 509628.Mar 30 2023, 5:36 AM
  • [Flang][Driver][Test] Move flang-omp.f90 tests into omp-frontend-forwarding.f90

Recent update(s) moved flag/driver testing of the flags from Clang to Flang's omp-frontend-forwarding.f90 aligning it with previous tests.

agozillon retitled this revision from [OpenMP][MLIR] Lower and apply RTLModuleFlagsAttr to [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr.Mar 30 2023, 5:54 AM
agozillon edited the summary of this revision. (Show Details)

Unfortunately I do not believe an mlir-translate test that tests if the OffloadModuleInterface is accessible when directly utilizing mlir-translate is possible for this patch... I forgot I removed the is device check as it is already done at the initial creation of the attribute. However, I do have a future patch that it will be utilised in and when @jsjodin's initial TargetOp work is in I can likely create a test around that functionality.

The driver plumbing looks good to me. I will defer reviewing the OpenMP changes to experts. Thanks for working on this!

clang/include/clang/Driver/Options.td
2692–2693

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

flang/tools/bbc/CMakeLists.txt
29 ↗(On Diff #509628)

This a frontend driver library and so far bbc and flang-new -fc1 have been entirely separate. Could this dependency be avoided?

agozillon added inline comments.Mar 30 2023, 6:27 AM
clang/include/clang/Driver/Options.td
2692–2693

I can have a look into doing this in this patch :-)

flang/tools/bbc/CMakeLists.txt
29 ↗(On Diff #509628)

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?

agozillon added inline comments.Mar 30 2023, 8:30 AM
flang/tools/bbc/CMakeLists.txt
29 ↗(On Diff #509628)

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

agozillon updated this revision to Diff 509837.Mar 30 2023, 3:17 PM

Squash commits, format and apply requested changes

agozillon added inline comments.Mar 30 2023, 3:55 PM
clang/include/clang/Driver/Options.td
2692–2693

Made an attempt at tidying these up with the recent commit, hopefully this is close to what you expected!

flang/tools/bbc/CMakeLists.txt
29 ↗(On Diff #509628)

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!

awarzynski accepted this revision.Mar 31 2023, 1:45 AM

@agozillon Thanks for the updates, LGTM! Please wait for somebody to review the OpenMP changes before merging.

This revision is now accepted and ready to land.Mar 31 2023, 1:45 AM
kiranchandramohan requested changes to this revision.Mar 31 2023, 3:24 AM

Please split this patch into three:

  1. Code changes and testing for the driver and the FIR+OpenMP dialect generated.
  2. Code changes and test for FIR+OpenMP to LLVM+OpenMP dialect.
  3. Code changes and testing for the translation from LLVM + OpenMP dialect to LLVM IR. Code in OpenMPToLLVMIRTranslation.cpp should be tested with mlir-translate.

Unfortunately I do not believe an mlir-translate test that tests if the OffloadModuleInterface is accessible when directly utilizing mlir-translate is possible for this patch... I forgot I removed the is device check as it is already done at the initial creation of the attribute. However, I do have a future patch that it will be utilised in and when @jsjodin's initial TargetOp work is in I can likely create a test around that functionality.

You may delay the translation code till then.

This revision now requires changes to proceed.Mar 31 2023, 3:24 AM

Please split this patch into three:

  1. Code changes and testing for the driver and the FIR+OpenMP dialect generated.
  2. Code changes and test for FIR+OpenMP to LLVM+OpenMP dialect.
  3. Code changes and testing for the translation from LLVM + OpenMP dialect to LLVM IR. Code in OpenMPToLLVMIRTranslation.cpp should be tested with mlir-translate.

More than happy to do all of the above, however, there is no code for step 2 in this case as it undergoes no rewriting or transformation when going from FIR+OpenMP -> LLVM+OpenMP. However, I am happy to make a test to assure it is retained during the conversion, but I believe it would just be a test in the patch, would it be okay to merge patch 2 and 3 from your comment? Meaning there would be two patches, the driver additions and the attribute generation from the flags. And then the lowering of the attribute to LLVM-IR (with additional mlir-translate tests for LLVM+OpenMP -> LLVM-IR and FIR+OpenMP -> LLVM+OpenMP).

Unfortunately I do not believe an mlir-translate test that tests if the OffloadModuleInterface is accessible when directly utilizing mlir-translate is possible for this patch... I forgot I removed the is device check as it is already done at the initial creation of the attribute. However, I do have a future patch that it will be utilised in and when @jsjodin's initial TargetOp work is in I can likely create a test around that functionality.

You may delay the translation code till then.

Thank you.

Fragmented this patch into https://reviews.llvm.org/D147344 (lowering) and https://reviews.llvm.org/D147324 (driver/tool changes and application of attribute). I will keep this patch open until the others are closed to give a big picture for easier reference.

agozillon abandoned this revision.Apr 19 2023, 6:43 AM

Closing this patch now that each individual component is upstreamed via seperate phabricator patches! Thank you to all the reviewers for your time and help.