[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

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



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1491
+mapTypes = exitDataOp.getMapTypes();
+mapperFunc = false;
+return success();

mehdi_amini wrote:
> This line is not needed after the fix you pushed right?
> 
> Seems like we could also just set `bool mapperFunc = 
> isa(op);` or something like that.
I've already removed this line in my fix.

Yes, I think we can get rid of the variable altogether and just pass the `isa` 
as an argument when calling. I'll add this change in one of my ongoing patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1491
+mapTypes = exitDataOp.getMapTypes();
+mapperFunc = false;
+return success();

This line is not needed after the fix you pushed right?

Seems like we could also just set `bool mapperFunc = 
isa(op);` or something like that.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1533
+ompLoc, builder.saveIP(), mapTypeFlags, mapNames, mapperAllocas,
+mapperFunc, deviceID, ifCond, processMapOpCB, bodyCB));
+  } else {

TIFitis wrote:
> TIFitis wrote:
> > mehdi_amini wrote:
> > > mapperFunc is used uninitialized here which is UB, can you look into this?
> > Thanks for pointing out, I'll push a patch to fix this.
> https://github.com/llvm/llvm-project/commit/9ea3fcfa380c6097fddd0d9a9b2c13f0f20bc41a
> 
> This fixes it.
Thanks for the quick fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1533
+ompLoc, builder.saveIP(), mapTypeFlags, mapNames, mapperAllocas,
+mapperFunc, deviceID, ifCond, processMapOpCB, bodyCB));
+  } else {

TIFitis wrote:
> mehdi_amini wrote:
> > mapperFunc is used uninitialized here which is UB, can you look into this?
> Thanks for pointing out, I'll push a patch to fix this.
https://github.com/llvm/llvm-project/commit/9ea3fcfa380c6097fddd0d9a9b2c13f0f20bc41a

This fixes it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1533
+ompLoc, builder.saveIP(), mapTypeFlags, mapNames, mapperAllocas,
+mapperFunc, deviceID, ifCond, processMapOpCB, bodyCB));
+  } else {

mehdi_amini wrote:
> mapperFunc is used uninitialized here which is UB, can you look into this?
Thanks for pointing out, I'll push a patch to fix this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.
Herald added a subscriber: bviyer.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1533
+ompLoc, builder.saveIP(), mapTypeFlags, mapNames, mapperAllocas,
+mapperFunc, deviceID, ifCond, processMapOpCB, bodyCB));
+  } else {

mapperFunc is used uninitialized here which is UB, can you look into this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2d373e4dc7e9: [MLIR][OpenMP] Added OMPIRBuilder support for 
Target Data directives (authored by TIFitis).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- /dev/null
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -0,0 +1,176 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+llvm.func @_QPopenmp_target_data() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
+  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+llvm.store %2, %1 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: %[[VAL_3:.*]] = alloca i32, i64 1, align 4
+// CHECK: br label %[[VAL_4:.*]]
+// CHECK:   entry:; preds = %[[VAL_5:.*]]
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_7]], align 8
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %[[VAL_8]], align 4
+// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_10:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr %[[VAL_11]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: br label %[[VAL_12:.*]]
+// CHECK:   omp.data.region:  ; preds = %[[VAL_4]]
+// CHECK: store i32 99, ptr %[[VAL_3]], align 4
+// CHECK: br label %[[VAL_13:.*]]
+// CHECK:   omp.region.cont:  ; preds = %[[VAL_12]]
+// CHECK: %[[VAL_14:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_15:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_16:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_14]], ptr %[[VAL_15]], ptr %[[VAL_16]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: ret void
+
+// -
+
+llvm.func @_QPopenmp_target_data_region(%1 : !llvm.ptr>) {
+  omp.target_data   map((from -> %1 : !llvm.ptr>)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+%3 = llvm.mlir.constant(1 : i64) : i64
+%4 = llvm.mlir.constant(1 : i64) : i64
+%5 = llvm.mlir.constant(0 : i64) : i64
+%6 = llvm.getelementptr %1[0, %5] : (!llvm.ptr>, i64) -> !llvm.ptr
+llvm.store %2, %6 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 2]
+// CHECK-LABEL: define void @_QPopenmp_target_data_region
+// CHECK: (ptr %[[ARG_0:.*]]) {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: br label %[[VAL_3:.*]]
+// CHECK:   entry:; preds = %[[VAL_4:.*]]
+// CHECK: %[[VAL_5:.*]] = 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 506563.
TIFitis added a comment.

Rebased.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- /dev/null
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -0,0 +1,176 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+llvm.func @_QPopenmp_target_data() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
+  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+llvm.store %2, %1 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: %[[VAL_3:.*]] = alloca i32, i64 1, align 4
+// CHECK: br label %[[VAL_4:.*]]
+// CHECK:   entry:; preds = %[[VAL_5:.*]]
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_7]], align 8
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %[[VAL_8]], align 4
+// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_10:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr %[[VAL_11]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: br label %[[VAL_12:.*]]
+// CHECK:   omp.data.region:  ; preds = %[[VAL_4]]
+// CHECK: store i32 99, ptr %[[VAL_3]], align 4
+// CHECK: br label %[[VAL_13:.*]]
+// CHECK:   omp.region.cont:  ; preds = %[[VAL_12]]
+// CHECK: %[[VAL_14:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_15:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_16:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_14]], ptr %[[VAL_15]], ptr %[[VAL_16]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: ret void
+
+// -
+
+llvm.func @_QPopenmp_target_data_region(%1 : !llvm.ptr>) {
+  omp.target_data   map((from -> %1 : !llvm.ptr>)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+%3 = llvm.mlir.constant(1 : i64) : i64
+%4 = llvm.mlir.constant(1 : i64) : i64
+%5 = llvm.mlir.constant(0 : i64) : i64
+%6 = llvm.getelementptr %1[0, %5] : (!llvm.ptr>, i64) -> !llvm.ptr
+llvm.store %2, %6 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 2]
+// CHECK-LABEL: define void @_QPopenmp_target_data_region
+// CHECK: (ptr %[[ARG_0:.*]]) {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: br label %[[VAL_3:.*]]
+// CHECK:   entry:; preds = %[[VAL_4:.*]]
+// CHECK: %[[VAL_5:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_6:.*]], ptr %[[VAL_5]], 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1523
+  auto bodyCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP) {
+// DataOp has only one region associated with it.
+auto  = cast(op).getRegion();

kiranchandramohan wrote:
> Nit: I think this should be an assertion.
AFAIK the `mlir op` for this only allows for one region.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1502-1510
+  llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
+  llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+  findAllocaInsertPoint(builder, moduleTranslation);
+
+  struct llvm::OpenMPIRBuilder::MapperAllocas mapperAllocas;
+  SmallVector mapTypeFlags;
+  SmallVector mapNames;

kiranchandramohan wrote:
> TIFitis wrote:
> > kiranchandramohan wrote:
> > > Can all this go into the OpenMP IRBuilder? And be passed back if 
> > > necessary via the callback.
> > If we decide to move processMapOperand then these can be moved along with 
> > it.
> Now that we decided not to move `processMapOperand`, can this be moved to the 
> OpenMPIRBuilder?
Hi, I am working on a patch to add `use_device_ptr` and `use_deice_addr` 
clauses. As part of that I have restructured how `mapOperands` are processed 
and separated the mlir and codegen part of it. These are already moved to 
OMPIRBuilder in that patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 506054.
TIFitis marked 5 inline comments as done.
TIFitis added a comment.

Addresed reviewer comments. Removed flang changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- /dev/null
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -0,0 +1,176 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+llvm.func @_QPopenmp_target_data() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
+  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+llvm.store %2, %1 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: %[[VAL_3:.*]] = alloca i32, i64 1, align 4
+// CHECK: br label %[[VAL_4:.*]]
+// CHECK:   entry:; preds = %[[VAL_5:.*]]
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_7]], align 8
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %[[VAL_8]], align 4
+// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_10:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr %[[VAL_11]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: br label %[[VAL_12:.*]]
+// CHECK:   omp.data.region:  ; preds = %[[VAL_4]]
+// CHECK: store i32 99, ptr %[[VAL_3]], align 4
+// CHECK: br label %[[VAL_13:.*]]
+// CHECK:   omp.region.cont:  ; preds = %[[VAL_12]]
+// CHECK: %[[VAL_14:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_15:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_16:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_14]], ptr %[[VAL_15]], ptr %[[VAL_16]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: ret void
+
+// -
+
+llvm.func @_QPopenmp_target_data_region(%1 : !llvm.ptr>) {
+  omp.target_data   map((from -> %1 : !llvm.ptr>)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+%3 = llvm.mlir.constant(1 : i64) : i64
+%4 = llvm.mlir.constant(1 : i64) : i64
+%5 = llvm.mlir.constant(0 : i64) : i64
+%6 = llvm.getelementptr %1[0, %5] : (!llvm.ptr>, i64) -> !llvm.ptr
+llvm.store %2, %6 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 2]
+// CHECK-LABEL: define void @_QPopenmp_target_data_region
+// CHECK: (ptr %[[ARG_0:.*]]) {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: br label %[[VAL_3:.*]]
+// CHECK:   entry:; preds = %[[VAL_4:.*]]
+// CHECK: %[[VAL_5:.*]] = getelementptr inbounds [1 x ptr], ptr 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: jplehr.

LGTM. Please wait a day before submission. Move the flang portion to a separate 
commit.




Comment at: flang/lib/Lower/OpenMP.cpp:727
 for (mlir::Value mapOp : mapOperand) {
+  auto checkType = mapOp.getType();
+  if (auto refType = checkType.dyn_cast())

kiranchandramohan wrote:
> These changes should go in a separate patch.
Nit: expand auto.



Comment at: flang/lib/Lower/OpenMP.cpp:727-738
+  auto checkType = mapOp.getType();
+  if (auto refType = checkType.dyn_cast())
+checkType = refType.getElementType();
+  if (auto boxType = checkType.dyn_cast())
+checkType = boxType.getElementType();
+
+  if (!(fir::isa_std_type(checkType) ||

These changes should go in a separate patch.



Comment at: flang/lib/Lower/OpenMP.cpp:737
+ !checkType.cast().hasDynamicExtents(
+TODO(currentLocation, "OMPD_target_data MapOperand type not 
supported");
+

Since TODO will print a "Not yet implemented" message, I think we can remove 
the trailing "not supported suffix".



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1523
+  auto bodyCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP) {
+// DataOp has only one region associated with it.
+auto  = cast(op).getRegion();

Nit: I think this should be an assertion.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1502-1510
+  llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
+  llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+  findAllocaInsertPoint(builder, moduleTranslation);
+
+  struct llvm::OpenMPIRBuilder::MapperAllocas mapperAllocas;
+  SmallVector mapTypeFlags;
+  SmallVector mapNames;

TIFitis wrote:
> kiranchandramohan wrote:
> > Can all this go into the OpenMP IRBuilder? And be passed back if necessary 
> > via the callback.
> If we decide to move processMapOperand then these can be moved along with it.
Now that we decided not to move `processMapOperand`, can this be moved to the 
OpenMPIRBuilder?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 505456.
TIFitis marked an inline comment as done.
TIFitis added a comment.

Fix test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  flang/lib/Lower/OpenMP.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- /dev/null
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -0,0 +1,176 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+llvm.func @_QPopenmp_target_data() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
+  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+llvm.store %2, %1 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: %[[VAL_3:.*]] = alloca i32, i64 1, align 4
+// CHECK: br label %[[VAL_4:.*]]
+// CHECK:   entry:; preds = %[[VAL_5:.*]]
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_7]], align 8
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %[[VAL_8]], align 4
+// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_10:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr %[[VAL_11]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: br label %[[VAL_12:.*]]
+// CHECK:   omp.data.region:  ; preds = %[[VAL_4]]
+// CHECK: store i32 99, ptr %[[VAL_3]], align 4
+// CHECK: br label %[[VAL_13:.*]]
+// CHECK:   omp.region.cont:  ; preds = %[[VAL_12]]
+// CHECK: %[[VAL_14:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_15:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_16:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_14]], ptr %[[VAL_15]], ptr %[[VAL_16]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: ret void
+
+// -
+
+llvm.func @_QPopenmp_target_data_region(%1 : !llvm.ptr>) {
+  omp.target_data   map((from -> %1 : !llvm.ptr>)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+%3 = llvm.mlir.constant(1 : i64) : i64
+%4 = llvm.mlir.constant(1 : i64) : i64
+%5 = llvm.mlir.constant(0 : i64) : i64
+%6 = llvm.getelementptr %1[0, %5] : (!llvm.ptr>, i64) -> !llvm.ptr
+llvm.store %2, %6 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 2]
+// CHECK-LABEL: define void @_QPopenmp_target_data_region
+// CHECK: (ptr %[[ARG_0:.*]]) {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: br label %[[VAL_3:.*]]
+// CHECK:   entry:; preds = %[[VAL_4:.*]]
+// CHECK: %[[VAL_5:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked an inline comment as done.
TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,

kiranchandramohan wrote:
> TIFitis wrote:
> > TIFitis wrote:
> > > TIFitis wrote:
> > > > kiranchandramohan wrote:
> > > > > kiranchandramohan wrote:
> > > > > > TIFitis wrote:
> > > > > > > TIFitis wrote:
> > > > > > > > kiranchandramohan wrote:
> > > > > > > > > TIFitis wrote:
> > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > TIFitis wrote:
> > > > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > > > Isn't it possible to sink this whole function into 
> > > > > > > > > > > > > the OpenMPIRBuilder by passing it a list of 
> > > > > > > > > > > > > `mapOpValue` and `mapTypeFlags`?
> > > > > > > > > > > > > `lvm::Value *mapOpValue = 
> > > > > > > > > > > > > moduleTranslation.lookupValue(mapOp);`
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Did i miss something? Or is this in anticipation of 
> > > > > > > > > > > > > more processing required for other types?
> > > > > > > > > > > > I'm not fully sure but we might need more MLIR related 
> > > > > > > > > > > > things when supporting types other than 
> > > > > > > > > > > > LLVMPointerType. Also there is a call to 
> > > > > > > > > > > > mlir::LLVM::createMappingInformation.
> > > > > > > > > > > > 
> > > > > > > > > > > > I guess it might still be possible to move most of it 
> > > > > > > > > > > > to the IRBuilder, would you like me to do that?
> > > > > > > > > > > Callbacks are useful when there is frontend-specific 
> > > > > > > > > > > handling that is required. If more types require to be 
> > > > > > > > > > > handled then it is better to have the callback. We can 
> > > > > > > > > > > revisit this after all types are handled. I assume, the 
> > > > > > > > > > > current handling is for scalars and arrays of known-size.
> > > > > > > > > > I am a novice at FORTRAN so I'm not aware of all  the types 
> > > > > > > > > > and scenarios.
> > > > > > > > > > 
> > > > > > > > > > I've tested the following cases and they work end-to-end:
> > > > > > > > > > 
> > > > > > > > > > **Fortran:**
> > > > > > > > > > ```
> > > > > > > > > > subroutine openmp_target_data_region(a)
> > > > > > > > > > real :: a(*)
> > > > > > > > > > integer :: b(1024)
> > > > > > > > > > character :: c
> > > > > > > > > > integer, pointer :: p
> > > > > > > > > > !$omp target enter data map(to: a, b, c, p)
> > > > > > > > > > end subroutine openmp_target_data_region
> > > > > > > > > > ```
> > > > > > > > > > 
> > > > > > > > > > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 
> > > > > > > > > > -o test.ll`** ):**
> > > > > > > > > > 
> > > > > > > > > > ```
> > > > > > > > > > ; ModuleID = 'FIRModule'
> > > > > > > > > > source_filename = "FIRModule"
> > > > > > > > > > target datalayout = 
> > > > > > > > > > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > > > > > > > > > target triple = "x86_64-unknown-linux-gnu"
> > > > > > > > > > 
> > > > > > > > > > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > > > > > > > > > 
> > > > > > > > > > @0 = private unnamed_addr constant [13 x i8] 
> > > > > > > > > > c"loc(unknown)\00", align 1
> > > > > > > > > > @1 = private unnamed_addr constant [56 x i8] 
> > > > > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00",
> > > > > > > > > >  align 1
> > > > > > > > > > @2 = private unnamed_addr constant [56 x i8] 
> > > > > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00",
> > > > > > > > > >  align 1
> > > > > > > > > > @3 = private unnamed_addr constant [56 x i8] 
> > > > > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00",
> > > > > > > > > >  align 1
> > > > > > > > > > @4 = private unnamed_addr constant [23 x i8] 
> > > > > > > > > > c";unknown;unknown;0;0;;\00", align 1
> > > > > > > > > > @5 = private unnamed_addr constant %struct.ident_t { i32 0, 
> > > > > > > > > > i32 2, i32 0, i32 22, ptr @4 }, align 8
> > > > > > > > > > @.offload_maptypes = private unnamed_addr constant [4 x 
> > > > > > > > > > i64] [i64 1, i64 1, i64 1, i64 1]
> > > > > > > > > > @.offload_mapnames = private constant [4 x ptr] [ptr @0, 
> > > > > > > > > > ptr @1, ptr @2, ptr @3]
> > > > > > > > > > 
> > > > > > > > > > declare ptr @malloc(i64)
> > > > > > > > > > 
> > > > > > > > > > declare void @free(ptr)
> > > > > > > > > > 
> > > > > > > > > > define void @openmp_target_data_region_(ptr %0) {
> > > > > > > > > >   %2 = alloca [4 x ptr], align 8
> > > > > > > > > >   %3 = alloca [4 x ptr], align 8
> > > > > > > > > >   %4 = alloca [4 x i64], align 8
> > > > > > > > > >   %5 = alloca [1024 x i32], i64 1, align 4
> > > > > > > > > >   %6 = alloca 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 505116.
TIFitis added a comment.
Herald added a project: Flang.

Added TODO for unsupported map operand types in Flang frontend


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  flang/lib/Lower/OpenMP.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- /dev/null
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -0,0 +1,176 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+llvm.func @_QPopenmp_target_data() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
+  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+llvm.store %2, %1 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: %[[VAL_3:.*]] = alloca i32, i64 1, align 4
+// CHECK: br label %[[VAL_4:.*]]
+// CHECK:   entry:; preds = %[[VAL_5:.*]]
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_7]], align 8
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %[[VAL_8]], align 4
+// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_10:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr %[[VAL_11]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: br label %[[VAL_12:.*]]
+// CHECK:   omp.data.region:  ; preds = %[[VAL_4]]
+// CHECK: store i32 99, ptr %[[VAL_3]], align 4
+// CHECK: br label %[[VAL_13:.*]]
+// CHECK:   omp.region.cont:  ; preds = %[[VAL_12]]
+// CHECK: %[[VAL_14:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_15:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_16:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_14]], ptr %[[VAL_15]], ptr %[[VAL_16]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: ret void
+
+// -
+
+llvm.func @_QPopenmp_target_data_region(%1 : !llvm.ptr>) {
+  omp.target_data   map((from -> %1 : !llvm.ptr>)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+%3 = llvm.mlir.constant(1 : i64) : i64
+%4 = llvm.mlir.constant(1 : i64) : i64
+%5 = llvm.mlir.constant(0 : i64) : i64
+%6 = llvm.getelementptr %1[0, %5] : (!llvm.ptr>, i64) -> !llvm.ptr
+llvm.store %2, %6 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 2]
+// CHECK-LABEL: define void @_QPopenmp_target_data_region
+// CHECK: (ptr %[[ARG_0:.*]]) {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: br label %[[VAL_3:.*]]
+// CHECK:   entry:; preds = %[[VAL_4:.*]]
+// CHECK: %[[VAL_5:.*]] = 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,

TIFitis wrote:
> TIFitis wrote:
> > TIFitis wrote:
> > > kiranchandramohan wrote:
> > > > kiranchandramohan wrote:
> > > > > TIFitis wrote:
> > > > > > TIFitis wrote:
> > > > > > > kiranchandramohan wrote:
> > > > > > > > TIFitis wrote:
> > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > TIFitis wrote:
> > > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > > Isn't it possible to sink this whole function into the 
> > > > > > > > > > > > OpenMPIRBuilder by passing it a list of `mapOpValue` 
> > > > > > > > > > > > and `mapTypeFlags`?
> > > > > > > > > > > > `lvm::Value *mapOpValue = 
> > > > > > > > > > > > moduleTranslation.lookupValue(mapOp);`
> > > > > > > > > > > > 
> > > > > > > > > > > > Did i miss something? Or is this in anticipation of 
> > > > > > > > > > > > more processing required for other types?
> > > > > > > > > > > I'm not fully sure but we might need more MLIR related 
> > > > > > > > > > > things when supporting types other than LLVMPointerType. 
> > > > > > > > > > > Also there is a call to 
> > > > > > > > > > > mlir::LLVM::createMappingInformation.
> > > > > > > > > > > 
> > > > > > > > > > > I guess it might still be possible to move most of it to 
> > > > > > > > > > > the IRBuilder, would you like me to do that?
> > > > > > > > > > Callbacks are useful when there is frontend-specific 
> > > > > > > > > > handling that is required. If more types require to be 
> > > > > > > > > > handled then it is better to have the callback. We can 
> > > > > > > > > > revisit this after all types are handled. I assume, the 
> > > > > > > > > > current handling is for scalars and arrays of known-size.
> > > > > > > > > I am a novice at FORTRAN so I'm not aware of all  the types 
> > > > > > > > > and scenarios.
> > > > > > > > > 
> > > > > > > > > I've tested the following cases and they work end-to-end:
> > > > > > > > > 
> > > > > > > > > **Fortran:**
> > > > > > > > > ```
> > > > > > > > > subroutine openmp_target_data_region(a)
> > > > > > > > > real :: a(*)
> > > > > > > > > integer :: b(1024)
> > > > > > > > > character :: c
> > > > > > > > > integer, pointer :: p
> > > > > > > > > !$omp target enter data map(to: a, b, c, p)
> > > > > > > > > end subroutine openmp_target_data_region
> > > > > > > > > ```
> > > > > > > > > 
> > > > > > > > > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o 
> > > > > > > > > test.ll`** ):**
> > > > > > > > > 
> > > > > > > > > ```
> > > > > > > > > ; ModuleID = 'FIRModule'
> > > > > > > > > source_filename = "FIRModule"
> > > > > > > > > target datalayout = 
> > > > > > > > > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > > > > > > > > target triple = "x86_64-unknown-linux-gnu"
> > > > > > > > > 
> > > > > > > > > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > > > > > > > > 
> > > > > > > > > @0 = private unnamed_addr constant [13 x i8] 
> > > > > > > > > c"loc(unknown)\00", align 1
> > > > > > > > > @1 = private unnamed_addr constant [56 x i8] 
> > > > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00",
> > > > > > > > >  align 1
> > > > > > > > > @2 = private unnamed_addr constant [56 x i8] 
> > > > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00",
> > > > > > > > >  align 1
> > > > > > > > > @3 = private unnamed_addr constant [56 x i8] 
> > > > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00",
> > > > > > > > >  align 1
> > > > > > > > > @4 = private unnamed_addr constant [23 x i8] 
> > > > > > > > > c";unknown;unknown;0;0;;\00", align 1
> > > > > > > > > @5 = private unnamed_addr constant %struct.ident_t { i32 0, 
> > > > > > > > > i32 2, i32 0, i32 22, ptr @4 }, align 8
> > > > > > > > > @.offload_maptypes = private unnamed_addr constant [4 x i64] 
> > > > > > > > > [i64 1, i64 1, i64 1, i64 1]
> > > > > > > > > @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr 
> > > > > > > > > @1, ptr @2, ptr @3]
> > > > > > > > > 
> > > > > > > > > declare ptr @malloc(i64)
> > > > > > > > > 
> > > > > > > > > declare void @free(ptr)
> > > > > > > > > 
> > > > > > > > > define void @openmp_target_data_region_(ptr %0) {
> > > > > > > > >   %2 = alloca [4 x ptr], align 8
> > > > > > > > >   %3 = alloca [4 x ptr], align 8
> > > > > > > > >   %4 = alloca [4 x i64], align 8
> > > > > > > > >   %5 = alloca [1024 x i32], i64 1, align 4
> > > > > > > > >   %6 = alloca [1 x i8], i64 1, align 1
> > > > > > > > >   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 
> > > > > > > > > 8
> > > > > > > > >   %8 = alloca ptr, i64 1, align 8
> > > > > > > > >   store ptr null, ptr %8, align 8
> > > > 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,

TIFitis wrote:
> TIFitis wrote:
> > kiranchandramohan wrote:
> > > kiranchandramohan wrote:
> > > > TIFitis wrote:
> > > > > TIFitis wrote:
> > > > > > kiranchandramohan wrote:
> > > > > > > TIFitis wrote:
> > > > > > > > kiranchandramohan wrote:
> > > > > > > > > TIFitis wrote:
> > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > Isn't it possible to sink this whole function into the 
> > > > > > > > > > > OpenMPIRBuilder by passing it a list of `mapOpValue` and 
> > > > > > > > > > > `mapTypeFlags`?
> > > > > > > > > > > `lvm::Value *mapOpValue = 
> > > > > > > > > > > moduleTranslation.lookupValue(mapOp);`
> > > > > > > > > > > 
> > > > > > > > > > > Did i miss something? Or is this in anticipation of more 
> > > > > > > > > > > processing required for other types?
> > > > > > > > > > I'm not fully sure but we might need more MLIR related 
> > > > > > > > > > things when supporting types other than LLVMPointerType. 
> > > > > > > > > > Also there is a call to 
> > > > > > > > > > mlir::LLVM::createMappingInformation.
> > > > > > > > > > 
> > > > > > > > > > I guess it might still be possible to move most of it to 
> > > > > > > > > > the IRBuilder, would you like me to do that?
> > > > > > > > > Callbacks are useful when there is frontend-specific handling 
> > > > > > > > > that is required. If more types require to be handled then it 
> > > > > > > > > is better to have the callback. We can revisit this after all 
> > > > > > > > > types are handled. I assume, the current handling is for 
> > > > > > > > > scalars and arrays of known-size.
> > > > > > > > I am a novice at FORTRAN so I'm not aware of all  the types and 
> > > > > > > > scenarios.
> > > > > > > > 
> > > > > > > > I've tested the following cases and they work end-to-end:
> > > > > > > > 
> > > > > > > > **Fortran:**
> > > > > > > > ```
> > > > > > > > subroutine openmp_target_data_region(a)
> > > > > > > > real :: a(*)
> > > > > > > > integer :: b(1024)
> > > > > > > > character :: c
> > > > > > > > integer, pointer :: p
> > > > > > > > !$omp target enter data map(to: a, b, c, p)
> > > > > > > > end subroutine openmp_target_data_region
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o 
> > > > > > > > test.ll`** ):**
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > ; ModuleID = 'FIRModule'
> > > > > > > > source_filename = "FIRModule"
> > > > > > > > target datalayout = 
> > > > > > > > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > > > > > > > target triple = "x86_64-unknown-linux-gnu"
> > > > > > > > 
> > > > > > > > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > > > > > > > 
> > > > > > > > @0 = private unnamed_addr constant [13 x i8] 
> > > > > > > > c"loc(unknown)\00", align 1
> > > > > > > > @1 = private unnamed_addr constant [56 x i8] 
> > > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00", 
> > > > > > > > align 1
> > > > > > > > @2 = private unnamed_addr constant [56 x i8] 
> > > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00", 
> > > > > > > > align 1
> > > > > > > > @3 = private unnamed_addr constant [56 x i8] 
> > > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00", 
> > > > > > > > align 1
> > > > > > > > @4 = private unnamed_addr constant [23 x i8] 
> > > > > > > > c";unknown;unknown;0;0;;\00", align 1
> > > > > > > > @5 = private unnamed_addr constant %struct.ident_t { i32 0, i32 
> > > > > > > > 2, i32 0, i32 22, ptr @4 }, align 8
> > > > > > > > @.offload_maptypes = private unnamed_addr constant [4 x i64] 
> > > > > > > > [i64 1, i64 1, i64 1, i64 1]
> > > > > > > > @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr 
> > > > > > > > @1, ptr @2, ptr @3]
> > > > > > > > 
> > > > > > > > declare ptr @malloc(i64)
> > > > > > > > 
> > > > > > > > declare void @free(ptr)
> > > > > > > > 
> > > > > > > > define void @openmp_target_data_region_(ptr %0) {
> > > > > > > >   %2 = alloca [4 x ptr], align 8
> > > > > > > >   %3 = alloca [4 x ptr], align 8
> > > > > > > >   %4 = alloca [4 x i64], align 8
> > > > > > > >   %5 = alloca [1024 x i32], i64 1, align 4
> > > > > > > >   %6 = alloca [1 x i8], i64 1, align 1
> > > > > > > >   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
> > > > > > > >   %8 = alloca ptr, i64 1, align 8
> > > > > > > >   store ptr null, ptr %8, align 8
> > > > > > > >   br label %entry
> > > > > > > > 
> > > > > > > > entry:; preds = %1
> > > > > > > >   %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
> > > > > > > >   store ptr 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked an inline comment as done.
TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,

TIFitis wrote:
> kiranchandramohan wrote:
> > kiranchandramohan wrote:
> > > TIFitis wrote:
> > > > TIFitis wrote:
> > > > > kiranchandramohan wrote:
> > > > > > TIFitis wrote:
> > > > > > > kiranchandramohan wrote:
> > > > > > > > TIFitis wrote:
> > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > Isn't it possible to sink this whole function into the 
> > > > > > > > > > OpenMPIRBuilder by passing it a list of `mapOpValue` and 
> > > > > > > > > > `mapTypeFlags`?
> > > > > > > > > > `lvm::Value *mapOpValue = 
> > > > > > > > > > moduleTranslation.lookupValue(mapOp);`
> > > > > > > > > > 
> > > > > > > > > > Did i miss something? Or is this in anticipation of more 
> > > > > > > > > > processing required for other types?
> > > > > > > > > I'm not fully sure but we might need more MLIR related things 
> > > > > > > > > when supporting types other than LLVMPointerType. Also there 
> > > > > > > > > is a call to mlir::LLVM::createMappingInformation.
> > > > > > > > > 
> > > > > > > > > I guess it might still be possible to move most of it to the 
> > > > > > > > > IRBuilder, would you like me to do that?
> > > > > > > > Callbacks are useful when there is frontend-specific handling 
> > > > > > > > that is required. If more types require to be handled then it 
> > > > > > > > is better to have the callback. We can revisit this after all 
> > > > > > > > types are handled. I assume, the current handling is for 
> > > > > > > > scalars and arrays of known-size.
> > > > > > > I am a novice at FORTRAN so I'm not aware of all  the types and 
> > > > > > > scenarios.
> > > > > > > 
> > > > > > > I've tested the following cases and they work end-to-end:
> > > > > > > 
> > > > > > > **Fortran:**
> > > > > > > ```
> > > > > > > subroutine openmp_target_data_region(a)
> > > > > > > real :: a(*)
> > > > > > > integer :: b(1024)
> > > > > > > character :: c
> > > > > > > integer, pointer :: p
> > > > > > > !$omp target enter data map(to: a, b, c, p)
> > > > > > > end subroutine openmp_target_data_region
> > > > > > > ```
> > > > > > > 
> > > > > > > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o 
> > > > > > > test.ll`** ):**
> > > > > > > 
> > > > > > > ```
> > > > > > > ; ModuleID = 'FIRModule'
> > > > > > > source_filename = "FIRModule"
> > > > > > > target datalayout = 
> > > > > > > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > > > > > > target triple = "x86_64-unknown-linux-gnu"
> > > > > > > 
> > > > > > > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > > > > > > 
> > > > > > > @0 = private unnamed_addr constant [13 x i8] c"loc(unknown)\00", 
> > > > > > > align 1
> > > > > > > @1 = private unnamed_addr constant [56 x i8] 
> > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00", 
> > > > > > > align 1
> > > > > > > @2 = private unnamed_addr constant [56 x i8] 
> > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00", 
> > > > > > > align 1
> > > > > > > @3 = private unnamed_addr constant [56 x i8] 
> > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00", 
> > > > > > > align 1
> > > > > > > @4 = private unnamed_addr constant [23 x i8] 
> > > > > > > c";unknown;unknown;0;0;;\00", align 1
> > > > > > > @5 = private unnamed_addr constant %struct.ident_t { i32 0, i32 
> > > > > > > 2, i32 0, i32 22, ptr @4 }, align 8
> > > > > > > @.offload_maptypes = private unnamed_addr constant [4 x i64] [i64 
> > > > > > > 1, i64 1, i64 1, i64 1]
> > > > > > > @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr @1, 
> > > > > > > ptr @2, ptr @3]
> > > > > > > 
> > > > > > > declare ptr @malloc(i64)
> > > > > > > 
> > > > > > > declare void @free(ptr)
> > > > > > > 
> > > > > > > define void @openmp_target_data_region_(ptr %0) {
> > > > > > >   %2 = alloca [4 x ptr], align 8
> > > > > > >   %3 = alloca [4 x ptr], align 8
> > > > > > >   %4 = alloca [4 x i64], align 8
> > > > > > >   %5 = alloca [1024 x i32], i64 1, align 4
> > > > > > >   %6 = alloca [1 x i8], i64 1, align 1
> > > > > > >   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
> > > > > > >   %8 = alloca ptr, i64 1, align 8
> > > > > > >   store ptr null, ptr %8, align 8
> > > > > > >   br label %entry
> > > > > > > 
> > > > > > > entry:; preds = %1
> > > > > > >   %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
> > > > > > >   store ptr %0, ptr %9, align 8
> > > > > > >   %10 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
> > > > > > >   store ptr %0, ptr %10, align 8
> > > > > > >   %11 = getelementptr 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked 2 inline comments as done.
TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,

kiranchandramohan wrote:
> kiranchandramohan wrote:
> > TIFitis wrote:
> > > TIFitis wrote:
> > > > kiranchandramohan wrote:
> > > > > TIFitis wrote:
> > > > > > kiranchandramohan wrote:
> > > > > > > TIFitis wrote:
> > > > > > > > kiranchandramohan wrote:
> > > > > > > > > Isn't it possible to sink this whole function into the 
> > > > > > > > > OpenMPIRBuilder by passing it a list of `mapOpValue` and 
> > > > > > > > > `mapTypeFlags`?
> > > > > > > > > `lvm::Value *mapOpValue = 
> > > > > > > > > moduleTranslation.lookupValue(mapOp);`
> > > > > > > > > 
> > > > > > > > > Did i miss something? Or is this in anticipation of more 
> > > > > > > > > processing required for other types?
> > > > > > > > I'm not fully sure but we might need more MLIR related things 
> > > > > > > > when supporting types other than LLVMPointerType. Also there is 
> > > > > > > > a call to mlir::LLVM::createMappingInformation.
> > > > > > > > 
> > > > > > > > I guess it might still be possible to move most of it to the 
> > > > > > > > IRBuilder, would you like me to do that?
> > > > > > > Callbacks are useful when there is frontend-specific handling 
> > > > > > > that is required. If more types require to be handled then it is 
> > > > > > > better to have the callback. We can revisit this after all types 
> > > > > > > are handled. I assume, the current handling is for scalars and 
> > > > > > > arrays of known-size.
> > > > > > I am a novice at FORTRAN so I'm not aware of all  the types and 
> > > > > > scenarios.
> > > > > > 
> > > > > > I've tested the following cases and they work end-to-end:
> > > > > > 
> > > > > > **Fortran:**
> > > > > > ```
> > > > > > subroutine openmp_target_data_region(a)
> > > > > > real :: a(*)
> > > > > > integer :: b(1024)
> > > > > > character :: c
> > > > > > integer, pointer :: p
> > > > > > !$omp target enter data map(to: a, b, c, p)
> > > > > > end subroutine openmp_target_data_region
> > > > > > ```
> > > > > > 
> > > > > > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o 
> > > > > > test.ll`** ):**
> > > > > > 
> > > > > > ```
> > > > > > ; ModuleID = 'FIRModule'
> > > > > > source_filename = "FIRModule"
> > > > > > target datalayout = 
> > > > > > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > > > > > target triple = "x86_64-unknown-linux-gnu"
> > > > > > 
> > > > > > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > > > > > 
> > > > > > @0 = private unnamed_addr constant [13 x i8] c"loc(unknown)\00", 
> > > > > > align 1
> > > > > > @1 = private unnamed_addr constant [56 x i8] 
> > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00", 
> > > > > > align 1
> > > > > > @2 = private unnamed_addr constant [56 x i8] 
> > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00", 
> > > > > > align 1
> > > > > > @3 = private unnamed_addr constant [56 x i8] 
> > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00", 
> > > > > > align 1
> > > > > > @4 = private unnamed_addr constant [23 x i8] 
> > > > > > c";unknown;unknown;0;0;;\00", align 1
> > > > > > @5 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, 
> > > > > > i32 0, i32 22, ptr @4 }, align 8
> > > > > > @.offload_maptypes = private unnamed_addr constant [4 x i64] [i64 
> > > > > > 1, i64 1, i64 1, i64 1]
> > > > > > @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr @1, 
> > > > > > ptr @2, ptr @3]
> > > > > > 
> > > > > > declare ptr @malloc(i64)
> > > > > > 
> > > > > > declare void @free(ptr)
> > > > > > 
> > > > > > define void @openmp_target_data_region_(ptr %0) {
> > > > > >   %2 = alloca [4 x ptr], align 8
> > > > > >   %3 = alloca [4 x ptr], align 8
> > > > > >   %4 = alloca [4 x i64], align 8
> > > > > >   %5 = alloca [1024 x i32], i64 1, align 4
> > > > > >   %6 = alloca [1 x i8], i64 1, align 1
> > > > > >   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
> > > > > >   %8 = alloca ptr, i64 1, align 8
> > > > > >   store ptr null, ptr %8, align 8
> > > > > >   br label %entry
> > > > > > 
> > > > > > entry:; preds = %1
> > > > > >   %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
> > > > > >   store ptr %0, ptr %9, align 8
> > > > > >   %10 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
> > > > > >   store ptr %0, ptr %10, align 8
> > > > > >   %11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
> > > > > >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to 
> > > > > > i64), ptr %11, align 8
> > > > > >   %12 = getelementptr inbounds [4 x ptr], ptr 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.
Herald added a subscriber: sunshaoce.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,

kiranchandramohan wrote:
> TIFitis wrote:
> > TIFitis wrote:
> > > kiranchandramohan wrote:
> > > > TIFitis wrote:
> > > > > kiranchandramohan wrote:
> > > > > > TIFitis wrote:
> > > > > > > kiranchandramohan wrote:
> > > > > > > > Isn't it possible to sink this whole function into the 
> > > > > > > > OpenMPIRBuilder by passing it a list of `mapOpValue` and 
> > > > > > > > `mapTypeFlags`?
> > > > > > > > `lvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);`
> > > > > > > > 
> > > > > > > > Did i miss something? Or is this in anticipation of more 
> > > > > > > > processing required for other types?
> > > > > > > I'm not fully sure but we might need more MLIR related things 
> > > > > > > when supporting types other than LLVMPointerType. Also there is a 
> > > > > > > call to mlir::LLVM::createMappingInformation.
> > > > > > > 
> > > > > > > I guess it might still be possible to move most of it to the 
> > > > > > > IRBuilder, would you like me to do that?
> > > > > > Callbacks are useful when there is frontend-specific handling that 
> > > > > > is required. If more types require to be handled then it is better 
> > > > > > to have the callback. We can revisit this after all types are 
> > > > > > handled. I assume, the current handling is for scalars and arrays 
> > > > > > of known-size.
> > > > > I am a novice at FORTRAN so I'm not aware of all  the types and 
> > > > > scenarios.
> > > > > 
> > > > > I've tested the following cases and they work end-to-end:
> > > > > 
> > > > > **Fortran:**
> > > > > ```
> > > > > subroutine openmp_target_data_region(a)
> > > > > real :: a(*)
> > > > > integer :: b(1024)
> > > > > character :: c
> > > > > integer, pointer :: p
> > > > > !$omp target enter data map(to: a, b, c, p)
> > > > > end subroutine openmp_target_data_region
> > > > > ```
> > > > > 
> > > > > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o 
> > > > > test.ll`** ):**
> > > > > 
> > > > > ```
> > > > > ; ModuleID = 'FIRModule'
> > > > > source_filename = "FIRModule"
> > > > > target datalayout = 
> > > > > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > > > > target triple = "x86_64-unknown-linux-gnu"
> > > > > 
> > > > > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > > > > 
> > > > > @0 = private unnamed_addr constant [13 x i8] c"loc(unknown)\00", 
> > > > > align 1
> > > > > @1 = private unnamed_addr constant [56 x i8] 
> > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00", align 1
> > > > > @2 = private unnamed_addr constant [56 x i8] 
> > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00", align 1
> > > > > @3 = private unnamed_addr constant [56 x i8] 
> > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00", align 1
> > > > > @4 = private unnamed_addr constant [23 x i8] 
> > > > > c";unknown;unknown;0;0;;\00", align 1
> > > > > @5 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, 
> > > > > i32 0, i32 22, ptr @4 }, align 8
> > > > > @.offload_maptypes = private unnamed_addr constant [4 x i64] [i64 1, 
> > > > > i64 1, i64 1, i64 1]
> > > > > @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr @1, ptr 
> > > > > @2, ptr @3]
> > > > > 
> > > > > declare ptr @malloc(i64)
> > > > > 
> > > > > declare void @free(ptr)
> > > > > 
> > > > > define void @openmp_target_data_region_(ptr %0) {
> > > > >   %2 = alloca [4 x ptr], align 8
> > > > >   %3 = alloca [4 x ptr], align 8
> > > > >   %4 = alloca [4 x i64], align 8
> > > > >   %5 = alloca [1024 x i32], i64 1, align 4
> > > > >   %6 = alloca [1 x i8], i64 1, align 1
> > > > >   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
> > > > >   %8 = alloca ptr, i64 1, align 8
> > > > >   store ptr null, ptr %8, align 8
> > > > >   br label %entry
> > > > > 
> > > > > entry:; preds = %1
> > > > >   %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
> > > > >   store ptr %0, ptr %9, align 8
> > > > >   %10 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
> > > > >   store ptr %0, ptr %10, align 8
> > > > >   %11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
> > > > >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to 
> > > > > i64), ptr %11, align 8
> > > > >   %12 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 1
> > > > >   store ptr %5, ptr %12, align 8
> > > > >   %13 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 1
> > > > >   store ptr %5, ptr %13, align 8
> > > > >   %14 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 1
> > > > >   store 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,

