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

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

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



================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:826-827
+              Optional<AnyInteger>:$device,
+              Variadic<AnyType>:$use_device_ptr,
+              Variadic<AnyType>:$use_device_addr,
+              VariadicOfVariadic<AnyType, "map_operand_segments">:$map_operands,
----------------
This is OK for now, but we might have to switch to `OpenMPPointerType` later.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:829
+              VariadicOfVariadic<AnyType, "map_operand_segments">:$map_operands,
+              DenseI32ArrayAttr:$map_operand_segments);
+
----------------
Is `map_operand_segments` currently unused?


================
Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
   MLIRLLVMDialect
+  MLIRArithDialect
   )
----------------
TIFitis wrote:
> 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.
Can this be avoided by modelling constants as attributes?


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:577-583
+    if (auto constOp =
+            mlir::dyn_cast<mlir::arith::ConstantOp>(a.front().getDefiningOp()))
+      mapTypeBits = constOp.getValue()
+                        .cast<mlir::IntegerAttr>()
+                        .getValue()
+                        .getSExtValue();
+    else if (auto constOp = mlir::dyn_cast<mlir::LLVM::ConstantOp>(
----------------
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.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:595
+
+    std::stringstream typeMod, type;
+    if (always)
----------------
I think llvm prefers using `llvm::raw_string_ostream`. I have seen only two uses of std::stringstream in MLIR code, possibly accidentally introduced.


================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:389
+    // CHECK: omp.target_data
+    "omp.target_data"(%if_cond, %device, %data1, %data2) ({
+       
----------------
TIFitis wrote:
> 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.
Can you increase the coverage so that each clause is present atleast once, probably by adding more than one test for each operation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915