subject:"\[PATCH\] D146557\: \[MLIR\]\[OpenMP\] Refactoring how map clause is processed"

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-05-18 Thread Akash Banerjee via Phabricator via cfe-commits

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

Done migrating from Clang. This makes use of the clang functions now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

Files:
  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
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- mlir/test/Target/LLVMIR/omptarget-llvm.mlir
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -11,29 +11,28 @@
   llvm.return
 }
 
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4]
 // 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_2:.*]] = alloca [1 x ptr], align 8
 // CHECK: %[[VAL_3:.*]] = alloca i32, i64 1, align 4
 // CHECK: br label %[[VAL_4:.*]]
-// CHECK:   [[VAL_4]]:
-// CHECK: %[[VAL_5:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
-// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_5]], align 8
-// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// 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 i64], ptr %[[VAL_2]], i32 0, i32 0
-// CHECK: store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %[[VAL_7]], align 4
-// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
-// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
-// CHECK: %[[VAL_10:.*]] = 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_8]], ptr %[[VAL_9]], ptr %[[VAL_10]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// 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 ptr], ptr %[[VAL_2]], i64 0, i64 0
+// CHECK: store ptr null, ptr %[[VAL_8]], align 8
+// 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: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr null, ptr null)
 // CHECK: store i32 99, ptr %[[VAL_3]], align 4
 // CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
 // CHECK: %[[VAL_12:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
-// CHECK: %[[VAL_13:.*]] = 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_11]], ptr %[[VAL_12]], ptr %[[VAL_13]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_11]], ptr %[[VAL_12]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr null, ptr null)
 // CHECK: ret void
 
 // -
@@ -51,35 +50,34 @@
   llvm.return
 }
 
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4096]
 // 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: %[[VAL_2:.*]] = alloca [1 x ptr], align 8
 // CHECK: br label %[[VAL_3:.*]]
-// CHECK:   [[VAL_3]]:
-// CHECK: %[[VAL_4:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
-// CHECK: store ptr %[[VAL_5:.*]], ptr %[[VAL_4]], align 8
-// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
-// CHECK:

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-28 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis planned changes to this revision.
TIFitis added a comment.

@kiranchandramohan I am in the process of submitting a new patch for review 
which moves the map processing from Clang Codegen to OMPIRBuilder. Once that is 
moved we can just use that for dealing with the map operands.
Thus putting this patch on hold for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-28 Thread Akash Banerjee via Phabricator via cfe-commits

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

Rebased


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  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
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- mlir/test/Target/LLVMIR/omptarget-llvm.mlir
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -12,28 +12,24 @@
 }
 
 // CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4]
 // 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:   [[VAL_4]]:
+// CHECK: %[[VAL_2:.*]] = alloca i32, i64 1, align 4
+// 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_3]], ptr %[[VAL_5]], align 8
+// CHECK: store ptr %[[VAL_2]], ptr %[[VAL_5]], align 8
 // CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
-// CHECK: store ptr %[[VAL_3]], ptr %[[VAL_6]], align 8
-// CHECK: %[[VAL_7:.*]] = 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_7]], align 4
-// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
-// CHECK: %[[VAL_9:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
-// CHECK: %[[VAL_10:.*]] = 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_8]], ptr %[[VAL_9]], ptr %[[VAL_10]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
-// CHECK: store i32 99, ptr %[[VAL_3]], align 4
-// CHECK: %[[VAL_11:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
-// CHECK: %[[VAL_12:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
-// CHECK: %[[VAL_13:.*]] = 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_11]], ptr %[[VAL_12]], ptr %[[VAL_13]], ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: store ptr %[[VAL_2]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_7]], ptr %[[VAL_8]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: store i32 99, ptr %[[VAL_2]], 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: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
 // CHECK: ret void
 
 // -
@@ -52,34 +48,30 @@
 }
 
 // CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 2]
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4096]
 // 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:   [[VAL_3]]:
+// CHECK: br label %[[VAL_2:.*]]
+// CHECK:   entry:; preds = %[[VAL_3:.*]]
 // CHECK: %[[VAL_4:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
-// CHECK: store ptr %[[VAL_5:.*]], ptr

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-27 Thread Kiran Chandramohan via Phabricator via cfe-commits

kiranchandramohan added a comment.

