This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][MLIR] Add RTLModuleFlags attribute to OpenMP Dialect
ClosedPublic

Authored by agozillon on Feb 27 2023, 10:43 AM.

Details

Summary

The intent of this attribute is for it to be applied to a module and
then hold information on runtime library (RTL) flags given to
Flang (or other OpenMP frontend) that should be lowered down to
LLVM-IR for devices as LLVM globals. The following related
flags are:

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

These exist within Clang and are lowered into the IR when
offloading for device. This attribute allows this infromation
to be carried down from the Flang frontend to the OpenMP
to LLVM/OpenMP Dialect to LLVM-IR translation phase
and then be lowered to LLVM-IR.

Diff Detail

Event Timeline

agozillon created this revision.Feb 27 2023, 10:43 AM
agozillon requested review of this revision.Feb 27 2023, 10:43 AM
jdoerfert added inline comments.Feb 27 2023, 11:22 AM
mlir/test/Dialect/OpenMP/attr.mlir
25

What happens if we define only a subset? Will they be auto-added? Just add a test for now to document the behavior.

Could you update the discourse thread (https://discourse.llvm.org/t/rfc-omp-module-and-omp-function-vs-dialect-attributes-to-encode-openmp-properties/67998) with the chosen approach and reasons before proceeding?

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
35–40

This would probably mean that we only support mlir builtin Modules.

Could you update the discourse thread (https://discourse.llvm.org/t/rfc-omp-module-and-omp-function-vs-dialect-attributes-to-encode-openmp-properties/67998) with the chosen approach and reasons before proceeding?

Will do Kiran! Thank you and sorry for not updating it beforehand.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
35–40

True, thank you for pointing this out, I will try to make this more generic by using an Operation* to leave the door open for other use-cases than Flang (and if we ever swap to a specialised module) and I will do the same for the omp.is_device patch that has similar uses and functions.

mlir/test/Dialect/OpenMP/attr.mlir
25

With the current attribute definition, they will not be auto-added as I've not given the Optional<bool> or Default<bool> types in the attribute. It would make it an illegal attribute/error!

I originally did this so the values would always be pretty printed in the IR when not given. But in hindsight it's a lot more verbose looking than it likely should be, especially as most will be set to default in any case and it's existence on the module is more relevant than the lack of it. Perhaps something like the following would be better:

module attributes {omp.rtlmoduleflags = #omp.rtlmoduleflags<debug_kind : 300, assume_teams_oversubscription>}

Where the presence indicates that it's been set to true and the flag has been used, rather than explicitly stating (and requiring) the value of every flag always.

agozillon updated this revision to Diff 501118.Feb 28 2023, 6:33 AM
  • [OpenMP][MLIR] Simplify the RTLModuleFlagsAttr with optional parameters
  • [OpenMP][MLIR] Make set/getRTLFlags work on an operation rather than a builtin.module

Updated to add default parameters to the attribute, allowing optional specifying of values, changed the get/set RTL functions to work a little more generically and added further tests for the slightly improved syntax as it will now only show flags when they're set.

agozillon marked 2 inline comments as done.Mar 1 2023, 12:45 PM

The other option is for Modules to implement an interface and then the Translation can detect this and forward it to the OpenMP Dialect translation in OpenMPToLLVMIRTranslation.cpp.

Could you point me to the LLVMTranslation patch that will insert this info into the LLVM IR module? Is it https://reviews.llvm.org/D144883?

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
80

This is a strong assertion. We have some existing attributes that does not follow this.

177

Eg: Other attribute

The other option is for Modules to implement an interface and then the Translation can detect this and forward it to the OpenMP Dialect translation in OpenMPToLLVMIRTranslation.cpp.

Could you point me to the LLVMTranslation patch that will insert this info into the LLVM IR module? Is it https://reviews.llvm.org/D144883?

The Phabricator patch is not up yet, I've been holding it off as I am waiting to see how the other patches that it depends on change and land (if they do) , and I was a little unsure how to do dependent patches on Phabricator and wanted to avoid over complicating things for reviewers! But it seems that may have had the opposite affect. I will add the lowering as a Phabricator patch as well and link to it here when it is up.

But yes the patch D144883 is related, as it helps the lowering of this attribute when applied to the module, forwarding it on to the translation step in OpenMPToLLVMIRTranslation.cpp. I also utilised the same patch when lowering from a specialised omp.module as well which was quite useful. The lowering itself uses an amendOperation overload in the OpenMPToLLVMIRTranslation.cpp translation step to invoke OpenMPIRBuilder functionality for each parameter of the attribute to emit them as globals.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
80

Sorry, this was me borrowing from the LLVM Dialect in this case and I overlooked the comment, I'll remove it!

agozillon updated this revision to Diff 501828.Mar 2 2023, 4:22 AM
  • [OpenMP][MLIR] Simplify the RTLModuleFlagsAttr with optional parameters
  • [OpenMP][MLIR] Make set/getRTLFlags work on an operation rather than a builtin.module
  • [OpenMP][MLIR] Remove erroneous comment
agozillon marked an inline comment as done.Mar 2 2023, 4:24 AM

Removed the incorrect and stupidly strong comment made by myself in the last patch update.

The other option is for Modules to implement an interface and then the Translation can detect this and forward it to the OpenMP Dialect translation in OpenMPToLLVMIRTranslation.cpp.

Could you point me to the LLVMTranslation patch that will insert this info into the LLVM IR module? Is it https://reviews.llvm.org/D144883?

Hi Kiran,

Here's the application and lowering of this attribute: https://reviews.llvm.org/D145264

It took a little longer than I'd hoped to get the patch up, my apologies. It is dependent on the patches I currently have on-going at the moment in phabricator, I added reference to them inline in the patch.

As for the interface method I would need a little bit of a reference to build towards that if it was the desired direction we want to take!

I do know there is another patch from another member of my team that does something very similar to this patch with the addition of an attribute to the module that then gets lowered inside of OpenMPToLLVMIRTranslation.cpp, and they arrived at that conclusion without any prodding from me and vice versa (although they ended up with a slightly differing implementation of the phabricator patch you referenced from myself)! But it appears there are many ways to do the same thing in MLIR and just because we both came to the same solution doesn't mean it's the right one, so if the interface direction is more appealing and correct then I'm happy to go down that route.

agozillon added a comment.EditedMar 21 2023, 5:26 PM

Sorry to ping this for you busy reviewers, however, I was hoping that this could get reviewed again as the pre-requisite patches required for the lowering of this attribute now exist in tree! Namely the fopenmp-is-device patch and the ability to lower module attributes https://reviews.llvm.org/D145932

What will be done with the attribute in this patch can be found in this sequel patch: https://reviews.llvm.org/D145264, it is in bad need of a rebase, which I'll get around to if this patch gets accepted (or I can do it earlier if it helps any reviewer) as it is mainly a reference for the usage of this attribute for now to aid this review! Showcasing how this attribute will be applied and lowered.

In essence, several flags such as -fopenmp-teams-oversubscription can be provided to Flang and these lower to globals in the LLVM-IR which the OpenMP runtime (or perhaps other compiler backends) can access and use, the attribute in this patch holds the state of these flags and the attribute is then applied to the mlir module if we're compiling for device. Later in the OpenMP Dialect -> LLVM-IR lowering phase inside of OpenMPToLLVMIRTranslation.cpp this attribute is picked up from the MLIR Module via amendOperation and lowered to LLVM-IR globals utilising the existing OpenMPIRBuilder infrastructure.

Sorry to ping

Pinging every week is not too much but considered OK.

FWIW, I think the patch is reasonable. I would remove the "rtl" part, and probably just call it "omp.flags" but otherwise it makes sense.
That said, @kiranchandramohan should give his opinion.

mlir/test/Dialect/OpenMP/attr.mlir
25

verbosity is fine, IMHO. Just want to make sure we catch inconsistencies that we can't handle via a verifier.

Sorry to ping

Pinging every week is not too much but considered OK.

FWIW, I think the patch is reasonable. I would remove the "rtl" part, and probably just call it "omp.flags" but otherwise it makes sense.
That said, @kiranchandramohan should give his opinion.

Thank you @jdoerfert I'll keep that in mind for the future!

Thank you, I'll wait on @kiranchandramohan's opinion before I update the name then, so as to get the next series of review comments done in a single update.

Sorry to ping

Pinging every week is not too much but considered OK.

FWIW, I think the patch is reasonable. I would remove the "rtl" part, and probably just call it "omp.flags" but otherwise it makes sense.
That said, @kiranchandramohan should give his opinion.

Thank you @jdoerfert I'll keep that in mind for the future!

Thank you, I'll wait on @kiranchandramohan's opinion before I update the name then, so as to get the next series of review comments done in a single update.

Sorry for the delay here. I would like to just have a quick check whether having an interface is possible. If not, we can continue with this. I will come back to this tomorrow.

Sorry to ping

Pinging every week is not too much but considered OK.

FWIW, I think the patch is reasonable. I would remove the "rtl" part, and probably just call it "omp.flags" but otherwise it makes sense.
That said, @kiranchandramohan should give his opinion.

Thank you @jdoerfert I'll keep that in mind for the future!

Thank you, I'll wait on @kiranchandramohan's opinion before I update the name then, so as to get the next series of review comments done in a single update.

Sorry for the delay here. I would like to just have a quick check whether having an interface is possible. If not, we can continue with this. I will come back to this tomorrow.

I did not get much time to spend on this. But I have a patch ( https://reviews.llvm.org/D146721) with a trivial Device Interface. We can extend it later on and add attribute interfaces if required. It has two functions from one of @domada 's patch. As it is, the only benefits the interface gives are i) Put all the device related functions into a single place ii) Hide how the values are get and set (uses the module attributes that you have decided to use) iii) Makes it easy to change if there is a different mechanism iv) Also allows for an alternative implementation for other modules where these values are available differently. Please let me know how this looks. Also, given that this is straightforward I would also like to check whether I am missing anything.

Sorry to ping

Pinging every week is not too much but considered OK.

FWIW, I think the patch is reasonable. I would remove the "rtl" part, and probably just call it "omp.flags" but otherwise it makes sense.
That said, @kiranchandramohan should give his opinion.

Thank you @jdoerfert I'll keep that in mind for the future!

Thank you, I'll wait on @kiranchandramohan's opinion before I update the name then, so as to get the next series of review comments done in a single update.

Sorry for the delay here. I would like to just have a quick check whether having an interface is possible. If not, we can continue with this. I will come back to this tomorrow.

I did not get much time to spend on this. But I have a patch ( https://reviews.llvm.org/D146721) with a trivial Device Interface. We can extend it later on and add attribute interfaces if required. It has two functions from one of @domada 's patch. As it is, the only benefits the interface gives are i) Put all the device related functions into a single place ii) Hide how the values are get and set (uses the module attributes that you have decided to use) iii) Makes it easy to change if there is a different mechanism iv) Also allows for an alternative implementation for other modules where these values are available differently. Please let me know how this looks. Also, given that this is straightforward I would also like to check whether I am missing anything.

HI Kiran, no worries, thank you very much for spending the time you could on it! I personally think it's quite a good method of doing things as far as getting and setting things go. If I am correct in my understanding (which perhaps I am not) it would alter this current patch by moving the set/get methods into the interface rather than the OpenMPDialect, but the attribute definition inside of OpenMPOps.td would remain the same? And would the lowering of module attributes within the OpenMPDialect change at all? or would it very much just be the same case of checking if the attribute exists on the operation as far as everything else is concerned (my assumption is it would be the same with my current knowledge of interfaces, I just want to double check).

If you would like and if the interface is a direction we'd like to go down I'd be happy to integrate and give the interface a trial run on this patch and the already upstreamed omp.is_device, and if it goes well I could update this patch with the interface and create a patch based on yours which adds the interface and moves the device attribute into the interface to align what's already upstreamed with it (unless you wish to do so yourself of course). However, it is probably worth waiting till tomorrow to see other feedback on your patch!

Sorry to ping

Pinging every week is not too much but considered OK.

FWIW, I think the patch is reasonable. I would remove the "rtl" part, and probably just call it "omp.flags" but otherwise it makes sense.
That said, @kiranchandramohan should give his opinion.

Thank you @jdoerfert I'll keep that in mind for the future!

Thank you, I'll wait on @kiranchandramohan's opinion before I update the name then, so as to get the next series of review comments done in a single update.

Sorry for the delay here. I would like to just have a quick check whether having an interface is possible. If not, we can continue with this. I will come back to this tomorrow.

I did not get much time to spend on this. But I have a patch ( https://reviews.llvm.org/D146721) with a trivial Device Interface. We can extend it later on and add attribute interfaces if required. It has two functions from one of @domada 's patch. As it is, the only benefits the interface gives are i) Put all the device related functions into a single place ii) Hide how the values are get and set (uses the module attributes that you have decided to use) iii) Makes it easy to change if there is a different mechanism iv) Also allows for an alternative implementation for other modules where these values are available differently. Please let me know how this looks. Also, given that this is straightforward I would also like to check whether I am missing anything.

HI Kiran, no worries, thank you very much for spending the time you could on it! I personally think it's quite a good method of doing things as far as getting and setting things go. If I am correct in my understanding (which perhaps I am not) it would alter this current patch by moving the set/get methods into the interface rather than the OpenMPDialect, but the attribute definition inside of OpenMPOps.td would remain the same? And would the lowering of module attributes within the OpenMPDialect change at all? or would it very much just be the same case of checking if the attribute exists on the operation as far as everything else is concerned (my assumption is it would be the same with my current knowledge of interfaces, I just want to double check).

Yes. most things should remain the same. The only difference will be that we will always use the interface to get and set.

If you would like and if the interface is a direction we'd like to go down I'd be happy to integrate and give the interface a trial run on this patch and the already upstreamed omp.is_device, and if it goes well I could update this patch with the interface and create a patch based on yours which adds the interface and moves the device attribute into the interface to align what's already upstreamed with it (unless you wish to do so yourself of course). However, it is probably worth waiting till tomorrow to see other feedback on your patch!

Yes, it would be great if you can give this a trial. And also wait for any feedback.

  • [OpenMP][MLIR] Rebase on interface and remove RTL from attribute name

Rebased this on the module interface changes, and removed RTL from the attribute name, making it just omp.ModuleFlags now! I'll adjust the commit/title to match the revision changes on acceptance.

May I please get another review of this work @kiranchandramohan

LG.

See Nit comment inside regarding the naming of the attribute. Can these be called DeviceAttributes or something like that?

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
59

Nit: Now that we are using the interface, we can choose to rename the Attr. Or remove the Prefix Module.

This revision is now accepted and ready to land.Mar 29 2023, 2:26 AM

LG.

See Nit comment inside regarding the naming of the attribute. Can these be called DeviceAttributes or something like that?

Do you mean change the OpenMP_Attr that they inherit from to OpenMP_DeviceAttribute (or OffloadAttribute, which may be a little more general for host/device attributes relating to offloading) or to simply add a section comment stating the Flag attributes and friends relate to offloading?

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
59

I'll remove the prefix module during the commit, as I agree it being part of the interface now makes it clear what they're used for without the Module prefix :-)

agozillon updated this revision to Diff 509351.Mar 29 2023, 6:53 AM
  • [OpenMP][MLIR] Change ModuleFlags name to Flags
  • [OpenMP][Flang] Make failing omp-frontend-forwarding.f90 driver test OS agnostic
Herald added a project: Restricted Project. · View Herald Transcript

Changed omp.ModuleFlags -> omp.Flags with the last patch.

I have also included a fix to the failing driver test shown in the prior diff's windows buildbot (the test was added by myself in a prior patch), hoping to piggyback off of this reviews buildbot to make sure the tweak fixes it, as I don't have easy access to a windows build machine at the moment. I'll make separate upstream commits for this patch and the fix however. So as not to confuse the two issues further. Seems to primarily be that on windows the clang-offload-packager is postfixed with .exe!

LG. We might have to find a name for the Flags Attr in later patches.

Fixes for CI issues should land separately.

Thank you very much Kiran! I'll make sure to land them separately.

I think if we need a more specific name in the future RuntimeLibraryFlags or RTLFlags may be appropriate. It may be fine to keep it as Flags and add other more general flags to the attribute however, if it's required by future patches.