This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][Flang][MLIR] Implement OffloadModuleInterface for OpenMP Dialect and convert is_device to an Attribute
ClosedPublic

Authored by agozillon on Mar 24 2023, 3:41 PM.

Details

Summary

This commit adds the OffloadModuleInterface to the OpenMP dialect,
which will implement future module attribute get/set's for offloading.
Currently it implements set and get's for the omp.is_device attribute,
which is promoted to a real attribute in this commit as well (primarily
to allow switch cases to work nicely with it for future work and to keep
consistency with future module attributes).

Diff Detail

Event Timeline

agozillon created this revision.Mar 24 2023, 3:41 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
agozillon requested review of this revision.Mar 24 2023, 3:41 PM

A patch based on https://reviews.llvm.org/D146721 by @kiranchandramohan an attempt at adding a module interface for OpenMP offload related attributes and functions that apply to modules. Using the existing omp.is_device work as the initial attribute (although this has been tested with the RTLModuleFlag's patch i have up and it works fine as well).

It unfortunately wasn't as simple as just adding the ODS definition, it appears that a concrete model for the interface must also be defined (and attached rather than the interface) for you to get access to the default methods defined in the ODS. But this is my first attempt at using interfaces like this, so perhaps I am missing something, please do bring it up if there is a better method! I found that just using the interface as in the original proposed patch will result in a runtime assertion whenever you try to access the interface functions as I do in this patch (even with pre-defined default ODS implementations). Perhaps when you attach an interface to an operation using the ODS framework as opposed to the attachInterface it does some table gen magic to create the Model for the operation.

agozillon updated this revision to Diff 508241.Mar 24 2023, 4:10 PM
  • [MLIR][OpenMP] Newline at end of OpenMPInterfaces.h file

Thanks @agozillon for the changes. Looks fine.

It unfortunately wasn't as simple as just adding the ODS definition, it appears that a concrete model for the interface must also be defined (and attached rather than the interface) for you to get access to the default methods defined in the ODS.

Sorry about the difficulty here. I guess this is necessary. As I mentioned, I did not have sufficient time to go through this in detail but wanted to give some direction.

Did you (or @domada) check whether this can be used in the translation phase (where we connect with the OpenMP IRBuilder) as well?

Also, is there a way to test that other top-level builtin modules (if any) are not affected by this change? I guess in the flang flow there is only one.

flang/lib/Frontend/FrontendActions.cpp
227–228

Can this code be shared with bbc?

mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h
31

Nit: Is this change required?

Thanks @agozillon for the changes. Looks fine.

It unfortunately wasn't as simple as just adding the ODS definition, it appears that a concrete model for the interface must also be defined (and attached rather than the interface) for you to get access to the default methods defined in the ODS.

Sorry about the difficulty here. I guess this is necessary. As I mentioned, I did not have sufficient time to go through this in detail but wanted to give some direction.

No need to apologies! I just wanted to give some context to the changes from your original patch that's all, always happy to learn more.

Did you (or @domada) check whether this can be used in the translation phase (where we connect with the OpenMP IRBuilder) as well?

Yes, I have tested it, I can cast to the interface and call getIsDevice and get the correct results, so it appears to be functioning fine even in the OpenMPToLLVMIRTranslation phase, no unfortunate discarding of the interface thankfully!

Also, is there a way to test that other top-level builtin modules (if any) are not affected by this change? I guess in the flang flow there is only one.

I don't know of an easy way to test this unfortunately, however, yes, in the Flang flow there is only one module at a time (in MLIR in general it seems to assume only one builtin.module, you'll run into errors with existing tools if you have more than one (or none the only reason it won't complain in these cases is that the tools wrap MLIR implicitly in builtin.modules)). I am fairly certain it'll attach to other builtin modules in the given context (which will just be the one). If we do ever end up with two in a single context, it shouldn't be an issue as long as code segments are written based off of the contained attributes on the module, and not the existence of the interface itself I believe.

flang/lib/Frontend/FrontendActions.cpp
227–228

I use the same code in bbc at the moment a little lower in this patch :-) However, if you mean if this code can be shared across both the bbc tool and the Flang frontend then I am not so sure, I believe they do not run through the same paths, they create separate LoweringBridges and Modules in different code locations to generate their output.

mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h
31

In this case, we need a place to declare/define our C++ implementations of interfaces (models or if someone eventually decides to create an interface in C++) I thought separating it out into a separate file may be prudent. You also can't get away with forward declaring the model inside of OpenMPDialect.h as you need the fully defined class quite early on in compilation (although you could define the full model inside of the OpenMPDialect.h, but that may be messier than just the header, I am happy to do this though if that's what we wish or any other suggestions).

