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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

Thanks for the changes and your work here.

In D131915#3994160 <https://reviews.llvm.org/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.



================
Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
   MLIRLLVMDialect
+  MLIRArithDialect
   )
----------------
Why is this needed here?


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:572
+  for (const auto &a : map_operands) {
+    auto mapTypeBits = 0x00;
+    if (auto constOp =
----------------
Nit: can you specify the type here?


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:584
+    std::stringstream typeMod, type;
+    if (mapTypeBits & 0x04)
+      typeMod << "always ";
----------------
There is a `bitn` function in `printSynchronizationHint` and `verifySynchronizationHint`, can that be used here?


================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:389
+    // CHECK: omp.target_data
+    "omp.target_data"(%if_cond, %device, %data1, %data2) ({
+       
----------------
TIFitis wrote:
> clementval wrote:
> > Can you switch to the custom format form?
> Hi, I've updated the test file, Is this what you wanted?
Both the input and the CHECK lines in the custom format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915