In D146557#4301955 , @TIFitis wrote:

> In D146557#4295550 , 
> @kiranchandramohan wrote:
>
>> In D146557#4292223 , @TIFitis 
>> wrote:
>>
>>> Cleaned up how IsBegin argument is passed for createTargetData call.
>>
>> Please submit this directly as an NFC patch.
>
> I have merged it separately.
>
>> We should always work towards simple patches that are easy to review. 
>> Sometimes a big patch is required to give the full context. But it can then 
>> be broken up into small patches for faster review. I have created D149153 
>>  for the `inlineConvertOmpRegions` change.
>
> Thanks for creating your patch. I'll try to make my patches shorter in the 
> future.
>
> Once D149153  is merged I'll rebase this 
> patch.

I have submitted D149153 . 
https://github.com/llvm/llvm-project/commit/1ed522623d95ce7dcf95e711b0f2e3844d2e6be1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-27 Thread Akash Banerjee via Phabricator via cfe-commits

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

Rebased


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  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
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- mlir/test/Target/LLVMIR/omptarget-llvm.mlir
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -12,32 +12,24 @@
 }
 
 // CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4]
 // 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_2:.*]] = alloca i32, i64 1, align 4
+// 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_2]], ptr %[[VAL_5]], align 8
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_2]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_7]], ptr %[[VAL_8]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: store i32 99, ptr %[[VAL_2]], 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: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
 // CHECK: ret void
 
 // -
@@ -56,38 +48,30 @@
 }
 
 // CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 2]
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4096]
 // 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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-27 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis added a comment.

In D146557#4295550 , 
@kiranchandramohan wrote:

> In D146557#4292223 , @TIFitis wrote:
>
>> Cleaned up how IsBegin argument is passed for createTargetData call.
>
> Please submit this directly as an NFC patch.

I have merged it separately.

> We should always work towards simple patches that are easy to review. 
> Sometimes a big patch is required to give the full context. But it can then 
> be broken up into small patches for faster review. I have created D149153 
>  for the `inlineConvertOmpRegions` change.

Thanks for creating your patch. I'll try to make my patches shorter in the 
future.

Once D149153  is merged I'll rebase this 
patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-25 Thread Kiran Chandramohan via Phabricator via cfe-commits

kiranchandramohan added a comment.

In D146557#4292223 , @TIFitis wrote:

> Cleaned up how IsBegin argument is passed for createTargetData call.

Please submit this directly as an NFC patch.

We should always work towards simple patches that are easy to review. Sometimes 
a big patch is required to give the full context. But it can then be broken up 
into small patches for faster review. I have created D149153 
 for the `inlineConvertOmpRegions` change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-24 Thread Akash Banerjee via Phabricator via cfe-commits

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

Cleaned up how IsBegin argument is passed for createTargetData call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  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
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- mlir/test/Target/LLVMIR/omptarget-llvm.mlir
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -12,32 +12,24 @@
 }
 
 // CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4]
 // 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_2:.*]] = alloca i32, i64 1, align 4
+// 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_2]], ptr %[[VAL_5]], align 8
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_2]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_7]], ptr %[[VAL_8]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: store i32 99, ptr %[[VAL_2]], 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: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
 // CHECK: ret void
 
 // -
@@ -56,38 +48,30 @@
 }
 
 // CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 2]
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4096]
 // 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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-21 Thread Akash Banerjee via Phabricator via cfe-commits

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



Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:4-5
-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)) {

kiranchandramohan wrote:
> TIFitis wrote:
> > kiranchandramohan wrote:
> > > Why is the test changed?
> > Moved the map_operand to function parameter to be consistent with other 
> > tests. I think it can remain unchanged if you prefer.
> Consistency can be achieved in a follow-up NFC patch that you can submit 
> without review. If the test can only show the differences of this patch it is 
> easier to review.
Thanks for letting me know. I've updated the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-21 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis updated this revision to Diff 515805.
TIFitis marked an inline comment as done.
TIFitis edited the summary of this revision.
TIFitis added a comment.

Updated tests. Addressed reviewer comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  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
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- mlir/test/Target/LLVMIR/omptarget-llvm.mlir
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -12,32 +12,24 @@
 }
 
 // CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4]
 // 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_2:.*]] = alloca i32, i64 1, align 4