TIFitis wrote:
> TIFitis wrote:
> > kiranchandramohan wrote:
> > > TIFitis wrote:
> > > > kiranchandramohan wrote:
> > > > > TIFitis wrote:
> > > > > > kiranchandramohan wrote:
> > > > > > > Isn't it possible to sink this whole function into the 
> > > > > > > OpenMPIRBuilder by passing it a list of `mapOpValue` and 
> > > > > > > `mapTypeFlags`?
> > > > > > > `lvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);`
> > > > > > > 
> > > > > > > Did i miss something? Or is this in anticipation of more 
> > > > > > > processing required for other types?
> > > > > > I'm not fully sure but we might need more MLIR related things when 
> > > > > > supporting types other than LLVMPointerType. Also there is a call 
> > > > > > to mlir::LLVM::createMappingInformation.
> > > > > > 
> > > > > > I guess it might still be possible to move most of it to the 
> > > > > > IRBuilder, would you like me to do that?
> > > > > Callbacks are useful when there is frontend-specific handling that is 
> > > > > required. If more types require to be handled then it is better to 
> > > > > have the callback. We can revisit this after all types are handled. I 
> > > > > assume, the current handling is for scalars and arrays of known-size.
> > > > I am a novice at FORTRAN so I'm not aware of all  the types and 
> > > > scenarios.
> > > > 
> > > > I've tested the following cases and they work end-to-end:
> > > > 
> > > > **Fortran:**
> > > > ```
> > > > subroutine openmp_target_data_region(a)
> > > > real :: a(*)
> > > > integer :: b(1024)
> > > > character :: c
> > > > integer, pointer :: p
> > > > !$omp target enter data map(to: a, b, c, p)
> > > > end subroutine openmp_target_data_region
> > > > ```
> > > > 
> > > > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o test.ll`** 
> > > > ):**
> > > > 
> > > > ```
> > > > ; ModuleID = 'FIRModule'
> > > > source_filename = "FIRModule"
> > > > target datalayout = 
> > > > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > > > target triple = "x86_64-unknown-linux-gnu"
> > > > 
> > > > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > > > 
> > > > @0 = private unnamed_addr constant [13 x i8] c"loc(unknown)\00", align 1
> > > > @1 = private unnamed_addr constant [56 x i8] 
> > > > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00", align 1
> > > > @2 = private unnamed_addr constant [56 x i8] 
> > > > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00", align 1
> > > > @3 = private unnamed_addr constant [56 x i8] 
> > > > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00", align 1
> > > > @4 = private unnamed_addr constant [23 x i8] 
> > > > c";unknown;unknown;0;0;;\00", align 1
> > > > @5 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 
> > > > 0, i32 22, ptr @4 }, align 8
> > > > @.offload_maptypes = private unnamed_addr constant [4 x i64] [i64 1, 
> > > > i64 1, i64 1, i64 1]
> > > > @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr @1, ptr 
> > > > @2, ptr @3]
> > > > 
> > > > declare ptr @malloc(i64)
> > > > 
> > > > declare void @free(ptr)
> > > > 
> > > > define void @openmp_target_data_region_(ptr %0) {
> > > >   %2 = alloca [4 x ptr], align 8
> > > >   %3 = alloca [4 x ptr], align 8
> > > >   %4 = alloca [4 x i64], align 8
> > > >   %5 = alloca [1024 x i32], i64 1, align 4
> > > >   %6 = alloca [1 x i8], i64 1, align 1
> > > >   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
> > > >   %8 = alloca ptr, i64 1, align 8
> > > >   store ptr null, ptr %8, align 8
> > > >   br label %entry
> > > > 
> > > > entry:; preds = %1
> > > >   %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
> > > >   store ptr %0, ptr %9, align 8
> > > >   %10 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
> > > >   store ptr %0, ptr %10, align 8
> > > >   %11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
> > > >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), 
> > > > ptr %11, align 8
> > > >   %12 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 1
> > > >   store ptr %5, ptr %12, align 8
> > > >   %13 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 1
> > > >   store ptr %5, ptr %13, align 8
> > > >   %14 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 1
> > > >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), 
> > > > ptr %14, align 8
> > > >   %15 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 2
> > > >   store ptr %6, ptr %15, align 8
> > > >   %16 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 2
> > > >  

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,

