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

[TIFitis]

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 <[e-mail]>
Co-authored-by: raghavendra <[e-mail]>

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

[raghavendhra]

LGTM

[clementval]

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]

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]

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]

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

832

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

[raghavendhra]

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]

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

865

Yes

940

yes

1015

yes

[clementval]

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]

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]

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

915–916

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

[TIFitis]

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.

[TIFitis]

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]

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]

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]

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]

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]

Addressed latest comments

[raghavendhra]

Overlooked some restrictions in OpenMP standard and made changes accordingly.

[clementval]

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]

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.

[abidmalikwaterloo]

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.

[kiranchandramohan]

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.

[kiranchandramohan]

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]

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.

[abidmalikwaterloo]

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

[kiranchandramohan]

In D131915#3849417, @abidmalikwaterloo wrote:

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.

[raghavendhra]

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

[raghavendhra]

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

[raghavendhra]

Ping for review.

[TIFitis]

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.

[clementval]

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]

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]

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]

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]

mlir/test/Dialect/OpenMP/ops.mlir

389

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

[kiranchandramohan]

Thanks for the changes and your work here.

In D131915#3994160, @TIFitis wrote:

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.

[TIFitis]

Addresed reviewer comments.
Added second map clause to test.

[TIFitis]

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]

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]

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

584 ↗