+// 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_2]], ptr %[[VAL_5]], align 8
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_2]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_7]], ptr %[[VAL_8]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: store i32 99, ptr %[[VAL_2]], 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: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
 // CHECK: ret void
 
 // -
@@ -56,38 +48,30 @@
 }
 
 // CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 2]
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4096]
 // CHECK-LABEL: define void @_QPopenmp_target_data_region
 // CHECK: (ptr %[[ARG_0:.*]]) {
 // CHECK: %[[VAL_0:.*]] = alloca [1 x ptr], align 8
 // CHECK:

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-20 Thread Kiran Chandramohan via Phabricator via cfe-commits

kiranchandramohan added a comment.

Would it be possible to break this into a few patches like suggested below or 
similar?

1. Changed to using inlineConvertOmpRegions to generate target data associated 
region code inline in same block if possible.

2. Changed to calculating map_operand size from mlir type instead of llvm.

Moved getSizeInBytes function back into OpenACC code as it is no longer shared.

3. Moved most of map_operands related codegen to OMPIRBuilder.

Moved mapper_allocas related codegen to OMPIRBuilder.

4. Changed offload_sizes parameter to a global variable same as 
offload_maptypes to be more consistent with Clang generated code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-20 Thread Kiran Chandramohan via Phabricator via cfe-commits

kiranchandramohan added inline comments.



Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:4-5
-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)) {

TIFitis wrote:
> kiranchandramohan wrote:
> > Why is the test changed?
> Moved the map_operand to function parameter to be consistent with other 
> tests. I think it can remain unchanged if you prefer.
Consistency can be achieved in a follow-up NFC patch that you can submit 
without review. If the test can only show the differences of this patch it is 
easier to review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-20 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis added a comment.

In D146557#4283149 , 
@kiranchandramohan wrote:

> Please update the summary to convey all the changes introduced in the patch.
>
> From the tests, it looks like there is a substantial change in the IR. I was 
> hoping this to be an NFC change.

I have updated the summary highlighting the notable changes along with changes 
to the generated code.




Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:4-5
-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)) {

kiranchandramohan wrote:
> Why is the test changed?
Moved the map_operand to function parameter to be consistent with other tests. 
I think it can remain unchanged if you prefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-20 Thread Kiran Chandramohan via Phabricator via cfe-commits

kiranchandramohan added a comment.

Please update the summary to convey all the changes introduced in the patch.

From the tests, it looks like there is a substantial change in the IR. I was 
hoping this to be an NFC change.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1383
   struct MapperAllocas , int64_t DeviceID,
-  unsigned NumOperands);
+  unsigned NumOperands, Value *MapsizesArg = nullptr);
 

