[PATCH] D146648: [MLIR][OpenMP] Added MLIR translation support for use_device clauses

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

While looking into this patch, I noticed that the current implementation in Semantics and MLIR requires the MAP clause to always be present. But the standard only says the following,

  At least one map, use_device_addr or use_device_ptr clause must appear on the directive.

I will fix this. But this made having a look at the code generated only with `use_device_addr`, `use_device_ptr` and without `map` difficult.



================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1558
   auto bodyGenCB = [&](InsertPointTy codeGenIP, BodyGenTy bodyGenType) {
+    auto &region = cast<omp::DataOp>(op).getRegion();
     switch (bodyGenType) {
----------------
Nit: expand the type here. Put an assert before this line for the op being a `DataOp`.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1561
     case BodyGenTy::Priv:
+      if (!info.DevicePtrInfoMap.empty()) {
+        unsigned argIndex = 0;
----------------
Should this be an assert instead of an `if`? If not, Please add a comment on when it can and cannot be empty.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1563-1576
+        for (auto &devPtrOp : useDevPtrOperands) {
+          llvm::Value *mapOpValue = moduleTranslation.lookupValue(devPtrOp);
+          const auto &arg = region.front().getArgument(argIndex);
+          moduleTranslation.mapValue(arg,
+                                     info.DevicePtrInfoMap[mapOpValue].second);
+          argIndex++;
+        }
----------------
Might be worth creating a lambda and callling it separately with useDevPtrOperands, useDevAddrOperands.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1586
+    case BodyGenTy::NoPriv:
+      if (info.DevicePtrInfoMap.empty()) {
+        builder.restoreIP(codeGenIP);
----------------
Should this be an assert instead of an `if`? If not, Please add a comment on when it can and cannot be empty.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1387
   unsigned index = 0;
-  for (const auto &mapOp : mapOperands) {
-    if (!mapOp.getType().isa<LLVM::LLVMPointerType>()) {
-      // TODO: Only LLVMPointerTypes are handled.
-      combinedInfo.BasePointers.clear();
-      combinedInfo.Pointers.clear();
-      combinedInfo.Sizes.clear();
-      combinedInfo.Types.clear();
-      combinedInfo.Names.clear();
-      return;
+  auto fail = [&]() {
+    combinedInfo.BasePointers.clear();
----------------
Nit:This only needs to take in as reference `combinedInfo`.


================
Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:215
+
+llvm.func @_QPopenmp_target_use_dev_addr() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
----------------
Is the default usage of `dev_addr` also with the pointer type?
If not using a single reference example might be easier to follow.
```
llvm.func @_QPopenmp_target_use_dev_addr() {
  %0 = llvm.mlir.constant(1 : i64) : i64
  %a = llvm.alloca %0 x !llvm.ptr<i32> : (i64) -> !llvm.ptr<i32>
  omp.target_data  map((from -> %a : !llvm.ptr<i32>)) use_device_addr(%a : !llvm.ptr<i32>)  {
  ^bb0(%arg0: !llvm.ptr<i32>): 
    %1 = llvm.mlir.constant(10 : i32) : i32
    llvm.store %1, %arg0 : !llvm.ptr<i32>
    omp.terminator
  }
  llvm.return
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146648