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

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked 5 inline comments as done.
TIFitis added a comment.

In D131915#4056730 <https://reviews.llvm.org/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 <amalik at bnl.gov>
> Co-authored-by: raghavendra <Raghu.Maddhipatla at amd.com>

Done.

Cheers,
Akash



================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:790
+//===---------------------------------------------------------------------===//
+// 2.12.2 target data Construct
+//===---------------------------------------------------------------------===//
----------------
kiranchandramohan wrote:
> 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.
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.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:542
+// Parser, printer and verifier for Target Data
+//===----------------------------------------------------------------------===//
+static ParseResult
----------------
kiranchandramohan wrote:
> Could you specify the EBNF (https://mlir.llvm.org/docs/LangRef/#notation) for the expected structure of the map clause?
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131915/new/

https://reviews.llvm.org/D131915