This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][MLIR] Add OpenMP version attribute to OMP dialect
ClosedPublic

Authored by domada on May 11 2023, 3:26 AM.

Details

Summary

The intent of OpenMP version attribute is to be applied to a module and then hold information on OpenMP version flag.

Information about OpenMP version can be specified in Clang by flag -fopenmp-version and it is stored in LLVM-IR module metadata:

!llvm.module.flags = !{!0, !1}
!0 = !{i32 7, !"openmp", i32 51}
!1 = !{i32 7, !"openmp-device", i32 51}

OpenMP MLIR version attributes will allow to lower Flang frontend flag to OpenMP MLIR code and then to LLVM IR.

There are two OpenMP MLIR version attributes. The first one "omp.version" MLIR attribute corresponds to host OpenMP version. The second one corresponds to "openmp-device" LLVM-IR metadata. "openmp-device" LLVM-IR metadata is attached only for offloaded code.

Diff Detail

Event Timeline

domada created this revision.May 11 2023, 3:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2023, 3:26 AM
domada requested review of this revision.May 11 2023, 3:26 AM

Will the translation to LLVM be through amendOperation?

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

There seem to be two different defaults, 4.5 and 5.0 (In the Interface).

What is the real default and is it different for openmp and openmp-device?

domada updated this revision to Diff 521250.May 11 2023, 4:02 AM

Fixed default version of OpenMP

domada added inline comments.May 11 2023, 4:04 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
75

I corrected it to 50. In clang we use one flag to specify the version of OpenMP for the device and host. By default this flag is set to 50.

Will the translation to LLVM be through amendOperation?

That's my plan. If you wish I can wait till the output of @skatrak proposal: https://reviews.llvm.org/D150275

Will the translation to LLVM be through amendOperation?

That's my plan. If you wish I can wait till the output of @skatrak proposal: https://reviews.llvm.org/D150275

No need to wait. I believe that proposal was only to demonstrate that there is a way for out of tree users to use the Offload Attributes in the Dialect.
Also, if it uses amendOperation then you can consider removing the getVersion since we will not be using it.

Another question I have is whether the flags attribute will always be present with -fopenmp or only with some device/target specific options.

Will the translation to LLVM be through amendOperation?

That's my plan. If you wish I can wait till the output of @skatrak proposal: https://reviews.llvm.org/D150275

My patch was just to show that there are alternatives, purely within MLIR, to the amendOperation flow for lowering discardable dialect attributes attached to operations from other dialects. AFAIK there's no intention to use that approach for the time being at least.

agozillon added a comment.EditedMay 11 2023, 4:33 AM

Will the translation to LLVM be through amendOperation?

That's my plan. If you wish I can wait till the output of @skatrak proposal: https://reviews.llvm.org/D150275

No need to wait. I believe that proposal was only to demonstrate that there is a way for out of tree users to use the Offload Attributes in the Dialect.
Also, if it uses amendOperation then you can consider removing the getVersion since we will not be using it.

Another question I have is whether the flags attribute will always be present with -fopenmp or only with some device/target specific options.

Currently the flags were only present on device (and the original intent was it only being these flags, but it makes sense for it to extend broader to cover these flags as well), but that was because the flags that were in the attribute originally are only ever present on the device/target. However, all you would need to do to change this is move the is_device check that applies them in the frontend (initial attribute creation) to the back end (MLIR attribute -> LLVM IR) and then use the is_device flag in amendOperation to filter and generate the flags you wish. This would mean the attribute is present for host and device whenever -fopenmp is specified however, although I don't necessarily see a problem with that!

domada updated this revision to Diff 521255.May 11 2023, 4:39 AM

Resolve dependency between parent and child revision.

Will the translation to LLVM be through amendOperation?

That's my plan. If you wish I can wait till the output of @skatrak proposal: https://reviews.llvm.org/D150275

No need to wait. I believe that proposal was only to demonstrate that there is a way for out of tree users to use the Offload Attributes in the Dialect.
Also, if it uses amendOperation then you can consider removing the getVersion since we will not be using it.

Another question I have is whether the flags attribute will always be present with -fopenmp or only with some device/target specific options.

Currently the flags were only present on device (and the original intent was it only being these flags, but it makes sense for it to extend broader to cover these flags as well), but that was because the flags that were in the attribute originally are only ever present on the device/target. However, all you would need to do to change this is move the is_device check that applies them in the frontend (initial attribute creation) to the back end (MLIR attribute -> LLVM IR) and then use the is_device flag in amendOperation to filter and generate the flags you wish. This would mean the attribute is present for host and device whenever -fopenmp is specified however, although I don't necessarily see a problem with that!

Clang specifies two metadata (for host and the device) for OpenMP version:

!llvm.module.flags = !{!0, !1}
!0 = !{i32 7, !"openmp", i32 51}
!1 = !{i32 7, !"openmp-device", i32 51}

That's why I decided to reflect it in MLIR and add two fields in MLIR code (one for host - and the second for the device).

Could you clarify in the commit message that there are two attributes here: One representing the device version (in Flags Attr) and a standalone one called omp.version?

mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
86–108

Since this is not related to Device or Offloading, I think the omp.version set or get should be removed from this interface.

mlir/test/Dialect/OpenMP/attr.mlir
32

Can we have a roundtrip test for omp.version?

domada updated this revision to Diff 522152.May 15 2023, 6:14 AM

Applied remarks

domada edited the summary of this revision. (Show Details)May 15 2023, 6:16 AM

Could you clarify in the commit message that there are two attributes here: One representing the device version (in Flags Attr) and a standalone one called omp.version?

Please look at the modified description of this patch. Is it ok for you?

domada marked 2 inline comments as done.May 15 2023, 6:19 AM
domada added inline comments.
mlir/test/Dialect/OpenMP/attr.mlir
32

I added test case:
// CHECK: module attributes {omp.version = #omp.version<version = 51>} {
module attributes {omp.version = #omp.version<version = 51>} {}

LG.

Where will the omp.version be set in Flang? Assuming it is in a separate patch.

flang/include/flang/Tools/CrossToolHelpers.h
64–65
This revision is now accepted and ready to land.May 15 2023, 8:02 AM
domada marked an inline comment as done.May 15 2023, 8:27 AM

LG.

Where will the omp.version be set in Flang? Assuming it is in a separate patch.

Please look at: https://reviews.llvm.org/D150354 . Andrzej made remark about default version of OpenMP. If you wish you can express your opinion about it.