(On Diff #482732)

+1

[kiranchandramohan]

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

In D131915#4001620, @TIFitis wrote:

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.

[TIFitis]

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

[TIFitis]

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.

[kiranchandramohan]

mlir/lib/Dialect/OpenMP/CMakeLists.txt

15 ↗

(On Diff #482732)

Can we use an ArrayAttr (https://mlir.llvm.org/docs/Dialects/Builtin/#arrayattr) or something derived from it?
https://github.com/llvm/llvm-project/blob/6ee5a1a090f3f4b6ae7ec9915900023277daf8d7/mlir/include/mlir/IR/OpBase.td#L1467

[TIFitis]

Removed use of VariadicOfVariadic. Added new I64ArrayAttr for map_types.

[TIFitis]

Minor indentation changes.

[kiranchandramohan]

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 <[e-mail]>
Co-authored-by: raghavendra <[e-mail]>

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?

[TIFitis]

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]

In D131915#4056730, @kiranchandramohan wrote:

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 <[e-mail]>
Co-authored-by: raghavendra <[e-mail]>

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]

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

[clementval]

Just small nit

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

628–629 ↗

(On Diff #489881)

auto should be spelled out here.

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

def TaskyieldOp : OpenMP_Op<"taskyield"> {
   let description = [{
     The taskyield construct specifies that the current task can be suspended
     in favor of execution of a different task.
   }];
 
   let assemblyFormat = "attr-dict";
 }
 
+//===---------------------------------------------------------------------===//
+// 2.12.2 target data Construct 
+//===---------------------------------------------------------------------===//
+
+def Target_Data: OpenMP_Op<"target_data", [AttrSizedOperandSegments]>{
+  let summary = "target data construct";
+  let description = [{
+    Map variables to a device data environment for the extent of the region.
+
+    The omp target data directive maps variables to a device data 
+    environment, and defines the lexical scope of the data environment 
+    that is created. The omp target data directive can reduce data copies 
+    to and from the offloading device when multiple target regions are using 
+    the same data.
+
+    The optional $if_expr parameter specifies a boolean result of a
+    conditional check. If this value is 1 or is not provided then the target
+    region runs on a device, if it is 0 then the target region is executed 
+    on the host device.
+
+    The optional $device parameter specifies the device number for the target 
+    region.
+
+    The optional $use_device_ptr specifies the device pointers to the 
+    corresponding list items in the device data environment
+  
+    The optional $use_device_addr specifies the adress of the objects in the 
+    device data enviornment
+
+    The $map_operands specifies operands with no map type
+
+    The $map_to_operands specifies operands with  map type "to"
+
+    The $map_from_operands specifies operands with map type "from"
+    
+    The $map_to_from_operands specifies operands with map type "tofrom"
+
+    The $map_alloc_operands specifies operands with map type "alloc"
+
+    The $map_release_operands specifies operands with map type "release"
+
+    The $map_delete_operands specifies operands with map type "delete"
+
+    The $map_type_modifier vector specifies the modifier for each map type 
+    operand
+    
+    /// map_type_modifier can be always, close, and mapper
+  }];
+
+  let arguments = (ins Optional<I1>:$if_expr,
+             Optional<AnyInteger>:$device,
+             Variadic<AnyType>:$use_device_ptr,
+             Variadic<AnyType>:$use_device_addr,
+             Variadic<AnyType>:$map_operands,
+             Variadic<AnyType>:$map_to_operands,
+             Variadic<AnyType>:$map_from_operands,
+             Variadic<AnyType>:$map_to_from_operands,
+             Variadic<AnyType>:$map_alloc_operands,
+             Variadic<AnyType>:$map_release_operands,
+             Variadic<AnyType>:$map_delete_operands,
+             Variadic<AnyInteger>:$map_type_modifier_operands);
+       
+  let regions = (region AnyRegion:$region); 
+      
+  let assemblyFormat = [{
+    (`if` `(` $if_expr^ `)` )?
+    (`device` `(` $device^ `:` type($device) `)` )?
+    (`use_device_ptr` `(` $use_device_ptr^ `:` type($use_device_ptr) `)` )?
+    (`use_device_addr` `(` $use_device_addr^ `:` type($use_device_addr) `)` )?
+    (`map` `(` $map_operands^ `:` type($map_operands) `)` )?
+    (`map_to` `(` $map_to_operands^ `:` type($map_to_operands) `)` )?
+    (`map_from` `(` $map_from_operands^ `:` type($map_from_operands) `)` )?
+    (`map_to_from` `(` $map_to_from_operands^ `:` type($map_to_from_operands) `)` )?
+    (`map_alloc` `(` $map_alloc_operands^ `:` type($map_alloc_operands) `)` )?
+    (`map_release` `(` $map_release_operands^ `:` type($map_release_operands) `)` )?
+    (`map_delete` `(` $map_delete_operands^ `:` type($map_delete_operands) `)` )?
+    (`map_type_modifier` `(` $map_type_modifier_operands^ `:` type($map_type_modifier_operands) `)` )?
+    $region attr-dict
+  }];          
+  
+}
+
+//===---------------------------------------------------------------------===//
+// 2.12.3 target enter data Construct
+//===---------------------------------------------------------------------===//
+
+def Target_EnterDataOp: OpenMP_Op<"target_enter_data", 
+                                                 [AttrSizedOperandSegments]>{
+  let  summary = "target enter data construct";
+  let description = [{
+    The target enter data directive specifies that variables are mapped to 
+    a device data environment. The target enter data directive is a 
+    stand-alone directive.
+  
+    The optional $if_expr parameter specifies a boolean result of a
+    conditional check. If this value is 1 or is not provided then the target
+    region runs on a device, if it is 0 then the target region is executed on 
+    the host device.
+
+    The optional $device parameter specifies the device number for the 
+    target region.
+
+    The optional $nowait elliminates the implicit barrier so the parent task 
+    can make progress even if the target task is not yet completed.
+
+    The $map_operands specifies operands with no map type
+
+    The $map_to_operands specifies operands with  map type "to"
+
+    The $map_from_operands specifies operands with map type "from"
+    
+    The $map_to_from_operands specifies operands with map type "tofrom"
+
+    The $map_alloc_operands specifies operands with map type "alloc"
+
+    The $map_release_operands specifies operands with map type "release"
+
+    The $map_delete_operands specifies operands with map type "delete"
+
+    The $map_type_modifier vector specifies the modifier for each map type 
+    operand
+    
+    /// map_type_modifier can be always, close, and mapper
+    
+    TODO:  depend
+  }];
+
+  let arguments = (ins Optional<I1>:$if_expr,
+             Optional<AnyInteger>:$device,
+             Optional<I1>:$nowait_operand,
+             UnitAttr:$nowait,
+             Variadic<AnyType>:$map_operands,
+             Variadic<AnyType>:$map_to_operands,
+             Variadic<AnyType>:$map_from_operands,
+             Variadic<AnyType>:$map_to_from_operands,
+             Variadic<AnyType>:$map_alloc_operands,
+             Variadic<AnyType>:$map_release_operands,
+             Variadic<AnyType>:$map_delete_operands,
+             Variadic<AnyInteger>:$map_type_modifier_operands);
+                        
+  let assemblyFormat = [{
+    (`if` `(` $if_expr^ `)` )?
+    (`device` `(` $device^ `:` type($device) `)` )?
+    (`nowait` `(` $nowait_operand^ `:` type($nowait_operand) `)` )?
+    (`map` `(` $map_operands^ `:` type($map_operands) `)` )?
+    (`map_to` `(` $map_to_operands^ `:` type($map_to_operands) `)` )?
+    (`map_from` `(` $map_from_operands^ `:` type($map_from_operands) `)` )?
+    (`map_to_from` `(` $map_to_from_operands^ `:` type($map_to_from_operands) `)` )?
+    (`map_alloc` `(` $map_alloc_operands^ `:` type($map_alloc_operands) `)` )?
+    (`map_release` `(` $map_release_operands^ `:` type($map_release_operands) `)` )?
+    (`map_delete` `(` $map_delete_operands^ `:` type($map_delete_operands) `)` )?
+    (`map_type_modifier` `(` $map_type_modifier_operands^ `:` type($map_type_modifier_operands) `)` )?
+    attr-dict
+  }];    
+             
+}
+
+//===---------------------------------------------------------------------===//
+// 2.12.4 target exit data Construct
+//===---------------------------------------------------------------------===//
+
+def Target_ExitDataOp: OpenMP_Op<"target_exit_data",
+                                                 [AttrSizedOperandSegments]>{
+  let  summary = "target exit data construct";
+  let description = [{
+    The target exit data directive specifies that variables are mapped to a 
+    device data environment. The target exit data directive is 
+    a stand-alone directive.
+
+    The optional $if_expr parameter specifies a boolean result of a
+    conditional check. If this value is 1 or is not provided then the target
+    region runs on a device, if it is 0 then the target region is executed 
+    on the host device.
+
+    The optional $device parameter specifies the device number for the 
+    target region.
+
+    The optional $nowait elliminates the implicit barrier so the parent 
+    task can make progress even if the target task is not yet completed.
+  
+    The $map_operands specifies operands with no map type
+
+    The $map_to_operands specifies operands with  map type "to"
+
+    The $map_from_operands specifies operands with map type "from"
+    
+    The $map_to_from_operands specifies operands with map type "tofrom"
+
+    The $map_alloc_operands specifies operands with map type "alloc"
+
+    The $map_release_operands specifies operands with map type "release"
+
+    The $map_delete_operands specifies operands with map type "delete"
+
+    The $map_type_modifier vector specifies the modifier for each map type 
+    operand
+    
+    /// map_type_modifier can be always, close, and mapper
+
+    TODO: depend
+  }];
+
+  let arguments = (ins Optional<I1>:$if_expr,
+             Optional<AnyInteger>:$device,
+             Optional<I1>:$nowait_operand,
+             UnitAttr:$nowait,
+             Variadic<AnyType>:$map_operands,
+             Variadic<AnyType>:$map_to_operands,
+             Variadic<AnyType>:$map_from_operands,
+             Variadic<AnyType>:$map_to_from_operands,
+             Variadic<AnyType>:$map_alloc_operands,
+             Variadic<AnyType>:$map_release_operands,
+             Variadic<AnyType>:$map_delete_operands,
+             Variadic<AnyInteger>:$map_type_modifier_operands);
+             
+  let assemblyFormat = [{
+    (`if` `(` $if_expr^ `)` )?
+    (`device` `(` $device^ `:` type($device) `)` )?
+    (`nowait` `(` $nowait_operand^ `:` type($nowait_operand) `)` )?
+    (`map` `(` $map_operands^ `:` type($map_operands) `)` )?
+    (`map_to` `(` $map_to_operands^ `:` type($map_to_operands) `)` )?
+    (`map_from` `(` $map_from_operands^ `:` type($map_from_operands) `)` )?
+    (`map_to_from` `(` $map_to_from_operands^ `:` type($map_to_from_operands) `)` )?
+    (`map_alloc` `(` $map_alloc_operands^ `:` type($map_alloc_operands) `)` )?
+    (`map_release` `(` $map_release_operands^ `:` type($map_release_operands) `)` )?
+    (`map_delete` `(` $map_delete_operands^ `:` type($map_delete_operands) `)` )?
+    (`map_type_modifier` `(` $map_type_modifier_operands^ `:` type($map_type_modifier_operands) `)` )?
+    attr-dict
+  }];              
+                                     
+}
+
 //===----------------------------------------------------------------------===//
 // 2.13.7 flush Construct
 //===----------------------------------------------------------------------===//
 def FlushOp : OpenMP_Op<"flush"> {
   let summary = "flush construct";
   let description = [{
     The flush construct executes the OpenMP flush operation. This operation
     makes a thread’s temporary view of memory consistent with memory and

mlir/test/Dialect/OpenMP/ops.mlir

func.func @omp_target(%if_cond : i1, %device : si32,  %num_threads : i32) -> () {
     }) {nowait, operand_segment_sizes = dense<[1,1,1]>: vector<3xi32>} : ( i1, si32, i32 ) -> ()
 
     // CHECK: omp.barrier
     omp.barrier
 
     return
 }
 
+// CHECK-LABEL: omp_target_data
+func.func @omp_target_data (%if_cond : i1, %device : si32,  %data1: memref<?xi32>, %data2: memref<?xi32>) -> () {
+
+    // CHECK: omp.target_data
+    "omp.target_data"(%if_cond, %device, %data1, %data2) ({
+       
+    }) {operand_segment_sizes = dense<[1,1,1,1,0,0,0,0,0,0,0,0]>: vector<12xi32> } : ( i1, si32, memref<?xi32>, memref<?xi32> ) -> ()
+
+    // CHECK: omp.target_enter_data 
+    "omp.target_enter_data"(%if_cond, %device)  {operand_segment_sizes = dense<[1,1,0,0,0,0,0,0,0,0,0]>: vector<11xi32>, nowait } : ( i1, si32 ) -> ()
+   
+    // CHECK: omp.target_exit_data
+    "omp.target_exit_data"(%if_cond, %device) {operand_segment_sizes = dense<[1,1,0,0,0,0,0,0,0,0,0]>: vector<11xi32>, nowait } : ( i1, si32 ) -> ()
+   
+    // CHECK: omp.barrier
+    omp.barrier
+
+   return
+}
+
+
+
 // CHECK-LABEL: omp_target_pretty
 func.func @omp_target_pretty(%if_cond : i1, %device : si32,  %num_threads : i32) -> () {
     // CHECK: omp.target if({{.*}}) device({{.*}})
     omp.target if(%if_cond) device(%device : si32) {
       omp.terminator
     }
 
     // CHECK: omp.target if({{.*}}) device({{.*}}) nowait