Please add the new argument to the documentation above.



Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:4-5
-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)) {

Why is the test changed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-17 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis added a comment.

ping for reviews. Thanks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-14 Thread Akash Banerjee via Phabricator via cfe-commits

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

Updated to use opaque pointers, removed BitCast Insts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  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
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- mlir/test/Target/LLVMIR/omptarget-llvm.mlir
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -1,8 +1,6 @@
 // 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
+llvm.func @_QPopenmp_target_data(%1 : !llvm.ptr) {
   omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
 %2 = llvm.mlir.constant(99 : i32) : i32
 llvm.store %2, %1 : !llvm.ptr
@@ -12,43 +10,35 @@
 }
 
 // CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
-// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4]
+// CHECK-LABEL: define void @_QPopenmp_target_data
+// 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: %[[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: br label %[[VAL_2:.*]]
+// CHECK:   entry:; preds = %[[VAL_3:.*]]
+// CHECK: %[[VAL_4:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[ARG_0]], ptr %[[VAL_4]], align 8
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[ARG_0]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_7]], ptr %[[VAL_8]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: store i32 99, ptr %[[ARG_0]], 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: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr @.offload_sizes, ptr

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-11 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis added a comment.
Herald added a subscriber: bviyer.

ping for reviews. Thanks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-04-03 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis marked 2 inline comments as not done.
TIFitis added a comment.

Polite request for reviews. Thanks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-03-30 Thread Akash Banerjee via Phabricator via cfe-commits

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

Updated unit test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  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
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- mlir/test/Target/LLVMIR/omptarget-llvm.mlir
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -1,8 +1,6 @@
 // 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
+llvm.func @_QPopenmp_target_data(%1 : !llvm.ptr) {
   omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
 %2 = llvm.mlir.constant(99 : i32) : i32
 llvm.store %2, %1 : !llvm.ptr
@@ -12,43 +10,35 @@
 }
 
 // CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
-// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4]
+// CHECK-LABEL: define void @_QPopenmp_target_data
+// 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: %[[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: br label %[[VAL_2:.*]]
+// CHECK:   entry:; preds = %[[VAL_3:.*]]
+// CHECK: %[[VAL_4:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[ARG_0]], ptr %[[VAL_4]], align 8
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[ARG_0]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_7]], ptr %[[VAL_8]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: store i32 99, ptr %[[ARG_0]], 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: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-03-30 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357-1362
+int64_t getSizeInBytes(DataLayout , const mlir::Type ) {
+  if (isa(type))
+return DL.getTypeSize(cast(type).getElementType());
+
+  return -1;
+}

@jdoerfert Is this way of getting the size correct? It seems to work for basic 
types and arrays which is what we support for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-03-30 Thread Akash Banerjee via Phabricator via cfe-commits

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

Changed how size is calculated. Updated tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  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
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- mlir/test/Target/LLVMIR/omptarget-llvm.mlir
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -1,8 +1,6 @@
 // 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
+llvm.func @_QPopenmp_target_data(%1 : !llvm.ptr) {
   omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {
 %2 = llvm.mlir.constant(99 : i32) : i32
 llvm.store %2, %1 : !llvm.ptr
@@ -12,43 +10,35 @@
 }
 
 // CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
-// CHECK-LABEL: define void @_QPopenmp_target_data() {
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4]
+// CHECK-LABEL: define void @_QPopenmp_target_data
+// 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: %[[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: br label %[[VAL_2:.*]]
+// CHECK:   entry:; preds = %[[VAL_3:.*]]
+// CHECK: %[[VAL_4:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: store ptr %[[ARG_0]], ptr %[[VAL_4]], align 8
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[ARG_0]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_7]], ptr %[[VAL_8]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: store i32 99, ptr %[[ARG_0]], 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: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-03-23 Thread Akash Banerjee via Phabricator via cfe-commits

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

Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4064-4077
+Value *BaseGEPPtr = Builder.CreateInBoundsGEP(
+ArrI8PtrTy, MapperAllocas.ArgsBase,
+{Builder.getInt32(0), Builder.getInt32(Index)});
+Value *BaseCastPtr = Builder.CreateBitCast(
+BaseGEPPtr, MapOpPtrBase->getType()->getPointerTo());
+Builder.CreateStore(MapOpPtrBase, BaseCastPtr);
+MapInfo.BasePtr = BaseGEPPtr;

I think these should just use the opaque ptr type instead of the ArrI8PtrTy it 
is currently using, and we can also get rid of the BitCast instructions.

However doing so would break the tests in OpenMPIRBuilderTest.cpp as that unit 
test still uses typed pointers.

AFAIK the consensus is to completely get rid of typed pointers in llvm 17. 
Should I updated this code to use opaque pointers and let the test fail until 
it gets converted to opaque as well?

Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4261
+  bool Temp;
+  return Val->getPointerDereferenceableBytes(M.getDataLayout(), Temp, Temp);
 }

jdoerfert wrote:
> TIFitis wrote:
> > `getPointerDereferenceableBytes` doesn't fully support `Arguments` and 
> > mostly returns 0.
> > 
> > I am looking at adding support for arguments in a separate patch, in the 
> > meantime the tests added in this patch fail because of incorrect 
> > @.offload_sizes.
> That function is not the right one for code generation. The sizes need to 
> come from the type + data layout, and potentially the expression used in the 
> map.
Thanks for letting me know.

Just to clarify, do you mean it should come from the MLIR::Type?

The llvm::Type doesn't let me get much information because of Opaque pointers.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-03-22 Thread Johannes Doerfert via Phabricator via cfe-commits

jdoerfert added inline comments.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4261
+  bool Temp;
+  return Val->getPointerDereferenceableBytes(M.getDataLayout(), Temp, Temp);
 }

