[MLIR][OpenMP][FLANG] Adding MLIR OpenMP dialect Operation for OpenMP's Declare Target

[agozillon]

Summary

For a block of Fortran code like this:

FUNCTION FUNC() RESULT(X)
    !$omp declare target
    INTEGER :: X
    X = 1
END FUNCTION FUNC

A target.declare operation of the following form would be created:

omp.target.declare to([@_QPfunc]) device_type(devicetypehost)

Where @_QPfunc is an MLIR symbol reference to the created MLIR FuncOp.

This patch includes an MLIR OpenMP Dialect operation added to OpenMPOps.td which takes 3 arguments representing the directives clauses.

Added a custom verifier which currently just checks that at least a To or Link argument has been populated.

Added a test case to ops.mlir to verify the appropriate syntax for the operation, for the moment it only checks functions, Declare Target also supports global variables, this will need some further testing, I am currently focusing on the use of functions with Declare Target.

Of note is that this currently does not factor in the C++ syntax begin and end region which does not exist in Fortran, however, in the current implementation, they could likely all be squished from the begin and end directives into a single operation packaging everything contained within (including the clauses).

[agozillon]

Rebase on top of recently applied patches to pass the pre-merge check and keep patch fresh.

[kiranchandramohan]

Could you post the patch with more context, using the options listed in https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line?

We are discussing operations for supporting target in https://discourse.llvm.org/t/rfc-omp-module-and-omp-function-vs-dialect-attributes-to-encode-openmp-properties/67998. I guess that discussion would have some effect on this patch. If you are not working with @jsjodin could you comment there on your plans?

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

1362

You will have to add a test for this in mlir/test/Dialect/OpenMP/invalid.mlir.

mlir/test/Dialect/OpenMP/ops.mlir

1863

Put these as separate tests using ----.

We might want to have some symbols to refer to in this test (ie. copy @add_f32 to this set of tests).

[agozillon]

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?

