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

[Akash Banerjee via Phabricator via llvm-commits]

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

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



================
Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
   MLIRLLVMDialect
+  MLIRArithDialect
   )
----------------
kiranchandramohan wrote:
> Why is this needed here?
The Arith Dialect needs to be linked against as we're using it extract the int value from arith.constant in the custom printer.


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


================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:389
+    // CHECK: omp.target_data
+    "omp.target_data"(%if_cond, %device, %data1, %data2) ({
+       
----------------
kiranchandramohan wrote:
> 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.
I've changed both to custom format. Added a second map clause argument to show support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915