This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.
ClosedPublic

Authored by TIFitis on Aug 15 2022, 12:39 PM.

Details

Summary

This includes a basic implementation for the OpenMP 5.1 Target Data, Target Exit Data and Target Enter Data constructs
operation.

TODO:

  • Depend clause support for Target Enter and Exit Data.
  • Mapper and Iterator value support for Map Type Modifiers.
  • Verifier for the operations.

Co-authored-by: abidmalikwaterloo <amalik@bnl.gov>
Co-authored-by: raghavendra <Raghu.Maddhipatla@amd.com>

This patch will replace the https://reviews.llvm.org/D105255/new/ (D105255)
It is the latest update of D105255
One can see the comments in the previous patch.
Every issue/comment has been taken care of from D105255

Diff Detail

Event Timeline

abidmalikwaterloo requested review of this revision.Aug 15 2022, 12:39 PM
abidmalikwaterloo edited the summary of this revision. (Show Details)
abidmalikwaterloo added a reviewer: clementval.
This revision is now accepted and ready to land.Aug 17 2022, 10:37 PM

In D105255 we discussed about adding assembly format directly. I don't see it here.

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

Any reason to add comment in the description that will be comment anyway? Same below.

abidmalikwaterloo marked an inline comment as done.Aug 18 2022, 6:34 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
832

I was asked by a reviewer in D105255 to clarify the values of the modifier through comments.

clementval added inline comments.Aug 18 2022, 7:24 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
832

What I meant is that the description block is already comments so you don't need to add ///.

abidmalikwaterloo marked 2 inline comments as done.Aug 18 2022, 8:11 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
832

Ah! :) Got it. I will remove it.

raghavendhra added inline comments.Aug 18 2022, 4:58 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
865

Are you planning to add verifier in future patches?

940

Same as above, Are you planning to add verifier in future patches?

1015

Same as above, Are you planning to add verifier in future patches?

abidmalikwaterloo marked 4 inline comments as done.Aug 19 2022, 12:40 PM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
865

Yes

940

yes

1015

yes

clementval added inline comments.Aug 19 2022, 1:31 PM
mlir/test/Dialect/OpenMP/ops.mlir
389

Can you switch to the custom format form?

394

Same here

397

Same here.

399–400

Not needed for this test.

TIFitis added inline comments.Aug 28 2022, 5:29 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
915–916

Do we need both $nowait_operand and $nowait?

990–991

Same here. Do we need both $nowait_operand and $nowait?

abidmalikwaterloo marked 5 inline comments as done.Sep 12 2022, 6:22 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
915–916

No. Thanks for pointing this out. I overlooked it. It will be corrected.

TIFitis requested changes to this revision.Sep 15 2022, 11:02 AM
TIFitis added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
846

When multiple map clauses are supplied, it is not possible to associate the map_type_modifier to its respective map.

Eg:

!$omp target enter data map(to: a) map(always alloc: b) can be lowered to something like
omp.target_enter_data map_to(%0 : !fir.ref<i32>) map_alloc(%1 : !fir.ref<i32>) map_type_modifier(%c4_i32 : i32)

It is not possible to deduce if the map_type_modifier %c4_i32 is for a or b.

I'm not sure if something like Variadic<Optional<AnyInteger>, AnyType> is supported then we can change all the map_operands' type to that. If not we might need a map_type_modifier for each map_operand type.

This revision now requires changes to proceed.Sep 15 2022, 11:02 AM
TIFitis added inline comments.Sep 15 2022, 11:16 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
917–923

Only map_to and map_alloc are allowed types for EnterData. We can perhaps get rid of the remaining map_operand types.

992–998

Only map_from, map_release and map_delete are allowed types for ExitData. We can perhaps get rid of the remaining map_operand types.

abidmalikwaterloo marked an inline comment as done.Sep 15 2022, 11:30 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
846

