• [MLIR][OpenMP] Newline at end of OpenMPInterfaces.h file

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

In D144896#4216544, @kiranchandramohan wrote:

In D144896#4213015, @kiranchandramohan wrote:

In D144896#4212840, @agozillon wrote:

In D144896#4211723, @jdoerfert wrote:

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.

In D144896#4211723, @jdoerfert wrote:

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.

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! Namely the fopenmp-is-device patch and the ability to lower module attributes https://reviews.llvm.org/D145932

I believe I understand your concerns.

Going to abandon and close this revision as the Reviewers point was implemented and accepted in the following patch https://reviews.llvm.org/D145932/ by @skatrak, thank you very much @skatrak for looking into this! And thank you @ftynse for recommending a better direction.

May I please request a final acceptance from both @jhuber6 and @awarzynski before I commit this upstream! If you have no further comments to add or requests of course.

Recent commit added some more detail to the comment on the fembed-offload-object argument as requested by @awarzynski

• [Flang][Driver] Expand further on the embed-offload-object comment

Deleted the clang test that was forwarding to Flang and merged with the omp-frontend-forwarding.f90 test where relevant. Second push was because I forgot to add a missing newline, which I seem to do frequently...

• Readd missing newline

In D145815#4199848, @tschuett wrote:

Flang will also support OpenACC for offload. It is very similar to OpenMP.

• [Flang][Driver] Try to fix failing omp-frontend-forwarding.f90 test
• [Flang][Driver] Simplify omp-frontend-forwarding.f90 test
• [Flang][Driver][Test] Delete flang-omp test from Clang and merge tests into omp-frontend-forwarding.f90

Recent update was to simplify the omp-frontend-forwarding.f90 test

• [Flang][Driver] Simplify omp-frontend-forwarding.f90 test

• [Flang][Driver] Try to fix failing omp-frontend-forwarding.f90 test

Cleaned up the test, thank you both for teaching me more about the compiler and test infrastructure! If you're happy with the test changes @kiranchandramohan I'll commit the changes upstream to close out the review?

• [Flang][Driver] Tidy up omp-is-device.f90 test

In D144864#4175257, @kiranchandramohan wrote:

LG. See one minor comment in the tests.

I would prefer having an Interface for Target Modules if that could be made to work. I guess this can be taken up separately after https://reviews.llvm.org/D144883.

The recent update removed the added offload related code from Flang.h/.cpp and removed the related test.

• [Flang][ToolChain][OpenMP][Driver] Reduce changes to only add -fc1 support

In D144896#4164152, @kiranchandramohan wrote:

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?

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:

Thank you very much @awarzynski I will wait for further approval!

The new update adds the suggested new tests! Thank you very much @awarzynski for suggesting them that was a great help. Hopefully the new additions cover what you have requested, if not I am of course happy to change or add more tests as needed.

• [Flang][MLIR][Test] Add newline at end of test file

• [MLIR][OpenMP] Fix attribute helpers to apply to more than builtin.module
• [Flang][mlir] Adding space at the end of the omp-is-device test file
• [Clang][Flang][Driver][Test] Add flang-omp.f90 to test fopenmp/fopenmp-is-device for flang driver-mode
• [Flang][MLIR][OpenMP] Add additional tests to omp-is-device.f90 test

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