TIFitis wrote:
> kiranchandramohan wrote:
> > TIFitis wrote:
> > > kiranchandramohan wrote:
> > > > TIFitis wrote:
> > > > > kiranchandramohan wrote:
> > > > > > Isn't it possible to sink this whole function into the 
> > > > > > OpenMPIRBuilder by passing it a list of `mapOpValue` and 
> > > > > > `mapTypeFlags`?
> > > > > > `lvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);`
> > > > > > 
> > > > > > Did i miss something? Or is this in anticipation of more processing 
> > > > > > required for other types?
> > > > > I'm not fully sure but we might need more MLIR related things when 
> > > > > supporting types other than LLVMPointerType. Also there is a call to 
> > > > > mlir::LLVM::createMappingInformation.
> > > > > 
> > > > > I guess it might still be possible to move most of it to the 
> > > > > IRBuilder, would you like me to do that?
> > > > Callbacks are useful when there is frontend-specific handling that is 
> > > > required. If more types require to be handled then it is better to have 
> > > > the callback. We can revisit this after all types are handled. I 
> > > > assume, the current handling is for scalars and arrays of known-size.
> > > I am a novice at FORTRAN so I'm not aware of all  the types and scenarios.
> > > 
> > > I've tested the following cases and they work end-to-end:
> > > 
> > > **Fortran:**
> > > ```
> > > subroutine openmp_target_data_region(a)
> > > real :: a(*)
> > > integer :: b(1024)
> > > character :: c
> > > integer, pointer :: p
> > > !$omp target enter data map(to: a, b, c, p)
> > > end subroutine openmp_target_data_region
> > > ```
> > > 
> > > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o test.ll`** 
> > > ):**
> > > 
> > > ```
> > > ; ModuleID = 'FIRModule'
> > > source_filename = "FIRModule"
> > > target datalayout = 
> > > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > > target triple = "x86_64-unknown-linux-gnu"
> > > 
> > > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > > 
> > > @0 = private unnamed_addr constant [13 x i8] c"loc(unknown)\00", align 1
> > > @1 = private unnamed_addr constant [56 x i8] 
> > > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00", align 1
> > > @2 = private unnamed_addr constant [56 x i8] 
> > > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00", align 1
> > > @3 = private unnamed_addr constant [56 x i8] 
> > > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00", align 1
> > > @4 = private unnamed_addr constant [23 x i8] 
> > > c";unknown;unknown;0;0;;\00", align 1
> > > @5 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, 
> > > i32 22, ptr @4 }, align 8
> > > @.offload_maptypes = private unnamed_addr constant [4 x i64] [i64 1, i64 
> > > 1, i64 1, i64 1]
> > > @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr @1, ptr @2, 
> > > ptr @3]
> > > 
> > > declare ptr @malloc(i64)
> > > 
> > > declare void @free(ptr)
> > > 
> > > define void @openmp_target_data_region_(ptr %0) {
> > >   %2 = alloca [4 x ptr], align 8
> > >   %3 = alloca [4 x ptr], align 8
> > >   %4 = alloca [4 x i64], align 8
> > >   %5 = alloca [1024 x i32], i64 1, align 4
> > >   %6 = alloca [1 x i8], i64 1, align 1
> > >   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
> > >   %8 = alloca ptr, i64 1, align 8
> > >   store ptr null, ptr %8, align 8
> > >   br label %entry
> > > 
> > > entry:; preds = %1
> > >   %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
> > >   store ptr %0, ptr %9, align 8
> > >   %10 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
> > >   store ptr %0, ptr %10, align 8
> > >   %11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
> > >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), 
> > > ptr %11, align 8
> > >   %12 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 1
> > >   store ptr %5, ptr %12, align 8
> > >   %13 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 1
> > >   store ptr %5, ptr %13, align 8
> > >   %14 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 1
> > >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), 
> > > ptr %14, align 8
> > >   %15 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 2
> > >   store ptr %6, ptr %15, align 8
> > >   %16 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 2
> > >   store ptr %6, ptr %16, align 8
> > >   %17 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 2
> > >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), 
> > > ptr %17, align 8
> > >   %18 = getelementptr 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 503741.
TIFitis added a comment.