TIFitis wrote:
> `getPointerDereferenceableBytes` doesn't fully support `Arguments` and mostly 
> returns 0.
> 
> I am looking at adding support for arguments in a separate patch, in the 
> meantime the tests added in this patch fail because of incorrect 
> @.offload_sizes.
That function is not the right one for code generation. The sizes need to come 
from the type + data layout, and potentially the expression used in the map.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-03-22 Thread Akash Banerjee via Phabricator via cfe-commits

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



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4181-4191
+for (auto  : UseDeviceInfos) {
+  auto *UseDeviceVal = UseDeviceOp.first;
+  auto  = UseDeviceOp.second;
+  SmallDenseSet ReplaceUses;
+  for (auto  : UseDeviceVal->uses()) {
+if (UseDeviceInfo.Uses.find() == UseDeviceInfo.Uses.end())
+  ReplaceUses.insert();

jdoerfert wrote:
> TIFitis wrote:
> > I couldn't come up with a better way for the code generated inside the 
> > `target data region` to use the new `AllocaPtrs` when accessing `map 
> > operands` marked with the `use_device_ptr` clause.
> > 
> > Currently I am just comparing the `Users` list before and after generating 
> > the `data region` and replacing the new `uses`.
> > 
> > Alternative method would be to use `mlir::LLVM::moduleTranslation.mapValue` 
> > method to temporarily remap the `mlir::Value` for the `use_device_ptr` 
> > operands to the `AllocaPtrs` `llvm::Value`. But `mapValue` doesn't allow 
> > remapping of values. @kiranchandramohan Is there a good reason to prevent 
> > remapping of values?
> > 
> > Let me know if anyone has a better way of doing this :)
> This is only a last resort. We should not generate stuff that is wrong and is 
> replaced. However, if we cannot find a better way we can talk about it.
> 
> Generally, I would have assumed that the value in the region in marked to be 
> a use_dev_ptr/addr and that we would generate code for that as soon as we 
> generate code for the value.
I have tried to explain this a little better in https://reviews.llvm.org/D146648

The region code is generated immediately, its that it still uses the old 
pointer for it instead of the newly allocated device ptr and this fixes that.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4261
+  bool Temp;
+  return Val->getPointerDereferenceableBytes(M.getDataLayout(), Temp, Temp);
 }

`getPointerDereferenceableBytes` doesn't fully support `Arguments` and mostly 
returns 0.

I am looking at adding support for arguments in a separate patch, in the 
meantime the tests added in this patch fail because of incorrect 
@.offload_sizes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

2023-03-22 Thread Akash Banerjee via Phabricator via cfe-commits

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

Updated tests. All tests pass now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  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
  mlir/test/Target/LLVMIR/omptarget-llvm.mlir

Index: mlir/test/Target/LLVMIR/omptarget-llvm.mlir
===
--- mlir/test/Target/LLVMIR/omptarget-llvm.mlir
+++ mlir/test/Target/LLVMIR/omptarget-llvm.mlir
@@ -12,43 +12,37 @@
 }
 
 // CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
+// CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4]
 // 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_2:.*]] = alloca i32, i64 1, align 4
+// 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_2]], ptr %[[VAL_5]], align 8
+// CHECK: %[[VAL_6:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: store ptr %[[VAL_2]], ptr %[[VAL_6]], align 8
+// CHECK: %[[VAL_7:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_0]], i32 0, i32 0
+// CHECK: %[[VAL_8:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_1]], i32 0, i32 0
+// CHECK: call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_7]], ptr %[[VAL_8]], ptr @.offload_sizes, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
+// CHECK: store i32 99, ptr %[[VAL_2]], 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: call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %[[VAL_9]], ptr %[[VAL_10]], ptr @.offload_sizes, 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
+llvm.func @_QPopenmp_target_data_region() {
+  %0 = llvm.mlir.constant(1 : i64) : i64
+  %a = llvm.alloca %0 x !llvm.ptr> : (i64) -> !llvm.ptr>
+  omp.target_data   map((from -> %a : !llvm.ptr>)) {
+%2 = llvm.mlir.constant(10 : i32) : i32
 %3 = llvm.mlir.constant(1 : i64) : i64
 %4 = llvm.mlir.constant(1 :

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

25 matches

Site Navigation

Mail list logo

Footer information