One possibility is to make it a vector for each map_type_modifier. The length is equal to map clauses. The index number can be used to associate the corresponding map clause in the construct and '1' will tell whether the map_type_modifier is active for the corresponding clause. But, `Variadic<Optional<AnyInteger>, AnyType> option seems more appropriate if available.

TIFitis added inline comments.Sep 15 2022, 11:44 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
846

I thought of using the index of map_type_modifier but that still doesn't solve the issue.

Eg,
omp target enter data map(to: a) map(always alloc: b) and
omp target enter data map(alloc: b) map(always to: a)

both get lowered to the same mlir

omp.target_enter_data map_to(a) map_alloc(b) map_type_modifier(false, true).

This is because upon lowering we lose information on ordering of map clauses from source.

clementval added inline comments.Sep 15 2022, 12:03 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
846

The value in the map_type_modifer should be a combination of the different map type modifiers for the operand at the same position and not just true false. The runtime and OpenMP IR Builder are using this already.

TIFitis added inline comments.Sep 15 2022, 1:38 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
846

Thanks for clearing that, I was wondering the same.

I think the current implementation is workable, but something like Variadic<Optional<AnyInteger>, AnyType> may be more preferrable.

raghavendhra commandeered this revision.Sep 28 2022, 9:40 AM
raghavendhra edited reviewers, added: abidmalikwaterloo; removed: raghavendhra.

Addressed latest comments

Overlooked some restrictions in OpenMP standard and made changes accordingly.

clementval added inline comments.Sep 30 2022, 2:40 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
835–840

Why not having a single map_operands and encoding all the rest in the map_type_modifier vector? This is what the runtime expect so it would simplify things here. Just the fact that you have to, from and toform would lead into such an encoding.

raghavendhra added inline comments.Sep 30 2022, 9:20 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
835–840

Sure, I can make that change.

Sorry I am unaware of how to implement this vector based map_operand. Can you please point me to any example I can refer here to get similar implementation here.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
835–840

I checked the OpenACC dialect. It does not support copyin and copyout operations yet. They are closest to the map_to, and map_from. I checked my notes. I dropped the idea of vector support because of no clarity of its implementation at that time. Do we have anything in the MLIR dialects now that we can take as a template for the proposed vector implementation? I checked acc dialect but could not figure out anything.

846

I thought of using the index of map_type_modifier but that still doesn't solve the issue.

Eg,
omp target enter data map(to: a) map(always alloc: b) and
omp target enter data map(alloc: b) map(always to: a)

both get lowered to the same mlir

omp.target_enter_data map_to(a) map_alloc(b) map_type_modifier(false, true).

This is because upon lowering we lose information on ordering of map clauses from source.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
835–840

I think what @clementval is suggesting is to just have the following, where the the map_operands and map_type_and_modifier_operands are in one-to-one correspondence.

Variadic<AnyType>:$map_operands,
Variadic<AnyType>:$map_type_and_modifier_operands);

The pretty print form of the map clause can be something like

map (always, to : a)  (none, fromto : b)

Assumption: The default map-type is fromto and there is no default map-type-modifier.

So map_operands will internally be [ a, b] and map_type_and_modifer operands will be [ <always, to>, <none, fromto> ].

We can write a custom printer and parser. The custom parse will parse this info into the $map_operands and the $map_type_and_modifier_operands.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
835–840

Also, thinking about this, the map_type_and_modifier will be an attribute and not an operand since this is a constant value known at compiler time and not a variable value. It is also possible to represent this as an Enum Attribute.

clementval added inline comments.Oct 10 2022, 9:00 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
835–840

Yeah that this more or less what I had in mind. OpenACC doesn't have this because to/from are not modifier but clauses. There are also fewer modifiers so the complexity is reduced and we could represent all the combination easily which is not the case for OpenMP.

Thanks, @kiranchandramohan @raghavendhra @clementval

In case:

map  (none, fromto: b) (always, to: a)

map_operands will internally be [ b, a] and map_type_and_modifer operands will be [ <none, fromto>, <always, to> ].

Thanks, @kiranchandramohan @raghavendhra @clementval

In case:

map  (none, fromto: b) (always, to: a)

map_operands will internally be [ b, a] and map_type_and_modifer operands will be [ <none, fromto>, <always, to> ].

Yes, that sounds right.

Added implementation according to @clementval, @kiranchandramohan and @abidmalikwaterloo's review comments discussion.

Seperated map_operands with map-type and map-type-modifier as enum attributes.

Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptOct 18 2022, 1:31 AM

Ping for review.

TIFitis accepted this revision.Oct 20 2022, 4:31 AM
This revision is now accepted and ready to land.Oct 20 2022, 4:31 AM
TIFitis requested changes to this revision.Oct 25 2022, 7:27 AM
TIFitis added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
830–831

I think these should be Variadic to allow multiple map clauses.

884–885

I think these should be Variadic to allow multiple map clauses.

935–936

I think these should be Variadic to allow multiple map clauses.

This revision now requires changes to proceed.Oct 25 2022, 7:27 AM
clementval added inline comments.Oct 27 2022, 9:29 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
841

It's not clear how to you deal with multiple map clauses here? Or do you plan to have an operation generation per map clause coming from the frontend?

892

Same comment here about multiple map clauses. You probably need a custom parser/printer for the map part together with the change requested by @TIFitis.

943

Same comment here.

TIFitis updated this revision to Diff 480029.Dec 5 2022, 2:48 AM

Changed map_operands to VariadicOfVariadic type.

Variadic Enums are not supported, hence I've changed the map type to VariadicOfVariadic which allows to store the entire map clause including types and modifiers in a single operand.

TIFitis commandeered this revision.Dec 5 2022, 2:50 AM
TIFitis edited reviewers, added: raghavendhra; removed: TIFitis.
TIFitis marked an inline comment as done.Dec 5 2022, 2:55 AM

If people are happy with this structure then I'll work on adding a pretty printer for the map clause. Please suggest any other changes that are required.

TIFitis updated this revision to Diff 482732.Dec 14 2022, 12:32 AM

Added custom printer & parser for map clause. Updated ops.mlir test to address reviewer comments.

Custom Printer example:

OMP: !$omp target exit data map(from: a,b) map(release: c) map(delete: d) map(always, from: e)
CustomPrinter: omp.target_exit_data map((%c2_i64 -> none , from : %0 : !fir.ref<!fir.array<1024xi32>>), (%c2_i64 -> none , from : %1 : !fir.ref<!fir.array<1024xi32>>), (%c0_i64 -> none , release : %2 : !fir.ref<!fir.array<1024xi32>>), (%c8_i64 -> none , delete : %3 : !fir.ref<!fir.array<1024xi32>>), (%c6_i64 -> always , from : %4 : !fir.ref<!fir.array<1024xi32>>))

Requesting comments and suggestions :)

TIFitis marked 10 inline comments as done.Dec 14 2022, 12:51 AM
TIFitis added inline comments.
mlir/test/Dialect/OpenMP/ops.mlir
389

Hi, I've updated the test file, Is this what you wanted?

kiranchandramohan requested changes to this revision.Dec 16 2022, 5:08 AM

Thanks for the changes and your work here.

Added custom printer & parser for map clause. Updated ops.mlir test to address reviewer comments.

Custom Printer example:

OMP: !$omp target exit data map(from: a,b) map(release: c) map(delete: d) map(always, from: e)
CustomPrinter: omp.target_exit_data map((%c2_i64 -> none , from : %0 : !fir.ref<!fir.array<1024xi32>>), (%c2_i64 -> none , from : %1 : !fir.ref<!fir.array<1024xi32>>), (%c0_i64 -> none , release : %2 : !fir.ref<!fir.array<1024xi32>>), (%c8_i64 -> none , delete : %3 : !fir.ref<!fir.array<1024xi32>>), (%c6_i64 -> always , from : %4 : !fir.ref<!fir.array<1024xi32>>))

Can you add this as a test? AFAIS, the tests attached to this patch do not seem to be exercising the VariadicofVariadic requirement. An explanation with an example would be great.

  1. Fortran Source + OpenMP example that needs a VariadicOfVariadic
  2. MLIR OpenMP representation

I am assuming we would need a verifier as well for the map clause.

Can you also restrict this patch to one of the constructs say target data for this patch? Once we decide on that then the other two can be easily added in a separate patch.

mlir/lib/Dialect/OpenMP/CMakeLists.txt
15 ↗(On Diff #482732)

Why is this needed here?

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
572 ↗(On Diff #482732)

Nit: can you specify the type here?

584 ↗(On Diff #482732)

There is a bitn function in printSynchronizationHint and verifySynchronizationHint, can that be used here?

mlir/test/Dialect/OpenMP/ops.mlir
389

Both the input and the CHECK lines in the custom format.

This revision now requires changes to proceed.Dec 16 2022, 5:08 AM
TIFitis updated this revision to Diff 483545.Dec 16 2022, 7:57 AM
TIFitis marked 2 inline comments as done.

Addresed reviewer comments.
Added second map clause to test.

TIFitis marked 3 inline comments as done.Dec 16 2022, 8:25 AM

Can you add this as a test? AFAIS, the tests attached to this patch do not seem to be exercising the VariadicofVariadic requirement. An explanation with an example would be great.

VariadicOfVariadic gives us a SmallVector<ValueRange> where ValueRange is essentially a SmallVector<Value>. As it is possible to have multiple map clauses for a single target construct, this allows us to represent each map clause as a row in $map_operands.

I've updated the ops.mlir test to use two map clauses which better shows this use case.

  1. Fortran Source + OpenMP example that needs a VariadicOfVariadic
  2. MLIR OpenMP representation

Fortran Source:

subroutine omp_target_enter
   integer :: a(1024)
   integer :: b(1024)
   !$omp target enter data map(to: a,) map(always, alloc: b)
end subroutine omp_target_enter

MLIR OpenMP representation:

func.func @_QPomp_target_enter() {
  %0 = fir.alloca !fir.array<1024xi32> {bindc_name = "a", uniq_name = "_QFomp_target_enterEa"}
  %1 = fir.alloca !fir.array<1024xi32> {bindc_name = "b", uniq_name = "_QFomp_target_enterEb"}
  %c1_i64 = arith.constant 1 : i64
  %c4_i64 = arith.constant 4 : i64
  omp.target_enter_data   map((%c1_i64 -> none , to : %0 : !fir.ref<!fir.array<1024xi32>>), (%c4_i64 -> always , alloc : %1 : !fir.ref<!fir.array<1024xi32>>))
  return
}

I plan on adding these as tests along with Fortran lowering support in upcoming patches.

I am assuming we would need a verifier as well for the map clause.

AFAIK the map clause rules are op specific. I plan on adding verifiers for the ops soon in a separate patch.

Can you also restrict this patch to one of the constructs say target data for this patch? Once we decide on that then the other two can be easily added in a separate patch.

Since I didn't make any changes to the ops I've left them in. If the patch requires further changes, I'll leave them out.

mlir/lib/Dialect/OpenMP/CMakeLists.txt
15 ↗(On Diff #482732)

The Arith Dialect needs to be linked against as we're using it extract the int value from arith.constant in the custom printer.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
584 ↗(On Diff #482732)

I've added something similar. I got the bit values from Clang codegen and they use hex so I've kept it that way for uniformity. Let me know if you'd rather it be in n'th bit format.

mlir/test/Dialect/OpenMP/ops.mlir
389

I've changed both to custom format. Added a second map clause argument to show support.

jdoerfert added inline comments.Dec 16 2022, 12:05 PM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
584 ↗(On Diff #482732)

Clang codegen magic constants (and everything else) is moved to llvm/.../Frontend/OpenMP for a reason. Hardcoding the numbers again somewhere else seems like exactly the wrong thing to do.

clementval added inline comments.Dec 16 2022, 12:18 PM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
584 ↗(On Diff #482732)

+1

kiranchandramohan requested changes to this revision.Dec 16 2022, 3:00 PM

Can you add the other authors as Co-authors in the patch Summary?

Can you add this as a test? AFAIS, the tests attached to this patch do not seem to be exercising the VariadicofVariadic requirement. An explanation with an example would be great.

VariadicOfVariadic gives us a SmallVector<ValueRange> where ValueRange is essentially a SmallVector<Value>. As it is possible to have multiple map clauses for a single target construct, this allows us to represent each map clause as a row in $map_operands.

I've updated the ops.mlir test to use two map clauses which better shows this use case.

  1. Fortran Source + OpenMP example that needs a VariadicOfVariadic
  2. MLIR OpenMP representation

Fortran Source:

subroutine omp_target_enter
   integer :: a(1024)
   integer :: b(1024)
   !$omp target enter data map(to: a,) map(always, alloc: b)
end subroutine omp_target_enter

MLIR OpenMP representation:

func.func @_QPomp_target_enter() {
  %0 = fir.alloca !fir.array<1024xi32> {bindc_name = "a", uniq_name = "_QFomp_target_enterEa"}
  %1 = fir.alloca !fir.array<1024xi32> {bindc_name = "b", uniq_name = "_QFomp_target_enterEb"}
  %c1_i64 = arith.constant 1 : i64
  %c4_i64 = arith.constant 4 : i64
  omp.target_enter_data   map((%c1_i64 -> none , to : %0 : !fir.ref<!fir.array<1024xi32>>), (%c4_i64 -> always , alloc : %1 : !fir.ref<!fir.array<1024xi32>>))
  return
}

Thanks, that helps.

I plan on adding these as tests along with Fortran lowering support in upcoming patches.

I am assuming we would need a verifier as well for the map clause.

AFAIK the map clause rules are op specific. I plan on adding verifiers for the ops soon in a separate patch.

Can you also restrict this patch to one of the constructs say target data for this patch? Once we decide on that then the other two can be easily added in a separate patch.

Since I didn't make any changes to the ops I've left them in. If the patch requires further changes, I'll leave them out.

The concern is with the introduction of VariadicOfVariadic with AnyType. I think this is new in the OpenMP Dialect and it pretty much allows anything. So, I was thinking whether it would be a good idea to write a verifier for the map clause, if not for the entire construct.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
824–825

This is OK for now, but we might have to switch to OpenMPPointerType later.

827

Is map_operand_segments currently unused?

mlir/lib/Dialect/OpenMP/CMakeLists.txt
15 ↗(On Diff #482732)

Can this be avoided by modelling constants as attributes?

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
577–583 ↗(On Diff #483545)

Generally, constant values are modelled as attributes in MLIR representation. Can we switch to that representation? This will also avoid the need for this if-else and dependence on the arith dialect.

595 ↗(On Diff #483545)

I think llvm prefers using llvm::raw_string_ostream. I have seen only two uses of std::stringstream in MLIR code, possibly accidentally introduced.

mlir/test/Dialect/OpenMP/ops.mlir
389

Can you increase the coverage so that each clause is present atleast once, probably by adding more than one test for each operation.

This revision now requires changes to proceed.Dec 16 2022, 3:00 PM
TIFitis updated this revision to Diff 489345.Jan 15 2023, 4:23 AM
TIFitis marked 7 inline comments as done.

Added verifiers, removed use of magic constants and addressed other reviewer comments.

TIFitis edited the summary of this revision. (Show Details)Jan 15 2023, 4:27 AM
TIFitis marked 4 inline comments as done.
TIFitis added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
827

map_operand_segments is required by VariadicOfVariadic type to keep track of the dimensions. I haven't made explicit use of it, however, it is used internally and cant be avoided AFAIK.

It is akin to the operand_segment_sizes which is implicitly present for Operations with Optional or Variadic operands.

mlir/lib/Dialect/OpenMP/CMakeLists.txt
15 ↗(On Diff #482732)

The issue with attributes is AFAIK Variadic<Attribute> is not supported, and as previously discussed we need it be Variadic to support multiple map clauses.

If I am wrong and there is indeed a way to have Variadic<Attribute> then this can be avoided.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
577–583 ↗(On Diff #483545)

Copy:
The issue with attributes is AFAIK Variadic<Attribute> is not supported, and as previously discussed we need it be Variadic to support multiple map clauses.

If I am wrong and there is indeed a way to have Variadic<Attribute> then this can be avoided.

TIFitis updated this revision to Diff 489510.Jan 16 2023, 5:09 AM

Removed use of VariadicOfVariadic. Added new I64ArrayAttr for map_types.

TIFitis updated this revision to Diff 489515.Jan 16 2023, 5:17 AM

Minor indentation changes.

kiranchandramohan accepted this revision.EditedJan 16 2023, 9:17 AM

LGTM. Thanks for making the changes and for your patience. Please wait a couple of days to give other reviewers a chance to have a look before submitting.

Could you update the Summary of this patch to specify what is in this patch and what is left out? Also, might be useful to specify any special modelling, like for the map clause.

Could you specify the co-authors in the following way?
Co-authored-by: abidmalikwaterloo <amalik@bnl.gov>
Co-authored-by: raghavendra <Raghu.Maddhipatla@amd.com>

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

Is this number from the OpenMP 5.0 standard? I think 5.0 does not have the present map-type modifier. The printer includes this. I think we can either remove things that are not there in 5.0 or add comments when items from newer versions are included or alternatively change the number to the latest version and call out everything that is not implemented.

815–817

Update these two to their current meanings. (Also replace map_operand_segments with map_types.

Same comment for the other two operations.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
542 ↗(On Diff #489515)

Could you specify the EBNF (https://mlir.llvm.org/docs/LangRef/#notation) for the expected structure of the map clause?

595 ↗(On Diff #489515)

Nit: Should this be an assert?

This revision is now accepted and ready to land.Jan 16 2023, 9:17 AM
TIFitis updated this revision to Diff 489881.Jan 17 2023, 10:40 AM

Addressed reviewer comments.
Updated printer and parser to remove the IntegerAttr from appearing. Removed none from map type modifiers, absence of other modifiers implicitly means none. This helps keep it in closer to the specification.

TIFitis edited the summary of this revision. (Show Details)Jan 17 2023, 10:41 AM
TIFitis edited the summary of this revision. (Show Details)Jan 17 2023, 10:58 AM
TIFitis marked 5 inline comments as done.Jan 17 2023, 11:03 AM

LGTM. Thanks for making the changes and for your patience. Please wait a couple of days to give other reviewers a chance to have a look before submitting.

Thanks for reviewing the changes as well :)

Could you update the Summary of this patch to specify what is in this patch and what is left out? Also, might be useful to specify any special modelling, like for the map clause.

I've updated the summary. The new map clause looks straight forward to me, anything particular you want me to mention in the summary?

Could you specify the co-authors in the following way?
Co-authored-by: abidmalikwaterloo <amalik@bnl.gov>
Co-authored-by: raghavendra <Raghu.Maddhipatla@amd.com>

Done.

Cheers,
Akash

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

Hi I've updated it to OpenMP 5.1 standard specification. We are missing the depend clause, and mapper and iterator values for the map_type_modifier which are already specified in the TODO.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
542 ↗(On Diff #489515)

Now that we are using IntegerAttr we no longer need the Attr to be explicitly present when printing it.

I have thus updated the printer and parser accordingly, also removed none as a map_type_modifier as absence of other modifiers implicitly implies it and prevents confusion by diverging from the specification.

Here's an example of the new custom printer:

omp.target_exit_data   map((release -> %1 : !llvm.ptr<array<1024 x i32>>), (always, close, from -> %2 : !llvm.ptr<array<1024 x i32>>))

Let me know if the EBNF is okay. Should I override clang-format and keep the grammar and Eg in the same line to make them easier to read?

TIFitis marked 2 inline comments as done.Jan 20 2023, 4:01 AM

I'm planning on merging the patch early next week. If you'd like to see any changes or need more time to review, please let me know :)

Akash

Just small nit

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
628–629 ↗(On Diff #489881)

auto should be spelled out here.