Added names for offload mapper variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- /dev/null
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -0,0 +1,176 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+llvm.func @_QPopenmp_target_data() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
+  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+llvm.store %2, %1 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: %[[VAL_3:.*]] = alloca i32, i64 1, align 4
+// CHECK: br label %[[VAL_4:.*]]
+// CHECK:   entry:; preds = %[[VAL_5:.*]]
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_7]], align 8
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %[[VAL_8]], align 4
+// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_10:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr %[[VAL_11]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: br label %[[VAL_12:.*]]
+// CHECK:   omp.data.region:  ; preds = %[[VAL_4]]
+// CHECK: store i32 99, ptr %[[VAL_3]], align 4
+// CHECK: br label %[[VAL_13:.*]]
+// CHECK:   omp.region.cont:  ; preds = %[[VAL_12]]
+// CHECK: %[[VAL_14:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_15:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_16:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_14]], ptr %[[VAL_15]], ptr %[[VAL_16]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: ret void
+
+// -
+
+llvm.func @_QPopenmp_target_data_region(%1 : !llvm.ptr>) {
+  omp.target_data   map((from -> %1 : !llvm.ptr>)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+%3 = llvm.mlir.constant(1 : i64) : i64
+%4 = llvm.mlir.constant(1 : i64) : i64
+%5 = llvm.mlir.constant(0 : i64) : i64
+%6 = llvm.getelementptr %1[0, %5] : (!llvm.ptr>, i64) -> !llvm.ptr
+llvm.store %2, %6 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 2]
+// CHECK-LABEL: define void @_QPopenmp_target_data_region
+// CHECK: (ptr %[[ARG_0:.*]]) {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: br label %[[VAL_3:.*]]
+// CHECK:   entry:; preds = %[[VAL_4:.*]]
+// CHECK: %[[VAL_5:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked an inline comment as done.
TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,

kiranchandramohan wrote:
> TIFitis wrote:
> > kiranchandramohan wrote:
> > > TIFitis wrote:
> > > > kiranchandramohan wrote:
> > > > > Isn't it possible to sink this whole function into the 
> > > > > OpenMPIRBuilder by passing it a list of `mapOpValue` and 
> > > > > `mapTypeFlags`?
> > > > > `lvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);`
> > > > > 
> > > > > Did i miss something? Or is this in anticipation of more processing 
> > > > > required for other types?
> > > > I'm not fully sure but we might need more MLIR related things when 
> > > > supporting types other than LLVMPointerType. Also there is a call to 
> > > > mlir::LLVM::createMappingInformation.
> > > > 
> > > > I guess it might still be possible to move most of it to the IRBuilder, 
> > > > would you like me to do that?
> > > Callbacks are useful when there is frontend-specific handling that is 
> > > required. If more types require to be handled then it is better to have 
> > > the callback. We can revisit this after all types are handled. I assume, 
> > > the current handling is for scalars and arrays of known-size.
> > I am a novice at FORTRAN so I'm not aware of all  the types and scenarios.
> > 
> > I've tested the following cases and they work end-to-end:
> > 
> > **Fortran:**
> > ```
> > subroutine openmp_target_data_region(a)
> > real :: a(*)
> > integer :: b(1024)
> > character :: c
> > integer, pointer :: p
> > !$omp target enter data map(to: a, b, c, p)
> > end subroutine openmp_target_data_region
> > ```
> > 
> > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o test.ll`** ):**
> > 
> > ```
> > ; ModuleID = 'FIRModule'
> > source_filename = "FIRModule"
> > target datalayout = 
> > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > target triple = "x86_64-unknown-linux-gnu"
> > 
> > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > 
> > @0 = private unnamed_addr constant [13 x i8] c"loc(unknown)\00", align 1
> > @1 = private unnamed_addr constant [56 x i8] 
> > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00", align 1
> > @2 = private unnamed_addr constant [56 x i8] 
> > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00", align 1
> > @3 = private unnamed_addr constant [56 x i8] 
> > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00", align 1
> > @4 = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", 
> > align 1
> > @5 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, 
> > i32 22, ptr @4 }, align 8
> > @.offload_maptypes = private unnamed_addr constant [4 x i64] [i64 1, i64 1, 
> > i64 1, i64 1]
> > @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr @1, ptr @2, 
> > ptr @3]
> > 
> > declare ptr @malloc(i64)
> > 
> > declare void @free(ptr)
> > 
> > define void @openmp_target_data_region_(ptr %0) {
> >   %2 = alloca [4 x ptr], align 8
> >   %3 = alloca [4 x ptr], align 8
> >   %4 = alloca [4 x i64], align 8
> >   %5 = alloca [1024 x i32], i64 1, align 4
> >   %6 = alloca [1 x i8], i64 1, align 1
> >   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
> >   %8 = alloca ptr, i64 1, align 8
> >   store ptr null, ptr %8, align 8
> >   br label %entry
> > 
> > entry:; preds = %1
> >   %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
> >   store ptr %0, ptr %9, align 8
> >   %10 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
> >   store ptr %0, ptr %10, align 8
> >   %11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
> >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr 
> > %11, align 8
> >   %12 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 1
> >   store ptr %5, ptr %12, align 8
> >   %13 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 1
> >   store ptr %5, ptr %13, align 8
> >   %14 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 1
> >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr 
> > %14, align 8
> >   %15 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 2
> >   store ptr %6, ptr %15, align 8
> >   %16 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 2
> >   store ptr %6, ptr %16, align 8
> >   %17 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 2
> >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr 
> > %17, align 8
> >   %18 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 3
> >   store ptr %7, ptr %18, align 8
> >   %19 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 3
> >   store ptr %7, ptr %19, align 8
> >  

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added inline comments.
This revision now requires changes to proceed.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,

TIFitis wrote:
> kiranchandramohan wrote:
> > TIFitis wrote:
> > > kiranchandramohan wrote:
> > > > Isn't it possible to sink this whole function into the OpenMPIRBuilder 
> > > > by passing it a list of `mapOpValue` and `mapTypeFlags`?
> > > > `lvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);`
> > > > 
> > > > Did i miss something? Or is this in anticipation of more processing 
> > > > required for other types?
> > > I'm not fully sure but we might need more MLIR related things when 
> > > supporting types other than LLVMPointerType. Also there is a call to 
> > > mlir::LLVM::createMappingInformation.
> > > 
> > > I guess it might still be possible to move most of it to the IRBuilder, 
> > > would you like me to do that?
> > Callbacks are useful when there is frontend-specific handling that is 
> > required. If more types require to be handled then it is better to have the 
> > callback. We can revisit this after all types are handled. I assume, the 
> > current handling is for scalars and arrays of known-size.
> I am a novice at FORTRAN so I'm not aware of all  the types and scenarios.
> 
> I've tested the following cases and they work end-to-end:
> 
> **Fortran:**
> ```
> subroutine openmp_target_data_region(a)
> real :: a(*)
> integer :: b(1024)
> character :: c
> integer, pointer :: p
> !$omp target enter data map(to: a, b, c, p)
> end subroutine openmp_target_data_region
> ```
> 
> **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o test.ll`** ):**
> 
> ```
> ; ModuleID = 'FIRModule'
> source_filename = "FIRModule"
> target datalayout = 
> "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> target triple = "x86_64-unknown-linux-gnu"
> 
> %struct.ident_t = type { i32, i32, i32, i32, ptr }
> 
> @0 = private unnamed_addr constant [13 x i8] c"loc(unknown)\00", align 1
> @1 = private unnamed_addr constant [56 x i8] 
> c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00", align 1
> @2 = private unnamed_addr constant [56 x i8] 
> c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00", align 1
> @3 = private unnamed_addr constant [56 x i8] 
> c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00", align 1
> @4 = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", 
> align 1
> @5 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 
> 22, ptr @4 }, align 8
> @.offload_maptypes = private unnamed_addr constant [4 x i64] [i64 1, i64 1, 
> i64 1, i64 1]
> @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr @1, ptr @2, ptr 
> @3]
> 
> declare ptr @malloc(i64)
> 
> declare void @free(ptr)
> 
> define void @openmp_target_data_region_(ptr %0) {
>   %2 = alloca [4 x ptr], align 8
>   %3 = alloca [4 x ptr], align 8
>   %4 = alloca [4 x i64], align 8
>   %5 = alloca [1024 x i32], i64 1, align 4
>   %6 = alloca [1 x i8], i64 1, align 1
>   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
>   %8 = alloca ptr, i64 1, align 8
>   store ptr null, ptr %8, align 8
>   br label %entry
> 
> entry:; preds = %1
>   %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
>   store ptr %0, ptr %9, align 8
>   %10 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
>   store ptr %0, ptr %10, align 8
>   %11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
>   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr 
> %11, align 8
>   %12 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 1
>   store ptr %5, ptr %12, align 8
>   %13 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 1
>   store ptr %5, ptr %13, align 8
>   %14 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 1
>   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr 
> %14, align 8
>   %15 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 2
>   store ptr %6, ptr %15, align 8
>   %16 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 2
>   store ptr %6, ptr %16, align 8
>   %17 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 2
>   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr 
> %17, align 8
>   %18 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 3
>   store ptr %7, ptr %18, align 8
>   %19 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 3
>   store ptr %7, ptr %19, align 8
>   %20 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 3
>   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr 
> %20, align 8
>   %21 = 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1561
+  /// should be placed.
+  /// \param HasRegion True if the op has a region associated with it, false
+  /// otherwise

kiranchandramohan wrote:
> TIFitis wrote:
> > kiranchandramohan wrote:
> > > Can't the presence/absence of the BodyGenCB indicate the presence/absence 
> > > of a region?
> > The `BodyGenCB` is always present as an argument right? Passing a `nullptr` 
> > when its not required doesn't seem possible at least when using the 
> > `BodyGenCallbackTy`. Is there a way to selectively pass the `BodyGenCB`?
> The only optional CallBack seems to be the `FinalizeCallbackTy`. It is 
> defined as an `std::function` whereas `BodyGenCallBackTy` is defined as a 
> `function_ref`. But the definition of `function_ref` looks like it can be 
> used to check whether it is a `nullptr` or not. Did you face any issues in 
> trying to make it optional with a default parameter value of `nullptr`?
> 
> https://llvm.org/doxygen/STLFunctionalExtras_8h_source.html#l00036
> 
> ```
>   void emitCancelationCheckImpl(Value *CancelFlag,
> omp::Directive CanceledDirective,
> FinalizeCallbackTy ExitCB = {});
> ```
Sorry, I was being stupid and trying to pass `nullptr` as an argument instead 
of making the actual parameter optional. `BodyGenCallBackTy` works.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4052-4060
+  // LLVM utilities like blocks with terminators.
+  auto *UI = Builder.CreateUnreachable();
+  Instruction *ThenTI = UI, *ElseTI = nullptr;
+  if (IfCond) {
+SplitBlockAndInsertIfThenElse(IfCond, UI, , );
+ThenTI->getParent()->setName("omp_if.then");
+ElseTI->getParent()->setName("omp_if.else");

kiranchandramohan wrote:
> TIFitis wrote:
> > kiranchandramohan wrote:
> > > TIFitis wrote:
> > > > kiranchandramohan wrote:
> > > > > There is some recent recommendation to not insert artificial 
> > > > > instructions and remove them. The preferred approach is to use 
> > > > > `splitBB` function(s) inside the OpenMPIRBuilder. This function works 
> > > > > on blocks without terminators. You can consult the `IfCondition` code 
> > > > > in `createParallel`.
> > > > Thanks a lot for taking the time to review this lengthy patch.
> > > > 
> > > > This one seems a bit tricky to do. At first glance `createParallel` 
> > > > seems to be doing something different where its calling different 
> > > > runtime functions based on the `IfCondition` instead of much in the way 
> > > > of Control Flow changes.
> > > > 
> > > > The `unreachable` inst helps out a lot here as it makes it really easy 
> > > > to keep trace of the resume point for adding instructions after the 
> > > > `BodyGen` codes are generated.
> > > > 
> > > > I am still looking into finding a way to do this elegantly without 
> > > > having to use the `unreachable` instruction, but would it be a major 
> > > > blocker if we had to keep it?
> > > > 
> > > > I have addressed all the other changes you requested.
> > > Thanks for explaining the need for the `unreachable`.  I will leave it 
> > > with you on whether to make the change.
> > > 
> > > You can see an instance of a past request for not using temporary 
> > > instruction. That patch (if for createTask) addressed the issue one way.
> > > https://reviews.llvm.org/D130615#inline-1257711
> > > 
> > > I gave a quick try and came up with the following code. I don't think it 
> > > is very elegant, but it is one way to do it. 
> > > ```
> > > -  // LLVM utilities like blocks with terminators.
> > > -  auto *UI = Builder.CreateUnreachable();
> > > +  BasicBlock *ContBB = nullptr;
> > >if (IfCond) {
> > > -auto *ThenTI =
> > > -SplitBlockAndInsertIfThen(IfCond, UI, /* Unreachable */ false);
> > > -ThenTI->getParent()->setName("omp_if.then");
> > > -Builder.SetInsertPoint(ThenTI);
> > > -  } else {
> > > -Builder.SetInsertPoint(UI);
> > > +BasicBlock *EntryBB = Builder.GetInsertBlock();
> > > +ContBB = splitBB(Builder, /*CreateBranch*/ false);
> > > +BasicBlock *ThenBB =
> > > +BasicBlock::Create(Builder.getContext(), EntryBB->getName() + ".if",
> > > +   ContBB->getParent(), ContBB);
> > > +ContBB->setName(EntryBB->getName() + ".if.end");
> > > +Builder.CreateCondBr(IfCond, ThenBB, ContBB);
> > > +Builder.SetInsertPoint(ThenBB);
> > > +Builder.CreateBr(ContBB);
> > > +Builder.SetInsertPoint(ThenBB->getTerminator());
> > >}
> > >  
> > >ProcessMapOpCB(Builder.saveIP(), Builder.saveIP());
> > > @@ -4087,9 +4090,10 @@ OpenMPIRBuilder::InsertPointTy 
> > > OpenMPIRBuilder::createTargetData(
> > >  emitMapperCall(Builder.saveIP(), beginMapperFunc, srcLocInfo, 
> > > MapTypesArg,
> > > MapNamesArg, MapperAllocas, DeviceID, 
> > > MapTypeFlags.size());
> > >  
> 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 503137.
TIFitis marked 3 inline comments as done.
TIFitis added a comment.

Addressed comments. Rebased.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- /dev/null
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -0,0 +1,176 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+llvm.func @_QPopenmp_target_data() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
+  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+llvm.store %2, %1 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: %[[VAL_3:.*]] = alloca i32, i64 1, align 4
+// CHECK: br label %[[VAL_4:.*]]
+// CHECK:   entry:; preds = %[[VAL_5:.*]]
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_7]], align 8
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %[[VAL_8]], align 4
+// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_10:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr %[[VAL_11]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: br label %[[VAL_12:.*]]
+// CHECK:   omp.data.region:  ; preds = %[[VAL_4]]
+// CHECK: store i32 99, ptr %[[VAL_3]], align 4
+// CHECK: br label %[[VAL_13:.*]]
+// CHECK:   omp.region.cont:  ; preds = %[[VAL_12]]
+// CHECK: %[[VAL_14:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_15:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_16:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_14]], ptr %[[VAL_15]], ptr %[[VAL_16]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: ret void
+
+// -
+
+llvm.func @_QPopenmp_target_data_region(%1 : !llvm.ptr>) {
+  omp.target_data   map((from -> %1 : !llvm.ptr>)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+%3 = llvm.mlir.constant(1 : i64) : i64
+%4 = llvm.mlir.constant(1 : i64) : i64
+%5 = llvm.mlir.constant(0 : i64) : i64
+%6 = llvm.getelementptr %1[0, %5] : (!llvm.ptr>, i64) -> !llvm.ptr
+llvm.store %2, %6 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 2]
+// CHECK-LABEL: define void @_QPopenmp_target_data_region
+// CHECK: (ptr %[[ARG_0:.*]]) {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: br label %[[VAL_3:.*]]
+// CHECK:   entry:; preds = %[[VAL_4:.*]]
+// CHECK: %[[VAL_5:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Thanks for the update and the replies. See comments inline.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1561
+  /// should be placed.
+  /// \param HasRegion True if the op has a region associated with it, false
+  /// otherwise

TIFitis wrote:
> kiranchandramohan wrote:
> > Can't the presence/absence of the BodyGenCB indicate the presence/absence 
> > of a region?
> The `BodyGenCB` is always present as an argument right? Passing a `nullptr` 
> when its not required doesn't seem possible at least when using the 
> `BodyGenCallbackTy`. Is there a way to selectively pass the `BodyGenCB`?
The only optional CallBack seems to be the `FinalizeCallbackTy`. It is defined 
as an `std::function` whereas `BodyGenCallBackTy` is defined as a 
`function_ref`. But the definition of `function_ref` looks like it can be used 
to check whether it is a `nullptr` or not. Did you face any issues in trying to 
make it optional with a default parameter value of `nullptr`?

https://llvm.org/doxygen/STLFunctionalExtras_8h_source.html#l00036

```
  void emitCancelationCheckImpl(Value *CancelFlag,
omp::Directive CanceledDirective,
FinalizeCallbackTy ExitCB = {});
```



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4052-4060
+  // LLVM utilities like blocks with terminators.
+  auto *UI = Builder.CreateUnreachable();
+  Instruction *ThenTI = UI, *ElseTI = nullptr;
+  if (IfCond) {
+SplitBlockAndInsertIfThenElse(IfCond, UI, , );
+ThenTI->getParent()->setName("omp_if.then");
+ElseTI->getParent()->setName("omp_if.else");

TIFitis wrote:
> kiranchandramohan wrote:
> > TIFitis wrote:
> > > kiranchandramohan wrote:
> > > > There is some recent recommendation to not insert artificial 
> > > > instructions and remove them. The preferred approach is to use 
> > > > `splitBB` function(s) inside the OpenMPIRBuilder. This function works 
> > > > on blocks without terminators. You can consult the `IfCondition` code 
> > > > in `createParallel`.
> > > Thanks a lot for taking the time to review this lengthy patch.
> > > 
> > > This one seems a bit tricky to do. At first glance `createParallel` seems 
> > > to be doing something different where its calling different runtime 
> > > functions based on the `IfCondition` instead of much in the way of 
> > > Control Flow changes.
> > > 
> > > The `unreachable` inst helps out a lot here as it makes it really easy to 
> > > keep trace of the resume point for adding instructions after the 
> > > `BodyGen` codes are generated.
> > > 
> > > I am still looking into finding a way to do this elegantly without having 
> > > to use the `unreachable` instruction, but would it be a major blocker if 
> > > we had to keep it?
> > > 
> > > I have addressed all the other changes you requested.
> > Thanks for explaining the need for the `unreachable`.  I will leave it with 
> > you on whether to make the change.
> > 
> > You can see an instance of a past request for not using temporary 
> > instruction. That patch (if for createTask) addressed the issue one way.
> > https://reviews.llvm.org/D130615#inline-1257711
> > 
> > I gave a quick try and came up with the following code. I don't think it is 
> > very elegant, but it is one way to do it. 
> > ```
> > -  // LLVM utilities like blocks with terminators.
> > -  auto *UI = Builder.CreateUnreachable();
> > +  BasicBlock *ContBB = nullptr;
> >if (IfCond) {
> > -auto *ThenTI =
> > -SplitBlockAndInsertIfThen(IfCond, UI, /* Unreachable */ false);
> > -ThenTI->getParent()->setName("omp_if.then");
> > -Builder.SetInsertPoint(ThenTI);
> > -  } else {
> > -Builder.SetInsertPoint(UI);
> > +BasicBlock *EntryBB = Builder.GetInsertBlock();
> > +ContBB = splitBB(Builder, /*CreateBranch*/ false);
> > +BasicBlock *ThenBB =
> > +BasicBlock::Create(Builder.getContext(), EntryBB->getName() + ".if",
> > +   ContBB->getParent(), ContBB);
> > +ContBB->setName(EntryBB->getName() + ".if.end");
> > +Builder.CreateCondBr(IfCond, ThenBB, ContBB);
> > +Builder.SetInsertPoint(ThenBB);
> > +Builder.CreateBr(ContBB);
> > +Builder.SetInsertPoint(ThenBB->getTerminator());
> >}
> >  
> >ProcessMapOpCB(Builder.saveIP(), Builder.saveIP());
> > @@ -4087,9 +4090,10 @@ OpenMPIRBuilder::InsertPointTy 
> > OpenMPIRBuilder::createTargetData(
> >  emitMapperCall(Builder.saveIP(), beginMapperFunc, srcLocInfo, 
> > MapTypesArg,
> > MapNamesArg, MapperAllocas, DeviceID, 
> > MapTypeFlags.size());
> >  
> > +BasicBlock *DataExitBB = splitBB(Builder, true, "target_data.exit");
> >  BodyGenCB(Builder.saveIP(), Builder.saveIP());
> >  
> > -Builder.SetInsertPoint(UI->getParent());
> > +Builder.SetInsertPoint(DataExitBB, DataExitBB->begin());
> >  // Create call to end the data region.

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1561
+  /// should be placed.
+  /// \param HasRegion True if the op has a region associated with it, false
+  /// otherwise

kiranchandramohan wrote:
> Can't the presence/absence of the BodyGenCB indicate the presence/absence of 
> a region?
The `BodyGenCB` is always present as an argument right? Passing a `nullptr` 
when its not required doesn't seem possible at least when using the 
`BodyGenCallbackTy`. Is there a way to selectively pass the `BodyGenCB`?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4052-4060
+  // LLVM utilities like blocks with terminators.
+  auto *UI = Builder.CreateUnreachable();
+  Instruction *ThenTI = UI, *ElseTI = nullptr;
+  if (IfCond) {
+SplitBlockAndInsertIfThenElse(IfCond, UI, , );
+ThenTI->getParent()->setName("omp_if.then");
+ElseTI->getParent()->setName("omp_if.else");

kiranchandramohan wrote:
> TIFitis wrote:
> > kiranchandramohan wrote:
> > > There is some recent recommendation to not insert artificial instructions 
> > > and remove them. The preferred approach is to use `splitBB` function(s) 
> > > inside the OpenMPIRBuilder. This function works on blocks without 
> > > terminators. You can consult the `IfCondition` code in `createParallel`.
> > Thanks a lot for taking the time to review this lengthy patch.
> > 
> > This one seems a bit tricky to do. At first glance `createParallel` seems 
> > to be doing something different where its calling different runtime 
> > functions based on the `IfCondition` instead of much in the way of Control 
> > Flow changes.
> > 
> > The `unreachable` inst helps out a lot here as it makes it really easy to 
> > keep trace of the resume point for adding instructions after the `BodyGen` 
> > codes are generated.
> > 
> > I am still looking into finding a way to do this elegantly without having 
> > to use the `unreachable` instruction, but would it be a major blocker if we 
> > had to keep it?
> > 
> > I have addressed all the other changes you requested.
> Thanks for explaining the need for the `unreachable`.  I will leave it with 
> you on whether to make the change.
> 
> You can see an instance of a past request for not using temporary 
> instruction. That patch (if for createTask) addressed the issue one way.
> https://reviews.llvm.org/D130615#inline-1257711
> 
> I gave a quick try and came up with the following code. I don't think it is 
> very elegant, but it is one way to do it. 
> ```
> -  // LLVM utilities like blocks with terminators.
> -  auto *UI = Builder.CreateUnreachable();
> +  BasicBlock *ContBB = nullptr;
>if (IfCond) {
> -auto *ThenTI =
> -SplitBlockAndInsertIfThen(IfCond, UI, /* Unreachable */ false);
> -ThenTI->getParent()->setName("omp_if.then");
> -Builder.SetInsertPoint(ThenTI);
> -  } else {
> -Builder.SetInsertPoint(UI);
> +BasicBlock *EntryBB = Builder.GetInsertBlock();
> +ContBB = splitBB(Builder, /*CreateBranch*/ false);
> +BasicBlock *ThenBB =
> +BasicBlock::Create(Builder.getContext(), EntryBB->getName() + ".if",
> +   ContBB->getParent(), ContBB);
> +ContBB->setName(EntryBB->getName() + ".if.end");
> +Builder.CreateCondBr(IfCond, ThenBB, ContBB);
> +Builder.SetInsertPoint(ThenBB);
> +Builder.CreateBr(ContBB);
> +Builder.SetInsertPoint(ThenBB->getTerminator());
>}
>  
>ProcessMapOpCB(Builder.saveIP(), Builder.saveIP());
> @@ -4087,9 +4090,10 @@ OpenMPIRBuilder::InsertPointTy 
> OpenMPIRBuilder::createTargetData(
>  emitMapperCall(Builder.saveIP(), beginMapperFunc, srcLocInfo, 
> MapTypesArg,
> MapNamesArg, MapperAllocas, DeviceID, 
> MapTypeFlags.size());
>  
> +BasicBlock *DataExitBB = splitBB(Builder, true, "target_data.exit");
>  BodyGenCB(Builder.saveIP(), Builder.saveIP());
>  
> -Builder.SetInsertPoint(UI->getParent());
> +Builder.SetInsertPoint(DataExitBB, DataExitBB->begin());
>  // Create call to end the data region.
>  emitMapperCall(Builder.saveIP(), endMapperFunc, srcLocInfo, MapTypesArg,
> MapNamesArg, MapperAllocas, DeviceID, 
> MapTypeFlags.size());
> @@ -4100,11 +4104,9 @@ OpenMPIRBuilder::InsertPointTy 
> OpenMPIRBuilder::createTargetData(
> MapTypeFlags.size());
>}
>  
> -  // Update the insertion point and remove the terminator we introduced.
> -  Builder.SetInsertPoint(UI->getParent());
> -  if (IfCond)
> -UI->getParent()->setName("omp_if.end");
> -  UI->eraseFromParent();
> +  if (ContBB)
> +return {ContBB, ContBB->begin()};
> +
> ```
I saw the other directives were making use of an explicit `ExitBB`, but for 
those directives they always needed to splice the directive into multiple BB's 
anyways.

Since for target data we don't need to splice the BB unless using the `if` 
clause or `target region` are present, I was 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 502670.
TIFitis marked 9 inline comments as done.
TIFitis added a comment.

Adrresed reviewer comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- /dev/null
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -0,0 +1,176 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+llvm.func @_QPopenmp_target_data() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
+  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+llvm.store %2, %1 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: %[[VAL_3:.*]] = alloca i32, i64 1, align 4
+// CHECK: br label %[[VAL_4:.*]]
+// CHECK:   entry:; preds = %[[VAL_5:.*]]
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_7]], align 8
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %[[VAL_8]], align 4
+// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_10:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr %[[VAL_11]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: br label %[[VAL_12:.*]]
+// CHECK:   omp.data.region:  ; preds = %[[VAL_4]]
+// CHECK: store i32 99, ptr %[[VAL_3]], align 4
+// CHECK: br label %[[VAL_13:.*]]
+// CHECK:   omp.region.cont:  ; preds = %[[VAL_12]]
+// CHECK: %[[VAL_14:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_15:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_16:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_14]], ptr %[[VAL_15]], ptr %[[VAL_16]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: ret void
+
+// -
+
+llvm.func @_QPopenmp_target_data_region(%1 : !llvm.ptr>) {
+  omp.target_data   map((from -> %1 : !llvm.ptr>)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+%3 = llvm.mlir.constant(1 : i64) : i64
+%4 = llvm.mlir.constant(1 : i64) : i64
+%5 = llvm.mlir.constant(0 : i64) : i64
+%6 = llvm.getelementptr %1[0, %5] : (!llvm.ptr>, i64) -> !llvm.ptr
+llvm.store %2, %6 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 2]
+// CHECK-LABEL: define void @_QPopenmp_target_data_region
+// CHECK: (ptr %[[ARG_0:.*]]) {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: br label %[[VAL_3:.*]]
+// CHECK:   entry:; preds = %[[VAL_4:.*]]
+// CHECK: %[[VAL_5:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

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

I have some more comments.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1556
 
+  /// Generator for '#omp target data'
+  ///

Nit: In the following comments either use full stops for all.

It will be helpful if the comments below refer to the OpenMP construct or 
clauses rather than the MLIR representation. Forr e.g Map Clause instead of Map 
operand.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1561
+  /// should be placed.
+  /// \param HasRegion True if the op has a region associated with it, false
+  /// otherwise

Can't the presence/absence of the BodyGenCB indicate the presence/absence of a 
region?



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1567
+  /// \param MapperAllocas Pointers to the AllocInsts for the map clause
+  /// \param MapperFunc Mapper Fucntion to be called for the Target Data op
+  /// \param DeviceID Stores the DeviceID from the device clause

This should be expanded to say whether it means `begin` or `end` mapper based 
on the value of the boolean and probably also renamed to `isBegin` or `isEnter` 
or something like that.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4052-4060
+  // LLVM utilities like blocks with terminators.
+  auto *UI = Builder.CreateUnreachable();
+  Instruction *ThenTI = UI, *ElseTI = nullptr;
+  if (IfCond) {
+SplitBlockAndInsertIfThenElse(IfCond, UI, , );
+ThenTI->getParent()->setName("omp_if.then");
+ElseTI->getParent()->setName("omp_if.else");

TIFitis wrote:
> kiranchandramohan wrote:
> > There is some recent recommendation to not insert artificial instructions 
> > and remove them. The preferred approach is to use `splitBB` function(s) 
> > inside the OpenMPIRBuilder. This function works on blocks without 
> > terminators. You can consult the `IfCondition` code in `createParallel`.
> Thanks a lot for taking the time to review this lengthy patch.
> 
> This one seems a bit tricky to do. At first glance `createParallel` seems to 
> be doing something different where its calling different runtime functions 
> based on the `IfCondition` instead of much in the way of Control Flow changes.
> 
> The `unreachable` inst helps out a lot here as it makes it really easy to 
> keep trace of the resume point for adding instructions after the `BodyGen` 
> codes are generated.
> 
> I am still looking into finding a way to do this elegantly without having to 
> use the `unreachable` instruction, but would it be a major blocker if we had 
> to keep it?
> 
> I have addressed all the other changes you requested.
Thanks for explaining the need for the `unreachable`.  I will leave it with you 
on whether to make the change.

You can see an instance of a past request for not using temporary instruction. 
That patch (if for createTask) addressed the issue one way.
https://reviews.llvm.org/D130615#inline-1257711

I gave a quick try and came up with the following code. I don't think it is 
very elegant, but it is one way to do it. 
```
-  // LLVM utilities like blocks with terminators.
-  auto *UI = Builder.CreateUnreachable();
+  BasicBlock *ContBB = nullptr;
   if (IfCond) {
-auto *ThenTI =
-SplitBlockAndInsertIfThen(IfCond, UI, /* Unreachable */ false);
-ThenTI->getParent()->setName("omp_if.then");
-Builder.SetInsertPoint(ThenTI);
-  } else {
-Builder.SetInsertPoint(UI);
+BasicBlock *EntryBB = Builder.GetInsertBlock();
+ContBB = splitBB(Builder, /*CreateBranch*/ false);
+BasicBlock *ThenBB =
+BasicBlock::Create(Builder.getContext(), EntryBB->getName() + ".if",
+   ContBB->getParent(), ContBB);
+ContBB->setName(EntryBB->getName() + ".if.end");
+Builder.CreateCondBr(IfCond, ThenBB, ContBB);
+Builder.SetInsertPoint(ThenBB);
+Builder.CreateBr(ContBB);
+Builder.SetInsertPoint(ThenBB->getTerminator());
   }
 
   ProcessMapOpCB(Builder.saveIP(), Builder.saveIP());
@@ -4087,9 +4090,10 @@ OpenMPIRBuilder::InsertPointTy 
OpenMPIRBuilder::createTargetData(
 emitMapperCall(Builder.saveIP(), beginMapperFunc, srcLocInfo, MapTypesArg,
MapNamesArg, MapperAllocas, DeviceID, MapTypeFlags.size());
 
+BasicBlock *DataExitBB = splitBB(Builder, true, "target_data.exit");
 BodyGenCB(Builder.saveIP(), Builder.saveIP());
 
-Builder.SetInsertPoint(UI->getParent());
+Builder.SetInsertPoint(DataExitBB, DataExitBB->begin());
 // Create call to end the data region.
 emitMapperCall(Builder.saveIP(), endMapperFunc, srcLocInfo, MapTypesArg,
MapNamesArg, MapperAllocas, DeviceID, MapTypeFlags.size());
@@ -4100,11 +4104,9 @@ OpenMPIRBuilder::InsertPointTy 
OpenMPIRBuilder::createTargetData(

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added inline comments.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4052-4060
+  // LLVM utilities like blocks with terminators.
+  auto *UI = Builder.CreateUnreachable();
+  Instruction *ThenTI = UI, *ElseTI = nullptr;
+  if (IfCond) {
+SplitBlockAndInsertIfThenElse(IfCond, UI, , );
+ThenTI->getParent()->setName("omp_if.then");
+ElseTI->getParent()->setName("omp_if.else");

kiranchandramohan wrote:
> There is some recent recommendation to not insert artificial instructions and 
> remove them. The preferred approach is to use `splitBB` function(s) inside 
> the OpenMPIRBuilder. This function works on blocks without terminators. You 
> can consult the `IfCondition` code in `createParallel`.
Thanks a lot for taking the time to review this lengthy patch.

This one seems a bit tricky to do. At first glance `createParallel` seems to be 
doing something different where its calling different runtime functions based 
on the `IfCondition` instead of much in the way of Control Flow changes.

The `unreachable` inst helps out a lot here as it makes it really easy to keep 
trace of the resume point for adding instructions after the `BodyGen` codes are 
generated.

I am still looking into finding a way to do this elegantly without having to 
use the `unreachable` instruction, but would it be a major blocker if we had to 
keep it?

I have addressed all the other changes you requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 502175.
TIFitis marked 7 inline comments as done.
TIFitis added a comment.

Addressed reviewer comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- /dev/null
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -0,0 +1,205 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+llvm.func @_QPopenmp_target_data() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
+  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+llvm.store %2, %1 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: %[[VAL_3:.*]] = alloca i32, i64 1, align 4
+// CHECK: br label %[[VAL_4:.*]]
+// CHECK:   entry:; preds = %[[VAL_5:.*]]
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_7]], align 8
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %[[VAL_8]], align 4
+// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_10:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr %[[VAL_11]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: br label %[[VAL_12:.*]]
+// CHECK:   omp.data.region:  ; preds = %[[VAL_4]]
+// CHECK: store i32 99, ptr %[[VAL_3]], align 4
+// CHECK: br label %[[VAL_13:.*]]
+// CHECK:   omp.region.cont:  ; preds = %[[VAL_12]]
+// CHECK: %[[VAL_14:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_15:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_16:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_14]], ptr %[[VAL_15]], ptr %[[VAL_16]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: ret void
+
+// -
+
+llvm.func @_QPopenmp_target_data_loop(%1 : !llvm.ptr>) {
+  %2 = llvm.mlir.constant(1 : i64) : i64
+  %3 = llvm.alloca %2 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_data_loopEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((from -> %1 : !llvm.ptr>)) {
+%4 = llvm.mlir.constant(1 : i32) : i32
+%5 = llvm.sext %4 : i32 to i64
+%6 = llvm.mlir.constant(1024 : i32) : i32
+%7 = llvm.sext %6 : i32 to i64
+%8 = llvm.mlir.constant(1 : index) : i64
+%9 = llvm.trunc %5 : i64 to i32
+%10 = llvm.sub %7, %5  : i64
+%11 = llvm.add %10, %8  : i64
+llvm.br ^bb1(%5, %9, %11 : i64, i32, i64)
+  ^bb1(%12: i64, %13: i32, %14: i64):  // 2 preds: ^bb0, ^bb2
+%15 = llvm.mlir.constant(0 : index) : i64
+%16 = llvm.icmp "sgt" %14, %15 : i64
+llvm.cond_br %16, ^bb2, ^bb3
+  ^bb2:  // pred: ^bb1
+llvm.store %13, %3 : !llvm.ptr
+%17 = llvm.load %3 : !llvm.ptr
+%18 = llvm.load %3 : !llvm.ptr
+%19 = llvm.sext %18 : i32 to i64
+%20 = 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

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

Thanks for making the change.
I am still going through the patch, I have a few comments.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1574-1575
+  const LocationDescription , OpenMPIRBuilder::InsertPointTy CodeGenIP,
+  bool HasRegion, SmallVector ,
+  SmallVector , struct MapperAllocas ,
+  Function *MapperFunc, int64_t DeviceID, Value *IfCond,

SmallVector -> SmallVectorImpl 
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4052-4060
+  // LLVM utilities like blocks with terminators.
+  auto *UI = Builder.CreateUnreachable();
+  Instruction *ThenTI = UI, *ElseTI = nullptr;
+  if (IfCond) {
+SplitBlockAndInsertIfThenElse(IfCond, UI, , );
+ThenTI->getParent()->setName("omp_if.then");
+ElseTI->getParent()->setName("omp_if.else");

There is some recent recommendation to not insert artificial instructions and 
remove them. The preferred approach is to use `splitBB` function(s) inside the 
OpenMPIRBuilder. This function works on blocks without terminators. You can 
consult the `IfCondition` code in `createParallel`.



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4983
+  CallInst *TargetDataCall = dyn_cast(>back());
+  BB->dump();
+  EXPECT_NE(TargetDataCall, nullptr);

Is this a debugging leftover?



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4990
+  EXPECT_TRUE(TargetDataCall->getOperand(2)->getType()->isIntegerTy(32));
+  EXPECT_TRUE(TargetDataCall->getOperand(8)->getType()->isPointerTy());
+}

Call verifyModule to ensure the IR is well formed.



Comment at: mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h:13-14
+
+#ifndef MLIR_TARGET_LLVMIR_DIALECT_UTILS_H
+#define MLIR_TARGET_LLVMIR_DIALECT_UTILS_H
+

Nit: The header guards would need changing.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1471
+mapTypes = enterDataOp.getMapTypes();
+mapperFunc = ompBuilder->getOrCreateRuntimeFunctionPtr(
+llvm::omp::OMPRTL___tgt_target_data_begin_mapper);

Ideally we would not want to create an OpenMP runtime calls in the Translation. 
This is the job of the OpenMPIRbuilder. I would recommend passing a boolean to 
the OpenMPIRBuilder and let the IRBuilder pick up the runtime function.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1492
+mapTypes = exitDataOp.getMapTypes();
+mapperFunc = ompBuilder->getOrCreateRuntimeFunctionPtr(
+llvm::omp::OMPRTL___tgt_target_data_end_mapper);

Same comment as above.



Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:1
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+

In MLIR it is preferred to test the minimal set of functionalities. So you will 
have to minimize the CHECK lines and write them manually. Focus on Checking the 
runtime call and any important IR inserted by the IRBuilder. Keep the contents 
of the region to a minimum.
https://mlir.llvm.org/getting_started/TestingGuide/#filecheck-best-practices


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added a comment.

@kiranchandramohan I've added the MLIR test as requested. Please let me know if 
any other changes are required :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 501842.
TIFitis added a comment.

Rebased


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- /dev/null
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -0,0 +1,228 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+llvm.func @_QPopenmp_target_data() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
+  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+llvm.store %2, %1 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: %[[VAL_3:.*]] = alloca i32, i64 1, align 4
+// CHECK: br label %[[VAL_4:.*]]
+// CHECK:   entry:; preds = %[[VAL_5:.*]]
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_7]], align 8
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %[[VAL_8]], align 4
+// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_10:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr %[[VAL_11]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: br label %[[VAL_12:.*]]
+// CHECK:   omp.data.region:  ; preds = %[[VAL_4]]
+// CHECK: store i32 99, ptr %[[VAL_3]], align 4
+// CHECK: br label %[[VAL_13:.*]]
+// CHECK:   omp.region.cont:  ; preds = %[[VAL_12]]
+// CHECK: %[[VAL_14:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_15:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_16:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_14]], ptr %[[VAL_15]], ptr %[[VAL_16]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: ret void
+
+// -
+
+llvm.func @_QPopenmp_target_data_loop(%1 : !llvm.ptr>) {
+  %2 = llvm.mlir.constant(1 : i64) : i64
+  %3 = llvm.alloca %2 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_data_loopEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((from -> %1 : !llvm.ptr>)) {
+%4 = llvm.mlir.constant(1 : i32) : i32
+%5 = llvm.sext %4 : i32 to i64
+%6 = llvm.mlir.constant(1024 : i32) : i32
+%7 = llvm.sext %6 : i32 to i64
+%8 = llvm.mlir.constant(1 : index) : i64
+%9 = llvm.trunc %5 : i64 to i32
+%10 = llvm.sub %7, %5  : i64
+%11 = llvm.add %10, %8  : i64
+llvm.br ^bb1(%5, %9, %11 : i64, i32, i64)
+  ^bb1(%12: i64, %13: i32, %14: i64):  // 2 preds: ^bb0, ^bb2
+%15 = llvm.mlir.constant(0 : index) : i64
+%16 = llvm.icmp "sgt" %14, %15 : i64
+llvm.cond_br %16, ^bb2, ^bb3
+  ^bb2:  // pred: ^bb1
+llvm.store %13, %3 : !llvm.ptr
+%17 = llvm.load %3 : !llvm.ptr
+%18 = llvm.load %3 : !llvm.ptr
+%19 = llvm.sext %18 : i32 to i64
+%20 = llvm.mlir.constant(1 : i64) : i64
+%21 = llvm.sub %19, %20  : 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 500807.
TIFitis added a comment.

Added MLIR test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- /dev/null
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -0,0 +1,228 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+llvm.func @_QPopenmp_target_data() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
+  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
+%2 = llvm.mlir.constant(99 : i32) : i32
+llvm.store %2, %1 : !llvm.ptr
+omp.terminator
+  }
+  llvm.return
+}
+
+// CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_1:.*]] = alloca [1 x ptr], align 8
+// CHECK: %[[VAL_2:.*]] = alloca [1 x i64], align 8
+// CHECK: %[[VAL_3:.*]] = alloca i32, i64 1, align 4
+// CHECK: br label %[[VAL_4:.*]]
+// CHECK:   entry:; preds = %[[VAL_5:.*]]
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_7]], align 8
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %[[VAL_8]], align 4
+// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_10:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr %[[VAL_11]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: br label %[[VAL_12:.*]]
+// CHECK:   omp.data.region:  ; preds = %[[VAL_4]]
+// CHECK: store i32 99, ptr %[[VAL_3]], align 4
+// CHECK: br label %[[VAL_13:.*]]
+// CHECK:   omp.region.cont:  ; preds = %[[VAL_12]]
+// CHECK: %[[VAL_14:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_15:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: %[[VAL_16:.*]] = getelementptr inbounds [1 x i64], ptr %[[VAL_2]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_14]], ptr %[[VAL_15]], ptr %[[VAL_16]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: ret void
+
+// -
+
+llvm.func @_QPopenmp_target_data_loop(%1 : !llvm.ptr>) {
+  %2 = llvm.mlir.constant(1 : i64) : i64
+  %3 = llvm.alloca %2 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array, uniq_name = "_QFopenmp_target_data_loopEi"} : (i64) -> !llvm.ptr
+  omp.target_data   map((from -> %1 : !llvm.ptr>)) {
+%4 = llvm.mlir.constant(1 : i32) : i32
+%5 = llvm.sext %4 : i32 to i64
+%6 = llvm.mlir.constant(1024 : i32) : i32
+%7 = llvm.sext %6 : i32 to i64
+%8 = llvm.mlir.constant(1 : index) : i64
+%9 = llvm.trunc %5 : i64 to i32
+%10 = llvm.sub %7, %5  : i64
+%11 = llvm.add %10, %8  : i64
+llvm.br ^bb1(%5, %9, %11 : i64, i32, i64)
+  ^bb1(%12: i64, %13: i32, %14: i64):  // 2 preds: ^bb0, ^bb2
+%15 = llvm.mlir.constant(0 : index) : i64
+%16 = llvm.icmp "sgt" %14, %15 : i64
+llvm.cond_br %16, ^bb2, ^bb3
+  ^bb2:  // pred: ^bb1
+llvm.store %13, %3 : !llvm.ptr
+%17 = llvm.load %3 : !llvm.ptr
+%18 = llvm.load %3 : !llvm.ptr
+%19 = llvm.sext %18 : i32 to i64
+%20 = llvm.mlir.constant(1 : i64) : i64
+%21 = llvm.sub %19, 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D142914#4145224 , @TIFitis wrote:

> In D142914#4144475 , 
> @kiranchandramohan wrote:
>
>> Please add tests for the MLIR portion.
>
> Can you please tell me where to add these?

In `mlir/test/Target/LLVMIR/openmp-llvm.mlir`. You can also consider starting a 
new file for target constructs in that directory.




Comment at: mlir/lib/Target/LLVMIR/Dialect/Utils.cpp:1
+//===- Utils.cpp - General Utils for translating MLIR dialect to LLVM 
IR---===//
+//

TIFitis wrote:
> kiranchandramohan wrote:
> > Nit: I would prefer the OpenMPCommon.cpp name suggested in 
> > https://discourse.llvm.org/t/rfc-adding-new-util-file-to-mlir-dialect-translation/68221.
> >  
> Would you also like me to move the file inside OpenMP ( 
> `mlir/lib/Target/LLVMIR/Dialect/OpenMP` ) ?
No, the current location is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 499583.
TIFitis marked an inline comment as done.
TIFitis added a comment.

Addressed reviewer comments, fixed error in MLIR translation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp

Index: mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
===
--- /dev/null
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMPCommon.cpp
@@ -0,0 +1,41 @@
+//===- OpenMPCommon.cpp - Utils for translating MLIR dialect to LLVM IR===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines general utilities for MLIR Dialect translations to LLVM IR.
+//
+//===--===//
+
+#include "mlir/Target/LLVMIR/Dialect/OpenMPCommon.h"
+
+llvm::Constant *
+mlir::LLVM::createSourceLocStrFromLocation(Location loc,
+   llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();
+unsigned colNo = fileLoc.getColumn();
+return builder.getOrCreateSrcLocStr(name, fileName, lineNo, colNo, strLen);
+  }
+  std::string locStr;
+  llvm::raw_string_ostream locOS(locStr);
+  locOS << loc;
+  return builder.getOrCreateSrcLocStr(locOS.str(), strLen);
+}
+
+llvm::Constant *
+mlir::LLVM::createMappingInformation(Location loc,
+ llvm::OpenMPIRBuilder ) {
+  uint32_t strLen;
+  if (auto nameLoc = loc.dyn_cast()) {
+StringRef name = nameLoc.getName();
+return createSourceLocStrFromLocation(nameLoc.getChildLoc(), builder, name,
+  strLen);
+  }
+  return createSourceLocStrFromLocation(loc, builder, "unknown", strLen);
+}
Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -15,10 +15,12 @@
 #include "mlir/IR/IRMapping.h"
 #include "mlir/IR/Operation.h"
 #include "mlir/Support/LLVM.h"
+#include "mlir/Target/LLVMIR/Dialect/OpenMPCommon.h"
 #include "mlir/Target/LLVMIR/ModuleTranslation.h"
 
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/IRBuilder.h"
@@ -1351,6 +1353,191 @@
   return success();
 }
 
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,
+const SmallVector , const ArrayAttr ,
+SmallVector ,
+SmallVectorImpl ,
+struct llvm::OpenMPIRBuilder::MapperAllocas ) {
+  auto numMapOperands = mapOperands.size();
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+  llvm::PointerType *i8PtrTy = builder.getInt8PtrTy();
+  llvm::ArrayType *arrI8PtrTy = llvm::ArrayType::get(i8PtrTy, numMapOperands);
+  llvm::IntegerType *i64Ty = builder.getInt64Ty();
+  llvm::ArrayType *arrI64Ty = llvm::ArrayType::get(i64Ty, numMapOperands);
+
+  unsigned index = 0;
+  for (const auto  : mapOperands) {
+const auto  = mapTypes[index];
+
+llvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);
+llvm::Value *mapOpPtrBase;
+llvm::Value *mapOpPtr;
+llvm::Value *mapOpSize;
+
+if (mapOp.getType().isa()) {
+  mapOpPtrBase = mapOpValue;
+  mapOpPtr = mapOpValue;
+  mapOpSize = ompBuilder->getSizeInBytes(mapOpValue);
+} else {
+  return failure();
+}
+
+// Store base pointer extracted from operand into the i-th position of
+// argBase.
+llvm::Value *ptrBaseGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.ArgsBase,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrBaseCast = builder.CreateBitCast(
+

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

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

In D142914#4144475 , 
@kiranchandramohan wrote:

> Please add tests for the MLIR portion.

Can you please tell me where to add these?

> Could you also post the full LLVM IR for a construct with the map clause?

The following is a simple example, let me know if you'd like a more complex 
example with loop :)

**Flang:**

  subroutine openmp_target_data
  integer :: i
  !$omp target data map(tofrom: i)
  i = 99;
  !$omp end target data
  end subroutine openmp_target_data

**FIR:**

  func.func @_QPopenmp_target_data() {
%0 = fir.alloca i32 {bindc_name = "i", uniq_name = 
"_QFopenmp_target_dataEi"}
omp.target_data   map((tofrom -> %0 : !fir.ref)) {
  %c99_i32 = arith.constant 99 : i32
  fir.store %c99_i32 to %0 : !fir.ref
  omp.terminator
}
return
  }

**LLVMIR:**

  llvm.func @_QPopenmp_target_data() {
%0 = llvm.mlir.constant(1 : i64) : i64
%1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, 
operand_segment_sizes = array, uniq_name = 
"_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
  %2 = llvm.mlir.constant(99 : i32) : i32
  llvm.store %2, %1 : !llvm.ptr
  omp.terminator
}
llvm.return
  }

**llvm IR .ll:**

  ; ModuleID = 'LLVMDialectModule'
  source_filename = "LLVMDialectModule"
  target datalayout = 
"e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
  target triple = "aarch64-unknown-linux-gnu"
  
  %struct.ident_t = type { i32, i32, i32, i32, ptr }
  
  @0 = private unnamed_addr constant [26 x i8] c";test.mlir;unknown;4;10;;\00", 
align 1
  @1 = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", 
align 1
  @2 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 
22, ptr @1 }, align 8
  @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
  @.offload_mapnames = private constant [1 x ptr] [ptr @0]
  
  declare ptr @malloc(i64)
  
  declare void @free(ptr)
  
  define void @_QPopenmp_target_data() {
%1 = alloca [1 x ptr], align 8
%2 = alloca [1 x ptr], align 8
%3 = alloca [1 x i64], align 8
%4 = alloca i32, i64 1, align 4
br label %entry
  
  entry:; preds = %0
%5 = getelementptr inbounds [1 x ptr], ptr %1, i32 0, i32 0
store ptr %4, ptr %5, align 8
%6 = getelementptr inbounds [1 x ptr], ptr %2, i32 0, i32 0
store ptr %4, ptr %6, align 8
%7 = getelementptr inbounds [1 x i64], ptr %3, i32 0, i32 0
store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr 
%7, align 8
%8 = getelementptr inbounds [1 x ptr], ptr %1, i32 0, i32 0
%9 = getelementptr inbounds [1 x ptr], ptr %2, i32 0, i32 0
%10 = getelementptr inbounds [1 x i64], ptr %3, i32 0, i32 0
call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %8, 
ptr %9, ptr %10, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
br label %omp.data.region
  
  omp.data.region:  ; preds = %entry
store i32 99, ptr %4, align 4
br label %omp.region.cont
  
  omp.region.cont:  ; preds = %omp.data.region
%11 = getelementptr inbounds [1 x ptr], ptr %1, i32 0, i32 0
%12 = getelementptr inbounds [1 x ptr], ptr %2, i32 0, i32 0
%13 = getelementptr inbounds [1 x i64], ptr %3, i32 0, i32 0
call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %11, ptr 
%12, ptr %13, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
ret void
  }
  
  ; Function Attrs: nounwind
  declare void @__tgt_target_data_begin_mapper(ptr, i64, i32, ptr, ptr, ptr, 
ptr, ptr, ptr) #0
  
  ; Function Attrs: nounwind
  declare void @__tgt_target_data_end_mapper(ptr, i64, i32, ptr, ptr, ptr, ptr, 
ptr, ptr) #0
  
  attributes #0 = { nounwind }
  
  !llvm.module.flags = !{!0}
  
  !0 = !{i32 2, !"Debug Info Version", i32 3}

Cheers,
Akash




Comment at: mlir/lib/Target/LLVMIR/Dialect/Utils.cpp:1
+//===- Utils.cpp - General Utils for translating MLIR dialect to LLVM 
IR---===//
+//

kiranchandramohan wrote:
> Nit: I would prefer the OpenMPCommon.cpp name suggested in 
> https://discourse.llvm.org/t/rfc-adding-new-util-file-to-mlir-dialect-translation/68221.
>  
Would you also like me to move the file inside OpenMP ( 
`mlir/lib/Target/LLVMIR/Dialect/OpenMP` ) ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

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

Please add tests for the MLIR portion.

Could you also post the full LLVM IR for a construct with the map clause?




Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4157
+Value *OpenMPIRBuilder::getSizeInBytes(Value *BasePtr) {
+
+  LLVMContext  = Builder.getContext();

Nit:Remove empty line.



Comment at: mlir/lib/Target/LLVMIR/Dialect/Utils.cpp:1
+//===- Utils.cpp - General Utils for translating MLIR dialect to LLVM 
IR---===//
+//

Nit: I would prefer the OpenMPCommon.cpp name suggested in 
https://discourse.llvm.org/t/rfc-adding-new-util-file-to-mlir-dialect-translation/68221.
 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added a comment.

@kiranchandramohan @clementval can you please take a look and let me know if 
any more changes are required for this patch?

Thanks,
Akash


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Raghu via Phabricator via cfe-commits]

raghavendhra accepted this revision.
raghavendhra added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 497045.
TIFitis added a comment.

Fixing build errors. Added mlir/lib/Target/LLVMIR/Dialect/Utils.cpp, made it 
part of the MLIRTargetLLVMIRExport lib.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h
  mlir/lib/Target/LLVMIR/CMakeLists.txt
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/Utils.cpp

Index: mlir/lib/Target/LLVMIR/Dialect/Utils.cpp
===
--- /dev/null
+++ mlir/lib/Target/LLVMIR/Dialect/Utils.cpp
@@ -0,0 +1,41 @@
+//===- Utils.cpp - General Utils for translating MLIR dialect to LLVM IR---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines general utilities for MLIR Dialect translations to LLVM IR.
+//
+//===--===//
+
+#include "mlir/Target/LLVMIR/Dialect/Utils.h"
+
+llvm::Constant *
+mlir::LLVM::createSourceLocStrFromLocation(Location loc,
+   llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();
+unsigned colNo = fileLoc.getColumn();
+return builder.getOrCreateSrcLocStr(name, fileName, lineNo, colNo, strLen);
+  }
+  std::string locStr;
+  llvm::raw_string_ostream locOS(locStr);
+  locOS << loc;
+  return builder.getOrCreateSrcLocStr(locOS.str(), strLen);
+}
+
+llvm::Constant *
+mlir::LLVM::createMappingInformation(Location loc,
+ llvm::OpenMPIRBuilder ) {
+  uint32_t strLen;
+  if (auto nameLoc = loc.dyn_cast()) {
+StringRef name = nameLoc.getName();
+return createSourceLocStrFromLocation(nameLoc.getChildLoc(), builder, name,
+  strLen);
+  }
+  return createSourceLocStrFromLocation(loc, builder, "unknown", strLen);
+}
Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -15,10 +15,12 @@
 #include "mlir/IR/IRMapping.h"
 #include "mlir/IR/Operation.h"
 #include "mlir/Support/LLVM.h"
+#include "mlir/Target/LLVMIR/Dialect/Utils.h"
 #include "mlir/Target/LLVMIR/ModuleTranslation.h"
 
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/IRBuilder.h"
@@ -1325,6 +1327,191 @@
   return success();
 }
 
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,
+const SmallVector , const ArrayAttr ,
+SmallVector ,
+SmallVectorImpl ,
+struct llvm::OpenMPIRBuilder::MapperAllocas ) {
+  auto numMapOperands = mapOperands.size();
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+  llvm::PointerType *i8PtrTy = builder.getInt8PtrTy();
+  llvm::ArrayType *arrI8PtrTy = llvm::ArrayType::get(i8PtrTy, numMapOperands);
+  llvm::IntegerType *i64Ty = builder.getInt64Ty();
+  llvm::ArrayType *arrI64Ty = llvm::ArrayType::get(i64Ty, numMapOperands);
+
+  unsigned index = 0;
+  for (const auto  : mapOperands) {
+const auto  = mapTypes[index];
+
+llvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);
+llvm::Value *mapOpPtrBase;
+llvm::Value *mapOpPtr;
+llvm::Value *mapOpSize;
+
+if (mapOp.getType().isa()) {
+  mapOpPtrBase = mapOpValue;
+  mapOpPtr = mapOpValue;
+  mapOpSize = ompBuilder->getSizeInBytes(mapOpValue);
+} else {
+  return failure();
+}
+
+// Store base pointer extracted from operand into the i-th position of
+// argBase.
+llvm::Value *ptrBaseGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.ArgsBase,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrBaseCast = builder.CreateBitCast(
+ptrBaseGEP, 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 495864.
TIFitis marked 5 inline comments as done.
TIFitis added a comment.

Addressed reviewer comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -15,10 +15,12 @@
 #include "mlir/IR/IRMapping.h"
 #include "mlir/IR/Operation.h"
 #include "mlir/Support/LLVM.h"
+#include "mlir/Target/LLVMIR/Dialect/Utils.h"
 #include "mlir/Target/LLVMIR/ModuleTranslation.h"
 
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/IRBuilder.h"
@@ -1338,6 +1340,191 @@
   return success();
 }
 
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,
+const SmallVector , const ArrayAttr ,
+SmallVector ,
+SmallVectorImpl ,
+struct llvm::OpenMPIRBuilder::MapperAllocas ) {
+  auto numMapOperands = mapOperands.size();
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+  llvm::PointerType *i8PtrTy = builder.getInt8PtrTy();
+  llvm::ArrayType *arrI8PtrTy = llvm::ArrayType::get(i8PtrTy, numMapOperands);
+  llvm::IntegerType *i64Ty = builder.getInt64Ty();
+  llvm::ArrayType *arrI64Ty = llvm::ArrayType::get(i64Ty, numMapOperands);
+
+  unsigned index = 0;
+  for (const auto  : mapOperands) {
+const auto  = mapTypes[index];
+
+llvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);
+llvm::Value *mapOpPtrBase;
+llvm::Value *mapOpPtr;
+llvm::Value *mapOpSize;
+
+if (mapOp.getType().isa()) {
+  mapOpPtrBase = mapOpValue;
+  mapOpPtr = mapOpValue;
+  mapOpSize = ompBuilder->getSizeInBytes(mapOpValue);
+} else {
+  return failure();
+}
+
+// Store base pointer extracted from operand into the i-th position of
+// argBase.
+llvm::Value *ptrBaseGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.ArgsBase,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrBaseCast = builder.CreateBitCast(
+ptrBaseGEP, mapOpPtrBase->getType()->getPointerTo());
+builder.CreateStore(mapOpPtrBase, ptrBaseCast);
+
+// Store pointer extracted from operand into the i-th position of args.
+llvm::Value *ptrGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.Args,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrCast =
+builder.CreateBitCast(ptrGEP, mapOpPtr->getType()->getPointerTo());
+builder.CreateStore(mapOpPtr, ptrCast);
+
+// Store size extracted from operand into the i-th position of argSizes.
+llvm::Value *sizeGEP = builder.CreateInBoundsGEP(
+arrI64Ty, mapperAllocas.ArgSizes,
+{builder.getInt32(0), builder.getInt32(index)});
+builder.CreateStore(mapOpSize, sizeGEP);
+
+mapTypeFlags.push_back(mapTypeOp.dyn_cast().getInt());
+llvm::Constant *mapName =
+mlir::LLVM::createMappingInformation(mapOp.getLoc(), *ompBuilder);
+mapNames.push_back(mapName);
+++index;
+  }
+
+  return success();
+}
+
+static LogicalResult
+convertOmpTargetData(Operation *op, llvm::IRBuilderBase ,
+ LLVM::ModuleTranslation ) {
+  unsigned numMapOperands;
+  llvm::Function *mapperFunc;
+  llvm::Value *ifCond = nullptr;
+  int64_t deviceID = llvm::omp::OMP_DEVICEID_UNDEF;
+  SmallVector mapOperands;
+  ArrayAttr mapTypes;
+
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+
+  LogicalResult result =
+  llvm::TypeSwitch(op)
+  .Case([&](omp::DataOp dataOp) {
+if (dataOp.getUseDeviceAddr().size() ||
+dataOp.getUseDevicePtr().size())
+  return failure();
+
+if (auto ifExprVar = dataOp.getIfExpr())
+  ifCond = moduleTranslation.lookupValue(ifExprVar);
+
+if (auto devId = dataOp.getDevice())
+  if (auto constOp = mlir::dyn_cast(
+  devId.getDefiningOp()))
+if (auto intAttr =
+constOp.getValue().dyn_cast())
+  

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Please avoid the MLIR style in any other subproject. I know some of the OpenMP 
Builder stuff already uses `llvm::` and MLIR style names, but that is in itself 
bad, not something we should extend.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1092
+  /// Computes the size of type in bytes.
+  llvm::Value *getSizeInBytes(llvm::Value *basePtr);
+

Nit: Style, no llvm::



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1573
+   llvm::Value *ifCond, BodyGenCallbackTy processMapOpCB,
+   BodyGenCallbackTy BodyGenCB);
+

Style, see above.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4093
+  return Builder.saveIP();
+}
+

Style, see above. 



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4152
+  return sizePtrToInt;
+}
+

Style, see above. 



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4991
+  EXPECT_TRUE(targetDataCall->getOperand(8)->getType()->isPointerTy());
+}
+

Style, see above. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 495530.
TIFitis edited the summary of this revision.
TIFitis added a comment.

Added explicit failure for use_device_ptr and use_device_addr clauses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -15,10 +15,12 @@
 #include "mlir/IR/IRMapping.h"
 #include "mlir/IR/Operation.h"
 #include "mlir/Support/LLVM.h"
+#include "mlir/Target/LLVMIR/Dialect/Utils.h"
 #include "mlir/Target/LLVMIR/ModuleTranslation.h"
 
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/IRBuilder.h"
@@ -1338,6 +1340,191 @@
   return success();
 }
 
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,
+const SmallVector , const ArrayAttr ,
+SmallVector ,
+SmallVectorImpl ,
+struct llvm::OpenMPIRBuilder::MapperAllocas ) {
+  auto numMapOperands = mapOperands.size();
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+  llvm::PointerType *i8PtrTy = builder.getInt8PtrTy();
+  llvm::ArrayType *arrI8PtrTy = llvm::ArrayType::get(i8PtrTy, numMapOperands);
+  llvm::IntegerType *i64Ty = builder.getInt64Ty();
+  llvm::ArrayType *arrI64Ty = llvm::ArrayType::get(i64Ty, numMapOperands);
+
+  unsigned index = 0;
+  for (const auto  : mapOperands) {
+const auto  = mapTypes[index];
+
+llvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);
+llvm::Value *mapOpPtrBase;
+llvm::Value *mapOpPtr;
+llvm::Value *mapOpSize;
+
+if (mapOp.getType().isa()) {
+  mapOpPtrBase = mapOpValue;
+  mapOpPtr = mapOpValue;
+  mapOpSize = ompBuilder->getSizeInBytes(mapOpValue);
+} else {
+  return failure();
+}
+
+// Store base pointer extracted from operand into the i-th position of
+// argBase.
+llvm::Value *ptrBaseGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.ArgsBase,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrBaseCast = builder.CreateBitCast(
+ptrBaseGEP, mapOpPtrBase->getType()->getPointerTo());
+builder.CreateStore(mapOpPtrBase, ptrBaseCast);
+
+// Store pointer extracted from operand into the i-th position of args.
+llvm::Value *ptrGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.Args,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrCast =
+builder.CreateBitCast(ptrGEP, mapOpPtr->getType()->getPointerTo());
+builder.CreateStore(mapOpPtr, ptrCast);
+
+// Store size extracted from operand into the i-th position of argSizes.
+llvm::Value *sizeGEP = builder.CreateInBoundsGEP(
+arrI64Ty, mapperAllocas.ArgSizes,
+{builder.getInt32(0), builder.getInt32(index)});
+builder.CreateStore(mapOpSize, sizeGEP);
+
+mapTypeFlags.push_back(mapTypeOp.dyn_cast().getInt());
+llvm::Constant *mapName =
+mlir::LLVM::createMappingInformation(mapOp.getLoc(), *ompBuilder);
+mapNames.push_back(mapName);
+++index;
+  }
+
+  return success();
+}
+
+static LogicalResult
+convertOmpTargetData(Operation *op, llvm::IRBuilderBase ,
+ LLVM::ModuleTranslation ) {
+  unsigned numMapOperands;
+  llvm::Function *mapperFunc;
+  llvm::Value *ifCond = nullptr;
+  int64_t deviceID = llvm::omp::OMP_DEVICEID_UNDEF;
+  SmallVector mapOperands;
+  ArrayAttr mapTypes;
+
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+
+  LogicalResult result =
+  llvm::TypeSwitch(op)
+  .Case([&](omp::DataOp dataOp) {
+if (dataOp.getUseDeviceAddr().size() ||
+dataOp.getUseDevicePtr().size())
+  return failure();
+
+if (auto ifExprVar = dataOp.getIfExpr())
+  ifCond = moduleTranslation.lookupValue(ifExprVar);
+
+if (auto devId = dataOp.getDevice())
+  if (auto constOp = mlir::dyn_cast(
+  devId.getDefiningOp()))
+if (auto intAttr =
+

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 495493.
TIFitis marked 4 inline comments as done.
TIFitis added a comment.

Changed namespace to mlir::LLVM for common files. Addressed reviewer comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -15,10 +15,12 @@
 #include "mlir/IR/IRMapping.h"
 #include "mlir/IR/Operation.h"
 #include "mlir/Support/LLVM.h"
+#include "mlir/Target/LLVMIR/Dialect/Utils.h"
 #include "mlir/Target/LLVMIR/ModuleTranslation.h"
 
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/IRBuilder.h"
@@ -1338,6 +1340,187 @@
   return success();
 }
 
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,
+const SmallVector , const ArrayAttr ,
+SmallVector ,
+SmallVectorImpl ,
+struct llvm::OpenMPIRBuilder::MapperAllocas ) {
+  auto numMapOperands = mapOperands.size();
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+  llvm::PointerType *i8PtrTy = builder.getInt8PtrTy();
+  llvm::ArrayType *arrI8PtrTy = llvm::ArrayType::get(i8PtrTy, numMapOperands);
+  llvm::IntegerType *i64Ty = builder.getInt64Ty();
+  llvm::ArrayType *arrI64Ty = llvm::ArrayType::get(i64Ty, numMapOperands);
+
+  unsigned index = 0;
+  for (const auto  : mapOperands) {
+const auto  = mapTypes[index];
+
+llvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);
+llvm::Value *mapOpPtrBase;
+llvm::Value *mapOpPtr;
+llvm::Value *mapOpSize;
+
+if (mapOp.getType().isa()) {
+  mapOpPtrBase = mapOpValue;
+  mapOpPtr = mapOpValue;
+  mapOpSize = ompBuilder->getSizeInBytes(mapOpValue);
+} else {
+  return failure();
+}
+
+// Store base pointer extracted from operand into the i-th position of
+// argBase.
+llvm::Value *ptrBaseGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.ArgsBase,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrBaseCast = builder.CreateBitCast(
+ptrBaseGEP, mapOpPtrBase->getType()->getPointerTo());
+builder.CreateStore(mapOpPtrBase, ptrBaseCast);
+
+// Store pointer extracted from operand into the i-th position of args.
+llvm::Value *ptrGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.Args,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrCast =
+builder.CreateBitCast(ptrGEP, mapOpPtr->getType()->getPointerTo());
+builder.CreateStore(mapOpPtr, ptrCast);
+
+// Store size extracted from operand into the i-th position of argSizes.
+llvm::Value *sizeGEP = builder.CreateInBoundsGEP(
+arrI64Ty, mapperAllocas.ArgSizes,
+{builder.getInt32(0), builder.getInt32(index)});
+builder.CreateStore(mapOpSize, sizeGEP);
+
+mapTypeFlags.push_back(mapTypeOp.dyn_cast().getInt());
+llvm::Constant *mapName =
+mlir::LLVM::createMappingInformation(mapOp.getLoc(), *ompBuilder);
+mapNames.push_back(mapName);
+++index;
+  }
+
+  return success();
+}
+
+static LogicalResult
+convertOmpTargetData(Operation *op, llvm::IRBuilderBase ,
+ LLVM::ModuleTranslation ) {
+  unsigned numMapOperands;
+  llvm::Function *mapperFunc;
+  llvm::Value *ifCond = nullptr;
+  int64_t deviceID = llvm::omp::OMP_DEVICEID_UNDEF;
+  SmallVector mapOperands;
+  ArrayAttr mapTypes;
+
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+
+  LogicalResult result =
+  llvm::TypeSwitch(op)
+  .Case([&](omp::DataOp dataOp) {
+if (auto ifExprVar = dataOp.getIfExpr())
+  ifCond = moduleTranslation.lookupValue(ifExprVar);
+
+if (auto devId = dataOp.getDevice())
+  if (auto constOp = mlir::dyn_cast(
+  devId.getDefiningOp()))
+if (auto intAttr =
+constOp.getValue().dyn_cast())
+  deviceID = intAttr.getInt();
+
+numMapOperands = 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Alex Zinenko via Phabricator via cfe-commits]

ftynse added inline comments.



Comment at: mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h:27
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,

It's not great to have static functions in a header. I suppose this is done to 
avoid build dependencies, but it's better to get the dependencies right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Alex Zinenko via Phabricator via cfe-commits]

ftynse added inline comments.



Comment at: mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h:13
+
+#ifndef MLIR_DIALECT_UTILS_H
+#define MLIR_DIALECT_UTILS_H

This tag is wrong. It should be `MLIR_TARGET_LLVMIR_DIALECT_UTILS_H`.



Comment at: mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h:59
+#endif // MLIR_DIALECT_UTILS_H
\ No newline at end of file


Nit: please add a newline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked an inline comment as done.
TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

kiranchandramohan wrote:
> TIFitis wrote:
> > clementval wrote:
> > > TIFitis wrote:
> > > > clementval wrote:
> > > > > TIFitis wrote:
> > > > > > clementval wrote:
> > > > > > > TIFitis wrote:
> > > > > > > > clementval wrote:
> > > > > > > > > TIFitis wrote:
> > > > > > > > > > clementval wrote:
> > > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > > clementval wrote:
> > > > > > > > > > > > > Instead of copy pasting this from 
> > > > > > > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > > > > > > > >  can you extract it and put it in a common shared 
> > > > > > > > > > > > > file so bith translation can use the same code 
> > > > > > > > > > > > > without duplication?
> > > > > > > > > > > > @raghavendhra put up a patch some time back and he 
> > > > > > > > > > > > faced some issues. It might be good to check with him 
> > > > > > > > > > > > or may be he can comment here.
> > > > > > > > > > > > https://reviews.llvm.org/D127037
> > > > > > > > > > > > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> > > > > > > > > > > Just moving the three functions should be trivial. I'm 
> > > > > > > > > > > not talking about the processMapOperand.
> > > > > > > > > > I've moved `getSizeInBytes`.
> > > > > > > > > > 
> > > > > > > > > > The other two functions make use of `mlir::Location` and 
> > > > > > > > > > thus can't be moved trivially.
> > > > > > > > > > 
> > > > > > > > > > I can still try to move them by individually passing the 
> > > > > > > > > > elements of `mlir::Location` but that might not be ideal. 
> > > > > > > > > > Is that what you'd like?
> > > > > > > > > What about a new header file in 
> > > > > > > > > `mlir/include/mlir/Target/LLVMIR/Dialect/**` shared by 
> > > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > > > >  and 
> > > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`.
> > > > > > > > >  That should be doable. 
> > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > > >  and 
> > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`
> > > > > > > >  already have access to the common `mlir::Location` type.
> > > > > > > > 
> > > > > > > > Problem is that `OMPIRBuilder.cpp` is the only common file 
> > > > > > > > between them  where I can move the two functions to. Currently 
> > > > > > > > there are no `mlir/**` include files in `OMPIRBuilder.cpp` and 
> > > > > > > > it seems to me like a strict design choice to have it that way.
> > > > > > > The functions can be header only. Why do you need to put them in 
> > > > > > > the OMPIRBuilder.cpp? I think it is better than duplicate the 
> > > > > > > exact same code over. 
> > > > > > Sorry, I misunderstood you earlier.
> > > > > > I've added a new header file 
> > > > > > `mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h`, this is my first 
> > > > > > attempt at adding a new header file, please let me know if you find 
> > > > > > any issues.
> > > > > Thanks! That's what I had in mind. We might want to check with MLIR 
> > > > > folks if `mlir::utils` is suited for that. I don't mind if it is 
> > > > > `mlir::omp::builder` or smth similar since it is related to the 
> > > > > OMPIRBuilder.
> > > > Since the utils file is common to all the dialects I kept it as 
> > > > `mlir::utils`.
> > > > 
> > > > How do I get the opinion from people working in MLIR on this, can you 
> > > > suggest some reviewers whom I can add?
> > > It's only valid for translation to the `llvmir` dialect so that why 
> > > `mlir::utils` seems to generic to me. 
> > > 
> > > Maybe @ftynse has some thoughts on this. 
> > I agree with you on that, would perhaps renaming it to something like 
> > `mlir::dialect-utils` be a better option?
> You can post in MLIR discourse MLIR section 
> (https://discourse.llvm.org/c/mlir/31) to get an opinion.
> 
> open-directive-utils , ompbuilder-utils are other options. Simialr names 
> could be considered for the file name as well.
> 
> 
> 
> 
I've created a discourse topic for this here.
https://discourse.llvm.org/t/rfc-adding-new-util-file-to-mlir-dialect-translation/68221?u=tifitis


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Please add more description to the summary regarding the scope of this patch. 
If it is only able to lower specific llvm-dialect types, please call out that. 
Please also explain the split in work between mlir::Translation and 
OpenMPIRbuilder. You can also consider splitting this into two patches. One 
that just adds the changes to OpenMPIRBuilder and the child version of the 
patch adding the translation in MLIR.




Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

TIFitis wrote:
> clementval wrote:
> > TIFitis wrote:
> > > clementval wrote:
> > > > TIFitis wrote:
> > > > > clementval wrote:
> > > > > > TIFitis wrote:
> > > > > > > clementval wrote:
> > > > > > > > TIFitis wrote:
> > > > > > > > > clementval wrote:
> > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > clementval wrote:
> > > > > > > > > > > > Instead of copy pasting this from 
> > > > > > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > > > > > > >  can you extract it and put it in a common shared file 
> > > > > > > > > > > > so bith translation can use the same code without 
> > > > > > > > > > > > duplication?
> > > > > > > > > > > @raghavendhra put up a patch some time back and he faced 
> > > > > > > > > > > some issues. It might be good to check with him or may be 
> > > > > > > > > > > he can comment here.
> > > > > > > > > > > https://reviews.llvm.org/D127037
> > > > > > > > > > > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> > > > > > > > > > Just moving the three functions should be trivial. I'm not 
> > > > > > > > > > talking about the processMapOperand.
> > > > > > > > > I've moved `getSizeInBytes`.
> > > > > > > > > 
> > > > > > > > > The other two functions make use of `mlir::Location` and thus 
> > > > > > > > > can't be moved trivially.
> > > > > > > > > 
> > > > > > > > > I can still try to move them by individually passing the 
> > > > > > > > > elements of `mlir::Location` but that might not be ideal. Is 
> > > > > > > > > that what you'd like?
> > > > > > > > What about a new header file in 
> > > > > > > > `mlir/include/mlir/Target/LLVMIR/Dialect/**` shared by 
> > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > > >  and 
> > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`.
> > > > > > > >  That should be doable. 
> > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > >  and 
> > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`
> > > > > > >  already have access to the common `mlir::Location` type.
> > > > > > > 
> > > > > > > Problem is that `OMPIRBuilder.cpp` is the only common file 
> > > > > > > between them  where I can move the two functions to. Currently 
> > > > > > > there are no `mlir/**` include files in `OMPIRBuilder.cpp` and it 
> > > > > > > seems to me like a strict design choice to have it that way.
> > > > > > The functions can be header only. Why do you need to put them in 
> > > > > > the OMPIRBuilder.cpp? I think it is better than duplicate the exact 
> > > > > > same code over. 
> > > > > Sorry, I misunderstood you earlier.
> > > > > I've added a new header file 
> > > > > `mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h`, this is my first 
> > > > > attempt at adding a new header file, please let me know if you find 
> > > > > any issues.
> > > > Thanks! That's what I had in mind. We might want to check with MLIR 
> > > > folks if `mlir::utils` is suited for that. I don't mind if it is 
> > > > `mlir::omp::builder` or smth similar since it is related to the 
> > > > OMPIRBuilder.
> > > Since the utils file is common to all the dialects I kept it as 
> > > `mlir::utils`.
> > > 
> > > How do I get the opinion from people working in MLIR on this, can you 
> > > suggest some reviewers whom I can add?
> > It's only valid for translation to the `llvmir` dialect so that why 
> > `mlir::utils` seems to generic to me. 
> > 
> > Maybe @ftynse has some thoughts on this. 
> I agree with you on that, would perhaps renaming it to something like 
> `mlir::dialect-utils` be a better option?
You can post in MLIR discourse MLIR section 
(https://discourse.llvm.org/c/mlir/31) to get an opinion.

open-directive-utils , ompbuilder-utils are other options. Simialr names could 
be considered for the file name as well.






Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked an inline comment as done.
TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

clementval wrote:
> TIFitis wrote:
> > clementval wrote:
> > > TIFitis wrote:
> > > > clementval wrote:
> > > > > TIFitis wrote:
> > > > > > clementval wrote:
> > > > > > > TIFitis wrote:
> > > > > > > > clementval wrote:
> > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > clementval wrote:
> > > > > > > > > > > Instead of copy pasting this from 
> > > > > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > > > > > >  can you extract it and put it in a common shared file so 
> > > > > > > > > > > bith translation can use the same code without 
> > > > > > > > > > > duplication?
> > > > > > > > > > @raghavendhra put up a patch some time back and he faced 
> > > > > > > > > > some issues. It might be good to check with him or may be 
> > > > > > > > > > he can comment here.
> > > > > > > > > > https://reviews.llvm.org/D127037
> > > > > > > > > > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> > > > > > > > > Just moving the three functions should be trivial. I'm not 
> > > > > > > > > talking about the processMapOperand.
> > > > > > > > I've moved `getSizeInBytes`.
> > > > > > > > 
> > > > > > > > The other two functions make use of `mlir::Location` and thus 
> > > > > > > > can't be moved trivially.
> > > > > > > > 
> > > > > > > > I can still try to move them by individually passing the 
> > > > > > > > elements of `mlir::Location` but that might not be ideal. Is 
> > > > > > > > that what you'd like?
> > > > > > > What about a new header file in 
> > > > > > > `mlir/include/mlir/Target/LLVMIR/Dialect/**` shared by 
> > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > >  and 
> > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`.
> > > > > > >  That should be doable. 
> > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > >  and 
> > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`
> > > > > >  already have access to the common `mlir::Location` type.
> > > > > > 
> > > > > > Problem is that `OMPIRBuilder.cpp` is the only common file between 
> > > > > > them  where I can move the two functions to. Currently there are no 
> > > > > > `mlir/**` include files in `OMPIRBuilder.cpp` and it seems to me 
> > > > > > like a strict design choice to have it that way.
> > > > > The functions can be header only. Why do you need to put them in the 
> > > > > OMPIRBuilder.cpp? I think it is better than duplicate the exact same 
> > > > > code over. 
> > > > Sorry, I misunderstood you earlier.
> > > > I've added a new header file 
> > > > `mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h`, this is my first 
> > > > attempt at adding a new header file, please let me know if you find any 
> > > > issues.
> > > Thanks! That's what I had in mind. We might want to check with MLIR folks 
> > > if `mlir::utils` is suited for that. I don't mind if it is 
> > > `mlir::omp::builder` or smth similar since it is related to the 
> > > OMPIRBuilder.
> > Since the utils file is common to all the dialects I kept it as 
> > `mlir::utils`.
> > 
> > How do I get the opinion from people working in MLIR on this, can you 
> > suggest some reviewers whom I can add?
> It's only valid for translation to the `llvmir` dialect so that why 
> `mlir::utils` seems to generic to me. 
> 
> Maybe @ftynse has some thoughts on this. 
I agree with you on that, would perhaps renaming it to something like 
`mlir::dialect-utils` be a better option?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Valentin Clement via Phabricator via cfe-commits]

clementval added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

TIFitis wrote:
> clementval wrote:
> > TIFitis wrote:
> > > clementval wrote:
> > > > TIFitis wrote:
> > > > > clementval wrote:
> > > > > > TIFitis wrote:
> > > > > > > clementval wrote:
> > > > > > > > kiranchandramohan wrote:
> > > > > > > > > clementval wrote:
> > > > > > > > > > Instead of copy pasting this from 
> > > > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > > > > >  can you extract it and put it in a common shared file so 
> > > > > > > > > > bith translation can use the same code without duplication?
> > > > > > > > > @raghavendhra put up a patch some time back and he faced some 
> > > > > > > > > issues. It might be good to check with him or may be he can 
> > > > > > > > > comment here.
> > > > > > > > > https://reviews.llvm.org/D127037
> > > > > > > > > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> > > > > > > > Just moving the three functions should be trivial. I'm not 
> > > > > > > > talking about the processMapOperand.
> > > > > > > I've moved `getSizeInBytes`.
> > > > > > > 
> > > > > > > The other two functions make use of `mlir::Location` and thus 
> > > > > > > can't be moved trivially.
> > > > > > > 
> > > > > > > I can still try to move them by individually passing the elements 
> > > > > > > of `mlir::Location` but that might not be ideal. Is that what 
> > > > > > > you'd like?
> > > > > > What about a new header file in 
> > > > > > `mlir/include/mlir/Target/LLVMIR/Dialect/**` shared by 
> > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > >  and 
> > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`.
> > > > > >  That should be doable. 
> > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > >  and 
> > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp` 
> > > > > already have access to the common `mlir::Location` type.
> > > > > 
> > > > > Problem is that `OMPIRBuilder.cpp` is the only common file between 
> > > > > them  where I can move the two functions to. Currently there are no 
> > > > > `mlir/**` include files in `OMPIRBuilder.cpp` and it seems to me like 
> > > > > a strict design choice to have it that way.
> > > > The functions can be header only. Why do you need to put them in the 
> > > > OMPIRBuilder.cpp? I think it is better than duplicate the exact same 
> > > > code over. 
> > > Sorry, I misunderstood you earlier.
> > > I've added a new header file 
> > > `mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h`, this is my first 
> > > attempt at adding a new header file, please let me know if you find any 
> > > issues.
> > Thanks! That's what I had in mind. We might want to check with MLIR folks 
> > if `mlir::utils` is suited for that. I don't mind if it is 
> > `mlir::omp::builder` or smth similar since it is related to the 
> > OMPIRBuilder.
> Since the utils file is common to all the dialects I kept it as `mlir::utils`.
> 
> How do I get the opinion from people working in MLIR on this, can you suggest 
> some reviewers whom I can add?
It's only valid for translation to the `llvmir` dialect so that why 
`mlir::utils` seems to generic to me. 

Maybe @ftynse has some thoughts on this. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked 2 inline comments as done.
TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

clementval wrote:
> TIFitis wrote:
> > clementval wrote:
> > > TIFitis wrote:
> > > > clementval wrote:
> > > > > TIFitis wrote:
> > > > > > clementval wrote:
> > > > > > > kiranchandramohan wrote:
> > > > > > > > clementval wrote:
> > > > > > > > > Instead of copy pasting this from 
> > > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > > > >  can you extract it and put it in a common shared file so 
> > > > > > > > > bith translation can use the same code without duplication?
> > > > > > > > @raghavendhra put up a patch some time back and he faced some 
> > > > > > > > issues. It might be good to check with him or may be he can 
> > > > > > > > comment here.
> > > > > > > > https://reviews.llvm.org/D127037
> > > > > > > > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> > > > > > > Just moving the three functions should be trivial. I'm not 
> > > > > > > talking about the processMapOperand.
> > > > > > I've moved `getSizeInBytes`.
> > > > > > 
> > > > > > The other two functions make use of `mlir::Location` and thus can't 
> > > > > > be moved trivially.
> > > > > > 
> > > > > > I can still try to move them by individually passing the elements 
> > > > > > of `mlir::Location` but that might not be ideal. Is that what you'd 
> > > > > > like?
> > > > > What about a new header file in 
> > > > > `mlir/include/mlir/Target/LLVMIR/Dialect/**` shared by 
> > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > >  and 
> > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`.
> > > > >  That should be doable. 
> > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` 
> > > > and 
> > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp` 
> > > > already have access to the common `mlir::Location` type.
> > > > 
> > > > Problem is that `OMPIRBuilder.cpp` is the only common file between them 
> > > >  where I can move the two functions to. Currently there are no 
> > > > `mlir/**` include files in `OMPIRBuilder.cpp` and it seems to me like a 
> > > > strict design choice to have it that way.
> > > The functions can be header only. Why do you need to put them in the 
> > > OMPIRBuilder.cpp? I think it is better than duplicate the exact same code 
> > > over. 
> > Sorry, I misunderstood you earlier.
> > I've added a new header file 
> > `mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h`, this is my first attempt 
> > at adding a new header file, please let me know if you find any issues.
> Thanks! That's what I had in mind. We might want to check with MLIR folks if 
> `mlir::utils` is suited for that. I don't mind if it is `mlir::omp::builder` 
> or smth similar since it is related to the OMPIRBuilder.
Since the utils file is common to all the dialects I kept it as `mlir::utils`.

How do I get the opinion from people working in MLIR on this, can you suggest 
some reviewers whom I can add?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Valentin Clement via Phabricator via cfe-commits]

clementval added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

TIFitis wrote:
> clementval wrote:
> > TIFitis wrote:
> > > clementval wrote:
> > > > TIFitis wrote:
> > > > > clementval wrote:
> > > > > > kiranchandramohan wrote:
> > > > > > > clementval wrote:
> > > > > > > > Instead of copy pasting this from 
> > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > > >  can you extract it and put it in a common shared file so bith 
> > > > > > > > translation can use the same code without duplication?
> > > > > > > @raghavendhra put up a patch some time back and he faced some 
> > > > > > > issues. It might be good to check with him or may be he can 
> > > > > > > comment here.
> > > > > > > https://reviews.llvm.org/D127037
> > > > > > > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> > > > > > Just moving the three functions should be trivial. I'm not talking 
> > > > > > about the processMapOperand.
> > > > > I've moved `getSizeInBytes`.
> > > > > 
> > > > > The other two functions make use of `mlir::Location` and thus can't 
> > > > > be moved trivially.
> > > > > 
> > > > > I can still try to move them by individually passing the elements of 
> > > > > `mlir::Location` but that might not be ideal. Is that what you'd like?
> > > > What about a new header file in 
> > > > `mlir/include/mlir/Target/LLVMIR/Dialect/**` shared by 
> > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` 
> > > > and 
> > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`. 
> > > > That should be doable. 
> > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` 
> > > and `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp` 
> > > already have access to the common `mlir::Location` type.
> > > 
> > > Problem is that `OMPIRBuilder.cpp` is the only common file between them  
> > > where I can move the two functions to. Currently there are no `mlir/**` 
> > > include files in `OMPIRBuilder.cpp` and it seems to me like a strict 
> > > design choice to have it that way.
> > The functions can be header only. Why do you need to put them in the 
> > OMPIRBuilder.cpp? I think it is better than duplicate the exact same code 
> > over. 
> Sorry, I misunderstood you earlier.
> I've added a new header file 
> `mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h`, this is my first attempt 
> at adding a new header file, please let me know if you find any issues.
Thanks! That's what I had in mind. We might want to check with MLIR folks if 
`mlir::utils` is suited for that. I don't mind if it is `mlir::omp::builder` or 
smth similar since it is related to the OMPIRBuilder.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked an inline comment as done.
TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

clementval wrote:
> TIFitis wrote:
> > clementval wrote:
> > > TIFitis wrote:
> > > > clementval wrote:
> > > > > kiranchandramohan wrote:
> > > > > > clementval wrote:
> > > > > > > Instead of copy pasting this from 
> > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > >  can you extract it and put it in a common shared file so bith 
> > > > > > > translation can use the same code without duplication?
> > > > > > @raghavendhra put up a patch some time back and he faced some 
> > > > > > issues. It might be good to check with him or may be he can comment 
> > > > > > here.
> > > > > > https://reviews.llvm.org/D127037
> > > > > > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> > > > > Just moving the three functions should be trivial. I'm not talking 
> > > > > about the processMapOperand.
> > > > I've moved `getSizeInBytes`.
> > > > 
> > > > The other two functions make use of `mlir::Location` and thus can't be 
> > > > moved trivially.
> > > > 
> > > > I can still try to move them by individually passing the elements of 
> > > > `mlir::Location` but that might not be ideal. Is that what you'd like?
> > > What about a new header file in 
> > > `mlir/include/mlir/Target/LLVMIR/Dialect/**` shared by 
> > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` 
> > > and 
> > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`. 
> > > That should be doable. 
> > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` and 
> > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp` 
> > already have access to the common `mlir::Location` type.
> > 
> > Problem is that `OMPIRBuilder.cpp` is the only common file between them  
> > where I can move the two functions to. Currently there are no `mlir/**` 
> > include files in `OMPIRBuilder.cpp` and it seems to me like a strict design 
> > choice to have it that way.
> The functions can be header only. Why do you need to put them in the 
> OMPIRBuilder.cpp? I think it is better than duplicate the exact same code 
> over. 
Sorry, I misunderstood you earlier.
I've added a new header file `mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h`, 
this is my first attempt at adding a new header file, please let me know if you 
find any issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 493615.
TIFitis marked an inline comment as done.
TIFitis added a comment.

Added new header file mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h, moved 
two common functions to the new file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -15,10 +15,12 @@
 #include "mlir/IR/IRMapping.h"
 #include "mlir/IR/Operation.h"
 #include "mlir/Support/LLVM.h"
+#include "mlir/Target/LLVMIR/Dialect/Utils.h"
 #include "mlir/Target/LLVMIR/ModuleTranslation.h"
 
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/IRBuilder.h"
@@ -1338,6 +1340,187 @@
   return success();
 }
 
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,
+const SmallVector , const ArrayAttr ,
+SmallVector ,
+SmallVectorImpl ,
+struct llvm::OpenMPIRBuilder::MapperAllocas ) {
+  auto numMapOperands = mapOperands.size();
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+  llvm::PointerType *i8PtrTy = builder.getInt8PtrTy();
+  llvm::ArrayType *arrI8PtrTy = llvm::ArrayType::get(i8PtrTy, numMapOperands);
+  llvm::IntegerType *i64Ty = builder.getInt64Ty();
+  llvm::ArrayType *arrI64Ty = llvm::ArrayType::get(i64Ty, numMapOperands);
+
+  unsigned index = 0;
+  for (const auto  : mapOperands) {
+const auto  = mapTypes[index];
+
+llvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);
+llvm::Value *mapOpPtrBase;
+llvm::Value *mapOpPtr;
+llvm::Value *mapOpSize;
+
+if (mapOp.getType().isa()) {
+  mapOpPtrBase = mapOpValue;
+  mapOpPtr = mapOpValue;
+  mapOpSize = ompBuilder->getSizeInBytes(mapOpValue);
+} else {
+  return failure();
+}
+
+// Store base pointer extracted from operand into the i-th position of
+// argBase.
+llvm::Value *ptrBaseGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.ArgsBase,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrBaseCast = builder.CreateBitCast(
+ptrBaseGEP, mapOpPtrBase->getType()->getPointerTo());
+builder.CreateStore(mapOpPtrBase, ptrBaseCast);
+
+// Store pointer extracted from operand into the i-th position of args.
+llvm::Value *ptrGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.Args,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrCast =
+builder.CreateBitCast(ptrGEP, mapOpPtr->getType()->getPointerTo());
+builder.CreateStore(mapOpPtr, ptrCast);
+
+// Store size extracted from operand into the i-th position of argSizes.
+llvm::Value *sizeGEP = builder.CreateInBoundsGEP(
+arrI64Ty, mapperAllocas.ArgSizes,
+{builder.getInt32(0), builder.getInt32(index)});
+builder.CreateStore(mapOpSize, sizeGEP);
+
+mapTypeFlags.push_back(mapTypeOp.dyn_cast().getInt());
+llvm::Constant *mapName =
+mlir::utils::createMappingInformation(mapOp.getLoc(), *ompBuilder);
+mapNames.push_back(mapName);
+++index;
+  }
+
+  return success();
+}
+
+static LogicalResult
+convertOmpTargetData(Operation *op, llvm::IRBuilderBase ,
+ LLVM::ModuleTranslation ) {
+  unsigned numMapOperands;
+  llvm::Function *mapperFunc;
+  llvm::Value *ifCond = nullptr;
+  int64_t deviceID = llvm::omp::OMP_DEVICEID_UNDEF;
+  SmallVector mapOperands;
+  ArrayAttr mapTypes;
+
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+
+  LogicalResult result =
+  llvm::TypeSwitch(op)
+  .Case([&](omp::DataOp dataOp) {
+if (auto ifExprVar = dataOp.getIfExpr())
+  ifCond = moduleTranslation.lookupValue(ifExprVar);
+
+if (auto devId = dataOp.getDevice())
+  if (auto constOp = mlir::dyn_cast(
+  devId.getDefiningOp()))
+if (auto intAttr =
+constOp.getValue().dyn_cast())
+  deviceID = intAttr.getInt();
+
+ 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Valentin Clement via Phabricator via cfe-commits]

clementval added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

TIFitis wrote:
> clementval wrote:
> > TIFitis wrote:
> > > clementval wrote:
> > > > kiranchandramohan wrote:
> > > > > clementval wrote:
> > > > > > Instead of copy pasting this from 
> > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > >  can you extract it and put it in a common shared file so bith 
> > > > > > translation can use the same code without duplication?
> > > > > @raghavendhra put up a patch some time back and he faced some issues. 
> > > > > It might be good to check with him or may be he can comment here.
> > > > > https://reviews.llvm.org/D127037
> > > > > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> > > > Just moving the three functions should be trivial. I'm not talking 
> > > > about the processMapOperand.
> > > I've moved `getSizeInBytes`.
> > > 
> > > The other two functions make use of `mlir::Location` and thus can't be 
> > > moved trivially.
> > > 
> > > I can still try to move them by individually passing the elements of 
> > > `mlir::Location` but that might not be ideal. Is that what you'd like?
> > What about a new header file in 
> > `mlir/include/mlir/Target/LLVMIR/Dialect/**` shared by 
> > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` and 
> > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`. That 
> > should be doable. 
> `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` and 
> `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp` already 
> have access to the common `mlir::Location` type.
> 
> Problem is that `OMPIRBuilder.cpp` is the only common file between them  
> where I can move the two functions to. Currently there are no `mlir/**` 
> include files in `OMPIRBuilder.cpp` and it seems to me like a strict design 
> choice to have it that way.
The functions can be header only. Why do you need to put them in the 
OMPIRBuilder.cpp? I think it is better than duplicate the exact same code over. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked an inline comment as done.
TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

clementval wrote:
> TIFitis wrote:
> > clementval wrote:
> > > kiranchandramohan wrote:
> > > > clementval wrote:
> > > > > Instead of copy pasting this from 
> > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > >  can you extract it and put it in a common shared file so bith 
> > > > > translation can use the same code without duplication?
> > > > @raghavendhra put up a patch some time back and he faced some issues. 
> > > > It might be good to check with him or may be he can comment here.
> > > > https://reviews.llvm.org/D127037
> > > > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> > > Just moving the three functions should be trivial. I'm not talking about 
> > > the processMapOperand.
> > I've moved `getSizeInBytes`.
> > 
> > The other two functions make use of `mlir::Location` and thus can't be 
> > moved trivially.
> > 
> > I can still try to move them by individually passing the elements of 
> > `mlir::Location` but that might not be ideal. Is that what you'd like?
> What about a new header file in `mlir/include/mlir/Target/LLVMIR/Dialect/**` 
> shared by 
> `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` and 
> `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`. That 
> should be doable. 
`mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` and 
`mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp` already 
have access to the common `mlir::Location` type.

Problem is that `OMPIRBuilder.cpp` is the only common file between them  where 
I can move the two functions to. Currently there are no `mlir/**` include files 
in `OMPIRBuilder.cpp` and it seems to me like a strict design choice to have it 
that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Valentin Clement via Phabricator via cfe-commits]

clementval added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

TIFitis wrote:
> clementval wrote:
> > kiranchandramohan wrote:
> > > clementval wrote:
> > > > Instead of copy pasting this from 
> > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` 
> > > > can you extract it and put it in a common shared file so bith 
> > > > translation can use the same code without duplication?
> > > @raghavendhra put up a patch some time back and he faced some issues. It 
> > > might be good to check with him or may be he can comment here.
> > > https://reviews.llvm.org/D127037
> > > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> > Just moving the three functions should be trivial. I'm not talking about 
> > the processMapOperand.
> I've moved `getSizeInBytes`.
> 
> The other two functions make use of `mlir::Location` and thus can't be moved 
> trivially.
> 
> I can still try to move them by individually passing the elements of 
> `mlir::Location` but that might not be ideal. Is that what you'd like?
What about a new header file in `mlir/include/mlir/Target/LLVMIR/Dialect/**` 
shared by 
`mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` and 
`mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`. That 
should be doable. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis updated this revision to Diff 493549.
TIFitis marked an inline comment as done.
TIFitis added a comment.

Moved getSizeInBytes to OMPIRBuilder.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -19,6 +19,7 @@
 
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/IRBuilder.h"
@@ -1338,6 +1339,216 @@
   return success();
 }
 
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();
+unsigned colNo = fileLoc.getColumn();
+return builder.getOrCreateSrcLocStr(name, fileName, lineNo, colNo, strLen);
+  }
+  std::string locStr;
+  llvm::raw_string_ostream locOS(locStr);
+  locOS << loc;
+  return builder.getOrCreateSrcLocStr(locOS.str(), strLen);
+}
+
+/// Create a constant string representing the mapping information extracted from
+/// the MLIR location information.
+static llvm::Constant *
+createMappingInformation(Location loc, llvm::OpenMPIRBuilder ) {
+  uint32_t strLen;
+  if (auto nameLoc = loc.dyn_cast()) {
+StringRef name = nameLoc.getName();
+return createSourceLocStrFromLocation(nameLoc.getChildLoc(), builder, name,
+  strLen);
+  }
+  return createSourceLocStrFromLocation(loc, builder, "unknown", strLen);
+}
+
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,
+const SmallVector , const ArrayAttr ,
+SmallVector ,
+SmallVectorImpl ,
+struct llvm::OpenMPIRBuilder::MapperAllocas ) {
+  auto numMapOperands = mapOperands.size();
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+  llvm::PointerType *i8PtrTy = builder.getInt8PtrTy();
+  llvm::ArrayType *arrI8PtrTy = llvm::ArrayType::get(i8PtrTy, numMapOperands);
+  llvm::IntegerType *i64Ty = builder.getInt64Ty();
+  llvm::ArrayType *arrI64Ty = llvm::ArrayType::get(i64Ty, numMapOperands);
+
+  unsigned index = 0;
+  for (const auto  : mapOperands) {
+const auto  = mapTypes[index];
+
+llvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);
+llvm::Value *mapOpPtrBase;
+llvm::Value *mapOpPtr;
+llvm::Value *mapOpSize;
+
+if (mapOp.getType().isa()) {
+  mapOpPtrBase = mapOpValue;
+  mapOpPtr = mapOpValue;
+  mapOpSize = ompBuilder->getSizeInBytes(mapOpValue);
+} else {
+  return failure();
+}
+
+// Store base pointer extracted from operand into the i-th position of
+// argBase.
+llvm::Value *ptrBaseGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.ArgsBase,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrBaseCast = builder.CreateBitCast(
+ptrBaseGEP, mapOpPtrBase->getType()->getPointerTo());
+builder.CreateStore(mapOpPtrBase, ptrBaseCast);
+
+// Store pointer extracted from operand into the i-th position of args.
+llvm::Value *ptrGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.Args,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrCast =
+builder.CreateBitCast(ptrGEP, mapOpPtr->getType()->getPointerTo());
+builder.CreateStore(mapOpPtr, ptrCast);
+
+// Store size extracted from operand into the i-th position of argSizes.
+llvm::Value *sizeGEP = builder.CreateInBoundsGEP(
+arrI64Ty, mapperAllocas.ArgSizes,
+{builder.getInt32(0), builder.getInt32(index)});
+builder.CreateStore(mapOpSize, sizeGEP);
+
+mapTypeFlags.push_back(mapTypeOp.dyn_cast().getInt());
+llvm::Constant *mapName =
+createMappingInformation(mapOp.getLoc(), *ompBuilder);
+mapNames.push_back(mapName);
+++index;
+  }
+
+  return success();
+}
+
+static LogicalResult
+convertOmpTargetData(Operation *op, llvm::IRBuilderBase ,
+   

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked 2 inline comments as done.
TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

clementval wrote:
> kiranchandramohan wrote:
> > clementval wrote:
> > > Instead of copy pasting this from 
> > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` 
> > > can you extract it and put it in a common shared file so bith translation 
> > > can use the same code without duplication?
> > @raghavendhra put up a patch some time back and he faced some issues. It 
> > might be good to check with him or may be he can comment here.
> > https://reviews.llvm.org/D127037
> > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> Just moving the three functions should be trivial. I'm not talking about the 
> processMapOperand.
I've moved `getSizeInBytes`.

The other two functions make use of `mlir::Location` and thus can't be moved 
trivially.

I can still try to move them by individually passing the elements of 
`mlir::Location` but that might not be ideal. Is that what you'd like?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Valentin Clement via Phabricator via cfe-commits]

clementval added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

kiranchandramohan wrote:
> clementval wrote:
> > Instead of copy pasting this from 
> > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` can 
> > you extract it and put it in a common shared file so bith translation can 
> > use the same code without duplication?
> @raghavendhra put up a patch some time back and he faced some issues. It 
> might be good to check with him or may be he can comment here.
> https://reviews.llvm.org/D127037
> https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
Just moving the three functions should be trivial. I'm not talking about the 
processMapOperand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a subscriber: raghavendhra.
kiranchandramohan added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

clementval wrote:
> Instead of copy pasting this from 
> `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` can 
> you extract it and put it in a common shared file so bith translation can use 
> the same code without duplication?
@raghavendhra put up a patch some time back and he faced some issues. It might 
be good to check with him or may be he can comment here.
https://reviews.llvm.org/D127037
https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Valentin Clement via Phabricator via cfe-commits]

clementval added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

Instead of copy pasting this from 
`mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` can you 
extract it and put it in a common shared file so bith translation can use the 
same code without duplication?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis created this revision.
TIFitis added reviewers: kiranchandramohan, clementval, jdoerfert.
Herald added subscribers: Moerafaat, zero9178, bzcheeseman, awarzynski, 
sdasgup3, wenzhicui, wrengr, cota, teijeong, rdzhabarov, tatianashp, msifontes, 
jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, mgester, arpith-jacob, 
antiagainst, shauheen, rriddle, mehdi_amini, thopre, guansong, hiraditya, 
yaxunl.
Herald added a reviewer: ftynse.
Herald added a project: All.
TIFitis requested review of this revision.
Herald added a reviewer: nicolasvasilache.
Herald added subscribers: llvm-commits, cfe-commits, sstefan1, 
stephenneuendorffer, nicolasvasilache.
Herald added a reviewer: dcaballe.
Herald added projects: clang, MLIR, LLVM.

This patch adds OpenMP IRBuilder support for the Target Data directives to 
allow lowering to LLVM IR.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142914

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -19,6 +19,7 @@
 
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/IRBuilder.h"
@@ -1338,6 +1339,230 @@
   return success();
 }
 
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder ,
+   StringRef name, uint32_t ) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();
+unsigned colNo = fileLoc.getColumn();
+return builder.getOrCreateSrcLocStr(name, fileName, lineNo, colNo, strLen);
+  }
+  std::string locStr;
+  llvm::raw_string_ostream locOS(locStr);
+  locOS << loc;
+  return builder.getOrCreateSrcLocStr(locOS.str(), strLen);
+}
+
+/// Create a constant string representing the mapping information extracted from
+/// the MLIR location information.
+static llvm::Constant *
+createMappingInformation(Location loc, llvm::OpenMPIRBuilder ) {
+  uint32_t strLen;
+  if (auto nameLoc = loc.dyn_cast()) {
+StringRef name = nameLoc.getName();
+return createSourceLocStrFromLocation(nameLoc.getChildLoc(), builder, name,
+  strLen);
+  }
+  return createSourceLocStrFromLocation(loc, builder, "unknown", strLen);
+}
+
+/// Computes the size of type in bytes.
+static llvm::Value *getSizeInBytes(llvm::IRBuilderBase ,
+   llvm::Value *basePtr) {
+  llvm::LLVMContext  = builder.getContext();
+  llvm::Value *null =
+  llvm::Constant::getNullValue(basePtr->getType()->getPointerTo());
+  llvm::Value *sizeGep =
+  builder.CreateGEP(basePtr->getType(), null, builder.getInt32(1));
+  llvm::Value *sizePtrToInt =
+  builder.CreatePtrToInt(sizeGep, llvm::Type::getInt64Ty(ctx));
+  return sizePtrToInt;
+}
+
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase , LLVM::ModuleTranslation ,
+const SmallVector , const ArrayAttr ,
+SmallVector ,
+SmallVectorImpl ,
+struct llvm::OpenMPIRBuilder::MapperAllocas ) {
+
+  auto numMapOperands = mapOperands.size();
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+  llvm::PointerType *i8PtrTy = builder.getInt8PtrTy();
+  llvm::ArrayType *arrI8PtrTy = llvm::ArrayType::get(i8PtrTy, numMapOperands);
+  llvm::IntegerType *i64Ty = builder.getInt64Ty();
+  llvm::ArrayType *arrI64Ty = llvm::ArrayType::get(i64Ty, numMapOperands);
+
+  unsigned index = 0;
+  for (const auto  : mapOperands) {
+const auto  = mapTypes[index];
+
+llvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);
+llvm::Value *mapOpPtrBase;
+llvm::Value *mapOpPtr;
+llvm::Value *mapOpSize;
+
+if (mapOp.getType().isa()) {
+  mapOpPtrBase = mapOpValue;
+  mapOpPtr = mapOpValue;
+  mapOpSize = getSizeInBytes(builder, mapOpValue);
+} else {
+  return failure();
+}
+
+// Store base pointer extracted from operand into the i-th position of
+// argBase.
+llvm::Value *ptrBaseGEP = builder.CreateInBoundsGEP(
+arrI8PtrTy, mapperAllocas.ArgsBase,
+{builder.getInt32(0), builder.getInt32(index)});
+llvm::Value *ptrBaseCast = builder.CreateBitCast(
+ptrBaseGEP,