I am working in the same team as Jan and this is one of the components we're looking at for offloading at the moment, I'm aware of the discussion, however, thank you very much for bringing it up, I am very happy to make changes based on the outcome of the discussion. However, I think regardless of the outcome, this operation is still likely to be needed (however, maybe the current format isn't good) or something similar. 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.

For the moment, I am primarily using it for function outlining/offloading and this operation helps to indicate that a function is required for a specific module, it currently allows me to search up a function tied to a symbol within OpenMPToLLVMIRTranslation.cpp and then modify it, at the moment I am mainly tagging the functions during lowering to LLVM with LLVM IR metadata (as we're still work shopping what needs to be done at this point) and I am trying to remove functions that are deemed unnecessary to the module as well based on a mismatch between device_type and the target destination of the module. However, in theory if we went down the OpenMP function operation route it could maybe be used to rewrite tagged functions from FuncOps -> OpenMP.FuncOps with specialist information for Declare Target (but perhaps this would be better done in the initial CodeGen from the parse tree).

I am very new to MLIR, Fortran, FLANG and OpenMP so any and all feedback, discussion and information is greatly appreciated as I am very sure my understanding is lacking in a lot of areas.

Thank you very much for your time and help.

[agozillon]

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

[agozillon]

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/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

1362

Added tests for this, although I may have slightly overkilled it for what the error is.

mlir/test/Dialect/OpenMP/ops.mlir

1863

Hopefully this is what you meant!

[kiranchandramohan]

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?

For the moment, I am primarily using it for function outlining/offloading and this operation helps to indicate that a function is required for a specific module, it currently allows me to search up a function tied to a symbol within OpenMPToLLVMIRTranslation.cpp and then modify it, at the moment I am mainly tagging the functions during lowering to LLVM with LLVM IR metadata (as we're still work shopping what needs to be done at this point) and I am trying to remove functions that are deemed unnecessary to the module as well based on a mismatch between device_type and the target destination of the module.

OK. If this operation is tagging the functions that are to be offloaded then it might make the case for omp.function weaker. However, should we delay the removal of functions that are not tagged to the Translation?

[agozillon]

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?

I intend to use the same mechanism and it appears it will work, as you can lookup global symbols in the module in the same manner to functions (both LLVM/OpenMP dialect and the in progress LLVM module) OpenMPLLVMIRTranslation.cpp, so I don't see any issues here, but I am cautiously optimistic as I am new to MLIR and the OpenMP CodeGen process, so I am very open to feedback if anyone perceives any issues from this design.

For the moment, I am primarily using it for function outlining/offloading and this operation helps to indicate that a function is required for a specific module, it currently allows me to search up a function tied to a symbol within OpenMPToLLVMIRTranslation.cpp and then modify it, at the moment I am mainly tagging the functions during lowering to LLVM with LLVM IR metadata (as we're still work shopping what needs to be done at this point) and I am trying to remove functions that are deemed unnecessary to the module as well based on a mismatch between device_type and the target destination of the module.

OK. If this operation is tagging the functions that are to be offloaded then it might make the case for omp.function weaker. However, should we delay the removal of functions that are not tagged to the Translation?

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.

As for the delayed removal, do you mean perhaps it's better to remove them earlier in the initial lowering to MLIR by simply removing the function that is irrelevant (based on if the current compilation pass is a device pass or host pass and the device_type) during lowering of the initial Declare Target parse tree construct Inside of flang/lib/Lower/OpenMP.cpp? That is one of the options I am considering, but I've not dug in deep to it too see if any problems arise, but if it is the direction we wish to go in I am more than happy to test it. I do believe a Declare Target operation may still be needed though if the data outlining works similarly to data mapping where there is some host transferal required to the device, if it isn't similar then it could probably be handled the same way as the functions where we remove them early from the generated MLIR module.

[kiranchandramohan]

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?

As for the delayed removal, do you mean perhaps it's better to remove them earlier in the initial lowering to MLIR by simply removing the function that is irrelevant (based on if the current compilation pass is a device pass or host pass and the device_type) during lowering of the initial Declare Target parse tree construct Inside of flang/lib/Lower/OpenMP.cpp? That is one of the options I am considering, but I've not dug in deep to it too see if any problems arise, but if it is the direction we wish to go in I am more than happy to test it. I do believe a Declare Target operation may still be needed though if the data outlining works similarly to data mapping where there is some host transferal required to the device, if it isn't similar then it could probably be handled the same way as the functions where we remove them early from the generated MLIR module.

Yeah, the removal of non-target code can possibly be done at several stages. In semantics by modifying the parse-tree, during lowering by appropriate passing target info to the bridge, as a pass in the OpenMP dialect, during conversion from OpenMP to LLVM Dialect, in Translation and may be in the OpenMPIRBuilder or an openmp-opt pass. Ideally, we would want the translation code to be kept to a minimum. Also, it is not clear to me whether we can delete llvm.funcs while translating OpenMP ops.

[agozillon]

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!

As for the delayed removal, do you mean perhaps it's better to remove them earlier in the initial lowering to MLIR by simply removing the function that is irrelevant (based on if the current compilation pass is a device pass or host pass and the device_type) during lowering of the initial Declare Target parse tree construct Inside of flang/lib/Lower/OpenMP.cpp? That is one of the options I am considering, but I've not dug in deep to it too see if any problems arise, but if it is the direction we wish to go in I am more than happy to test it. I do believe a Declare Target operation may still be needed though if the data outlining works similarly to data mapping where there is some host transferal required to the device, if it isn't similar then it could probably be handled the same way as the functions where we remove them early from the generated MLIR module.

Yeah, the removal of non-target code can possibly be done at several stages. In semantics by modifying the parse-tree, during lowering by appropriate passing target info to the bridge, as a pass in the OpenMP dialect, during conversion from OpenMP to LLVM Dialect, in Translation and may be in the OpenMPIRBuilder or an openmp-opt pass. Ideally, we would want the translation code to be kept to a minimum. Also, it is not clear to me whether we can delete llvm.funcs while translating OpenMP ops.

Yes, it's interesting to see which stage would be better, I agree that earlier is very likely better than later, my initial approach was to remove llvm.funcs from the module that aren't desired and tag LLVM IR functions with metadata to state they are required by the module and then a later llvm-opt pass could remove anything not marked with metadata (by target declare or any other outlineable directive) as needed. The removal of llvm.funcs from the module is proving a little difficult so far, but I need to dig into it a little deeper. But I am going to test the early removal first I think.

[jsjodin]

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?

[agozillon]

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?

For me personally if we went down the omp.function route I think only having one type of omp.function operator is perhaps better, then we only generate one inside of the module for "any" and have it tagged with device_type = any and then during the lowering each individual lowering phase (host + target/s) can check some global information (module or global variable or omp.module) to see if it is destined for offloading or not and do the appropriate lowering. If the omp.function operator started to become monolithic because it needs to do more than we expected, we could always fracture it into the two variants.

I think the case of generating two essentially duplicate function operations (omp.function or leave it as an mlir.func and omp.target_function) could work too, and is quite similar to omp.target.declare, except in the case of the omp.target.declare we just hold a symbol reference to the function rather than a region. It just adds a little more effort to clean up the module for each device afterwards, but we retain all information for host and device which might be good for cross host-device boundary optimisations in the future. And I think this is one of the benefits of not discarding functions early on (late outlining vs early outlining) if they are not needed within a module, it could still be possible to create optimisations with global information of both host and device.

[TIFitis]

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

976–978

I think the op will look cleaner in the MLIR if you drop the "devicetype" prefix.

[agozillon]

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

[agozillon]

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!