The ordering is also important in this case, the Module Interface (and possibly others in the future) rely on the OMP MLIR Attributes, and the ODS implementation requires them to be defined before use in either the ODS definition or the C++ definition, forward declaring the attributes inside of OpenMPDialect.h doesn't satisfy the compiler in this instance unfortunately.

LGTM.

Ideally, it should be an interface whose existence can be checked to see whether it is a device Module. But I guess we can proceed for now with this. Later on we can investigate if an attribute interface would have been a better choice here.

Did you (or @domada) check whether this can be used in the translation phase (where we connect with the OpenMP IRBuilder) as well?

Yes, I have tested it, I can cast to the interface and call getIsDevice and get the correct results, so it appears to be functioning fine even in the OpenMPToLLVMIRTranslation phase, no unfortunate discarding of the interface thankfully!

OK. That is great. Hope it is working fine at the translation phase for both with and without attributes.

Also, is there a way to test that other top-level builtin modules (if any) are not affected by this change? I guess in the flang flow there is only one.

I don't know of an easy way to test this unfortunately, however, yes, in the Flang flow there is only one module at a time (in MLIR in general it seems to assume only one builtin.module, you'll run into errors with existing tools if you have more than one (or none the only reason it won't complain in these cases is that the tools wrap MLIR implicitly in builtin.modules)). I am fairly certain it'll attach to other builtin modules in the given context (which will just be the one). If we do ever end up with two in a single context, it shouldn't be an issue as long as code segments are written based off of the contained attributes on the module, and not the existence of the interface itself I believe.

I hope we can test with mlir-translate at the translation phase with two modules where one has the attributes and the other does not.

flang/lib/Frontend/FrontendActions.cpp
227–228

So, I meant some function that given a module attaches the interface, applies the interface and inserts all the attributes. If that can be placed in a common place (/flang/include/flang/Tools ?) then both bbc and frontendactions can share it.

This revision is now accepted and ready to land.Mar 27 2023, 10:17 AM
awarzynski accepted this revision.Mar 27 2023, 10:23 AM

Thanks, LGTM!

LGTM.

Ideally, it should be an interface whose existence can be checked to see whether it is a device Module. But I guess we can proceed for now with this. Later on we can investigate if an attribute interface would have been a better choice here.

True, you can't check if it's a device module via the interface, but you can through the is_device attribute. Which is similar to Clang, just a simple flag check, perhaps not ideal, but it works.

Did you (or @domada) check whether this can be used in the translation phase (where we connect with the OpenMP IRBuilder) as well?

Yes, I have tested it, I can cast to the interface and call getIsDevice and get the correct results, so it appears to be functioning fine even in the OpenMPToLLVMIRTranslation phase, no unfortunate discarding of the interface thankfully!

OK. That is great. Hope it is working fine at the translation phase for both with and without attributes.

I believe it currently does and it breaks no existing tests.

Also, is there a way to test that other top-level builtin modules (if any) are not affected by this change? I guess in the flang flow there is only one.

I don't know of an easy way to test this unfortunately, however, yes, in the Flang flow there is only one module at a time (in MLIR in general it seems to assume only one builtin.module, you'll run into errors with existing tools if you have more than one (or none the only reason it won't complain in these cases is that the tools wrap MLIR implicitly in builtin.modules)). I am fairly certain it'll attach to other builtin modules in the given context (which will just be the one). If we do ever end up with two in a single context, it shouldn't be an issue as long as code segments are written based off of the contained attributes on the module, and not the existence of the interface itself I believe.

I hope we can test with mlir-translate at the translation phase with two modules where one has the attributes and the other does not.

I'm not sure how this would test that the interface is not attached to other modules, but I may be misunderstanding what you mean :-) As far as I am aware the only time this interface exists on a module is during either the bbc tool or Flang-new's execution, it's not present in the IR and will not be parsed in or out at any point. The only thing that will be present are the attributes if they've been applied and the interface being attached doesn't mean that the entire subset of attributes are automatically applied, that will likely depend on the flags provided and whether it's a host or device pass.

This more general MLIR test may answer the question, if I am of course understanding the original question which I may not be (so I'm very sorry if I'm missing the point!): https://github.com/llvm/llvm-project/blob/main/mlir/unittests/IR/InterfaceAttachmentTest.cpp#L332 The interface should not be present on other modules, provided they are utilising different MLIRContexts, if they share the same context then it will. In both the case of Flang and bbc, a new MLIRContext appears to be generated on a per source file basis, and only one module is generated per source file (and as far as mlir-translate is concerned at the moment appears to be legal per file), so modules should not accidentally have this interface applied to them, the only time it will be present is if the -fopenmp flag has been given on the command line (or been generated as an argument by something like split compilation).

Thank you both for the review!

  • [FLANG] Add helper function for applying interface and setting attributes
agozillon added a comment.EditedMar 27 2023, 1:00 PM

As requested by @kiranchandramohan I've updated the patch with a cross tool helper function that allows shared assignment of the attributes and interfaces, so no attriibute gets left behind! In theory at least. As there may be some Flags bbc will not support (so the attribute will be set to a default or not applied).

If you are both still happy with the patch and ready for it to land as is, I would appreciate a final acceptance after the last change please @kiranchandramohan @awarzynski and then I'll land it and close this tomorrow (Tuesday 28th).

Thank you both very much.

flang/lib/Frontend/FrontendActions.cpp
227–228

Ah, thank you for clarifying! I have now made one, hopefully it is in a reasonable location. I made a new file for general purpose cross tool functionality as there didn't appear to be a clear place within the Tools subdirectory.

agozillon updated this revision to Diff 508770.Mar 27 2023, 1:10 PM
  • [FLANG] Add helper function for applying interface and setting attributes

LGTM.

Ideally, it should be an interface whose existence can be checked to see whether it is a device Module. But I guess we can proceed for now with this. Later on we can investigate if an attribute interface would have been a better choice here.

True, you can't check if it's a device module via the interface, but you can through the is_device attribute. Which is similar to Clang, just a simple flag check, perhaps not ideal, but it works.

Did you (or @domada) check whether this can be used in the translation phase (where we connect with the OpenMP IRBuilder) as well?

Yes, I have tested it, I can cast to the interface and call getIsDevice and get the correct results, so it appears to be functioning fine even in the OpenMPToLLVMIRTranslation phase, no unfortunate discarding of the interface thankfully!

OK. That is great. Hope it is working fine at the translation phase for both with and without attributes.

I believe it currently does and it breaks no existing tests.

Also, is there a way to test that other top-level builtin modules (if any) are not affected by this change? I guess in the flang flow there is only one.

I don't know of an easy way to test this unfortunately, however, yes, in the Flang flow there is only one module at a time (in MLIR in general it seems to assume only one builtin.module, you'll run into errors with existing tools if you have more than one (or none the only reason it won't complain in these cases is that the tools wrap MLIR implicitly in builtin.modules)). I am fairly certain it'll attach to other builtin modules in the given context (which will just be the one). If we do ever end up with two in a single context, it shouldn't be an issue as long as code segments are written based off of the contained attributes on the module, and not the existence of the interface itself I believe.

I hope we can test with mlir-translate at the translation phase with two modules where one has the attributes and the other does not.

I'm not sure how this would test that the interface is not attached to other modules, but I may be misunderstanding what you mean :-) As far as I am aware the only time this interface exists on a module is during either the bbc tool or Flang-new's execution, it's not present in the IR and will not be parsed in or out at any point. The only thing that will be present are the attributes if they've been applied and the interface being attached doesn't mean that the entire subset of attributes are automatically applied, that will likely depend on the flags provided and whether it's a host or device pass.

This more general MLIR test may answer the question, if I am of course understanding the original question which I may not be (so I'm very sorry if I'm missing the point!): https://github.com/llvm/llvm-project/blob/main/mlir/unittests/IR/InterfaceAttachmentTest.cpp#L332 The interface should not be present on other modules, provided they are utilising different MLIRContexts, if they share the same context then it will. In both the case of Flang and bbc, a new MLIRContext appears to be generated on a per source file basis, and only one module is generated per source file (and as far as mlir-translate is concerned at the moment appears to be legal per file), so modules should not accidentally have this interface applied to them, the only time it will be present is if the -fopenmp flag has been given on the command line (or been generated as an argument by something like split compilation).

Thank you both for the review!

I was assuming that you will use the interface to access the attributes during the translation phase as well and hence this can be tested with mlir-translate independent of the flang flow.

agozillon updated this revision to Diff 508981.Mar 28 2023, 6:17 AM
  • [FLANG][OpenMP][MLIR] Move offload interface registration to OpenMPDialect
agozillon updated this revision to Diff 508982.Mar 28 2023, 6:19 AM
  • [FLANG][OpenMP][MLIR] Move offload interface registration to OpenMPDialect

I have moved the Model (interface) registration into the OpenMPDialect, so now it should be registered for all mlir::ModuleOp's in tools that initialise the OpenMPDialect (which should include mlir-translate). The mlir-translate test will come in an additional patch when there is an module attribute that can be appropriately tested in the OpenMP -> LLVM-IR lowering phase, likely in the following patch when rebased on this and other changes: https://reviews.llvm.org/D145264 as discussed external to the patch.

If it's possible could I ask for a final acceptance for this diff, before I commit the patch? @kiranchandramohan @awarzynski

LGTM. Thanks for the patch and addressing the change requests.

flang/tools/bbc/bbc.cpp
248–249

Nit: Braces are not required.