• [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

In D144896#4164152, @kiranchandramohan wrote:

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?

After asking @jsjodin and @skatrak for some input on additional tests that could be added and having a look around the existing tests we couldn't really come up with any additional tests to add to the ones that are in the patch at the moment (inside of omp-is-device.f90 and driver-help.f90) unfortunately. Primarily as there isn't any use of this attribute yet, although there will be quite soon.

• [Flang][mlir] Adding space at the end of the omp-is-device test file

Thank you @jeanPerier for the help and review, that's it landed now, sorry for making the FIR IR reading harder for you for the time being!

After a little bit of time going over the suggested change of ditching convertModuleOperation and using the existing convertOperation infrastructure, I can see two ways of doing it:

In D144883#4158684, @ftynse wrote:

This allows existing dialect modules such as gpu.module to continue to not do dialect specific lowering, allowing them to opt in or out based on their dialects needs.

This is why I also mentioned that we need to "teach the default translation to do nothing with it". Can't we add the interface implementation that returns success instead of hardcoding it as default?

In D144869#4157638, @jeanPerier wrote:

adding it as a discussion point in one of the weekly meetings or is an RFC on discourse perhaps better?

We can briefly discuss it at the meeting tomorrow to check no one rely on not having the module printed.

• [MLIR][OpenMP] Fix attribute helpers to apply to more than 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.

• [OpenMP][MLIR] Simplify the RTLModuleFlagsAttr with optional parameters
• [OpenMP][MLIR] Make set/getRTLFlags work on an operation rather than a builtin.module

In D144883#4156106, @ftynse wrote:

Why does it have to be a separate interface function? Module is just the builtin.module operation, it looks like we can just call convertOperation for it and teach the default translation to do nothing with it.

In D144896#4156571, @kiranchandramohan wrote:

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?

In D144883#4156106, @ftynse wrote:

Why does it have to be a separate interface function? Module is just the builtin.module operation, it looks like we can just call convertOperation for it and teach the default translation to do nothing with it.

In D144864#4156764, @jdoerfert wrote:

Tests?

In D144869#4155134, @kiranchandramohan wrote:

This will need a discussion with the FIR team.

Forgot to add an additional test in initial patch!

Thank you for the suggestion, I made the simplification of reducing the clause names, sorry for missing your comment for so long. I had to update which email my notifications were being sent to!

• [MLIR][OpenMP] Change clauses to there shorthand

In D142910#4099456, @jsjodin wrote:

In D142910#4099238, @agozillon wrote:

In D142910#4097813, @kiranchandramohan wrote:

In D142910#4096441, @agozillon wrote:

From my current work on Declare Target I don't think you necessarily need an omp.function at least so far, you can distinguish what is a device function via other OpenMP operations that are device outlining directives in OpenMP like Declare Target, but the work on Declare Target is very much in the early stages (https://github.com/ROCm-Developer-Tools/llvm-project/pull/185), so we may find some problems along the way. I think the benefits I see of omp.function is perhaps that it may make the code generation process easier, as you can perhaps just directly generate an omp.function from Declare Target clause members that are functions, the global data could be packaged in the declare.target operation, so we effectively skip the middle man. It also makes sure that the device/target lowering that may be necessary is easily separable from the regular FuncOp lowering.

If we require omp.function (may be we should call it omp.target_function) then we would require something like an omp.target_variable as well isn't it?

That is likely true, and maybe a more general approach than having omp.target_declare if there are other ways to create outlineable global variables (which I imagine there is) and just generally a little more flexible for outlined variables. For now I am waiting to see the results of the poll / discussion to see which way everyone leans on this topic!

Compared to having an attribute like omp.function() { device_type = any }, in the case of "any" and having omp.target_function and omp.function, would there be one of each, or would only one be generated depending on the current target?

In D142910#4097813, @kiranchandramohan wrote:

In D142910#4096441, @agozillon wrote:

From my current work on Declare Target I don't think you necessarily need an omp.function at least so far, you can distinguish what is a device function via other OpenMP operations that are device outlining directives in OpenMP like Declare Target, but the work on Declare Target is very much in the early stages (https://github.com/ROCm-Developer-Tools/llvm-project/pull/185), so we may find some problems along the way. I think the benefits I see of omp.function is perhaps that it may make the code generation process easier, as you can perhaps just directly generate an omp.function from Declare Target clause members that are functions, the global data could be packaged in the declare.target operation, so we effectively skip the middle man. It also makes sure that the device/target lowering that may be necessary is easily separable from the regular FuncOp lowering.

If we require omp.function (may be we should call it omp.target_function) then we would require something like an omp.target_variable as well isn't it?

In D142918#4096668, @clementval wrote:

LGTM. Thanks

Sorry, done, I've added the fully qualified names.

• [FLANG][MLIR] remove MangleNameOnCallOp

In D142910#4095988, @kiranchandramohan wrote:

As from my current understanding Declare Target also indicates that global data is to be offloaded not just functions and I think we need some way to encode that in the OpenMP dialect so the data can be lowered properly for host and device inside of the OpenMPIRBuilder.

I was assuming that you will be using the same mechanism for global variables as well since global variables also have symbols. Do you see any issues here?

In D142918#4096251, @clementval wrote:

In D142918#4096057, @agozillon wrote:

In D142918#4095817, @clementval wrote:

It looks like the right thing to do but I think we can remove the MangleNameOnCallOp because it should be updated by the op.replaceAllSymbolUses. That's why you don't see the issue with fir.call right now but if you remove the conversion pattern it should show up.

Thank you, I'll check that out and update the patch, there are fir::GlobalOp and fir::AddrOfOp rewrites as well that may run into a similar issue, do you believe it'd be sensible to remove these and rely on FuncOp to rewrite them with the symbol update from replaceAllSymbolUses or add a replaceAllSymbolUses within both? or alternatively just leave them as is!

You can leave fir::GlobalOp and fir::AddrOfOp for now. At least GlobalOp conversion pattern will have to stay because it is not related to funcOp in any way .

• [FLANG][MLIR] remove MangleNameOnCallOp

In D142918#4095817, @clementval wrote:

It looks like the right thing to do but I think we can remove the MangleNameOnCallOp because it should be updated by the op.replaceAllSymbolUses. That's why you don't see the issue with fir.call right now but if you remove the conversion pattern it should show up.

Used arcanist for the most recent update, hopefully it's done as expected and appended the extra context requested, if not I can try again. Thank you for introducing me to Arcanist, it's a lot smoother than the web API, not sure why I was avoiding it before!

• [MLIR][OpenMP] Add tests for Declare Target op

In D142918#4094488, @clementval wrote:

In D142918#4094454, @agozillon wrote:

Thank you, I've added a test now, in this case I've used func.call, rather than fir.call, it seems to affect the former but not the latter after some testing with and without the change.

F18 never uses func.call and I guess the problem doesn't show up with fir.call because we are also modifying the symbol in fir.call operations. Do you have an example of the initial issue you had in the first place?

In D142918#4093479, @clementval wrote:

In D142918#4093472, @agozillon wrote:

In D142918#4091445, @clementval wrote:

Please add a test.

More than happy to, I wasn't too sure where to add a test for this originally which is why I did not add one, I apologies! Is flang/test/Fir/external-mangling.fir perhaps the best place or is there somewhere more relevant or additional locations? And do you think an apt test would be a FuncOp containing an operation that contains a symbol pointing to the FuncOp and testing if it rewrites the contained symbol after the --external-name-interop pass (if that is the correct pass in this case)?

Thank you very much for your review feedback and help.

No worries. Yes, flang/test/Fir/external-mangling.fir is the right place. A simple funcOp and a fir.call that point to this funcOp should be enough to test this.

Adding a test that will show regression of the problem. I have used func.call rather than fir.call as these seem to be unaffected but the func.call's appear to display the issue.

In D142918#4091445, @clementval wrote:

Please add a test.

More than happy to do that, I will do so after I've completed the requested changes! Do you know if it Is possible to just alter the existing review with new diff data from arcanist or would it require a completely new Phabricator review?