subject:"\[PATCH\] D141627\: \[Clang\]\[OpenMP\] Fix the issue that list items in `has_device_addr` are still mapped to the target device"

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-05-25 Thread Shilei Tian via Phabricator via cfe-commits

tianshilei1992 added a comment.

`12d840375d5a81e9ce1050354371c550669de2d7` should fix the case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-05-25 Thread Shilei Tian via Phabricator via cfe-commits

tianshilei1992 added a comment.

In D141627#4374782 , @dyung wrote:

> In D141627#4374757 , 
> @tianshilei1992 wrote:
>
>> In D141627#4374753 , @Northbadge 
>> wrote:
>>
>>> FYI this is failing- 
>>> https://lab.llvm.org/buildbot/#/builders/109/builds/64971/steps/6/logs/FAIL__Clang__bug59160_c
>>
>> Thanks. I pushed a fix in eaf3de6970fc 
>> .
>
> In case you are not already aware, the test is still failing after your fix:
> https://lab.llvm.org/buildbot/#/builders/139/builds/41495

thanks. I’ll fix it soon or I’ll revert it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-05-25 Thread Douglas Yung via Phabricator via cfe-commits

dyung added a comment.

In D141627#4374757 , @tianshilei1992 
wrote:

> In D141627#4374753 , @Northbadge 
> wrote:
>
>> FYI this is failing- 
>> https://lab.llvm.org/buildbot/#/builders/109/builds/64971/steps/6/logs/FAIL__Clang__bug59160_c
>
> Thanks. I pushed a fix in eaf3de6970fc 
> .

In case you are not already aware, the test is still failing after your fix:
https://lab.llvm.org/buildbot/#/builders/139/builds/41495


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-05-25 Thread Shilei Tian via Phabricator via cfe-commits

tianshilei1992 added a comment.

In D141627#4374753 , @Northbadge 
wrote:

> FYI this is failing- 
> https://lab.llvm.org/buildbot/#/builders/109/builds/64971/steps/6/logs/FAIL__Clang__bug59160_c

Thanks. I pushed a fix in eaf3de6970fc 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-05-25 Thread Jin Xin Ng via Phabricator via cfe-commits

Northbadge added a comment.

FYI this is failing- 
https://lab.llvm.org/buildbot/#/builders/109/builds/64971/steps/6/logs/FAIL__Clang__bug59160_c


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-05-25 Thread Shilei Tian via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb8e3077d60de: [Clang][OpenMP] Fix the issue that list items 
in `has_device_addr` are still… (authored by tianshilei1992).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/bug59160.c

Index: clang/test/OpenMP/bug59160.c
===
--- /dev/null
+++ clang/test/OpenMP/bug59160.c
@@ -0,0 +1,175 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --check-globals --include-generated-funcs --replace-value-regex "__omp_offloading_[0-9a-z]+_[0-9a-z]+" --prefix-filecheck-ir-name _
+// RUN: %clang_cc1 -DCK1 -verify -fopenmp -fopenmp-version=51 -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -DCK1 -verify -fopenmp-simd -fopenmp-version=51 -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
+// expected-no-diagnostics
+
+void zoo(void) {
+  short x[10];
+  short *(xp[10]);
+  xp[1] = [0];
+  short **xpp = [0];
+  x[1] = 111;
+#pragma omp target data map(tofrom: xpp[1][1]) use_device_addr(xpp[1][1])
+#pragma omp target has_device_addr(xpp[1][1])
+  {
+xpp[1][1] = 222;
+  }
+}
+//.
+// CHECK: @.offload_sizes = private unnamed_addr constant [2 x i64] [i64 8, i64 2]
+// CHECK: @.offload_maptypes = private unnamed_addr constant [2 x i64] [i64 67, i64 19]
+// CHECK: @0 = private unnamed_addr constant [23 x i8] c"
+// CHECK: @1 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 22, ptr @0 }, align 8
+// CHECK: @.__omp_offloading_34_735f4a3a_zoo_l13.region_id = weak constant i8 0
+// CHECK: @.offload_sizes.1 = private unnamed_addr constant [1 x i64] [i64 8]
+// CHECK: @.offload_maptypes.2 = private unnamed_addr constant [1 x i64] [i64 288]
+// CHECK: @.omp_offloading.entry_name = internal unnamed_addr constant [37 x i8] c"__omp_offloading_34_735f4a3a_zoo_l13\00"
+// CHECK: @.omp_offloading.entry.__omp_offloading_34_735f4a3a_zoo_l13 = weak constant %struct.__tgt_offload_entry { ptr @.__omp_offloading_34_735f4a3a_zoo_l13.region_id, ptr @.omp_offloading.entry_name, i64 0, i32 0, i32 0 }, section "omp_offloading_entries", align 1
+// CHECK: @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 0, ptr @.omp_offloading.requires_reg, ptr null }]
+//.
+// CHECK-LABEL: define {{[^@]+}}@zoo
+// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[X:%.*]] = alloca [10 x i16], align 2
+// CHECK-NEXT:[[XP:%.*]] = alloca [10 x ptr], align 8
+// CHECK-NEXT:[[XPP:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:[[DOTOFFLOAD_BASEPTRS:%.*]] = alloca [2 x ptr], align 8
+// CHECK-NEXT:[[DOTOFFLOAD_PTRS:%.*]] = alloca [2 x ptr], align 8
+// CHECK-NEXT:[[DOTOFFLOAD_MAPPERS:%.*]] = alloca [2 x ptr], align 8
+// CHECK-NEXT:[[DOTOFFLOAD_BASEPTRS7:%.*]] = alloca [1 x ptr], align 8
+// CHECK-NEXT:[[DOTOFFLOAD_PTRS8:%.*]] = alloca [1 x ptr], align 8
+// CHECK-NEXT:[[DOTOFFLOAD_MAPPERS9:%.*]] = alloca [1 x ptr], align 8
+// CHECK-NEXT:[[KERNEL_ARGS:%.*]] = alloca [[STRUCT___TGT_KERNEL_ARGUMENTS:%.*]], align 8
+// CHECK-NEXT:[[ARRAYIDX:%.*]] = getelementptr inbounds [10 x i16], ptr [[X]], i64 0, i64 0
+// CHECK-NEXT:[[ARRAYIDX1:%.*]] = getelementptr inbounds [10 x ptr], ptr [[XP]], i64 0, i64 1
+// CHECK-NEXT:store ptr [[ARRAYIDX]], ptr [[ARRAYIDX1]], align 8
+// CHECK-NEXT:[[ARRAYIDX2:%.*]] = getelementptr inbounds [10 x ptr], ptr [[XP]], i64 0, i64 0
+// CHECK-NEXT:store ptr [[ARRAYIDX2]], ptr [[XPP]], align 8
+// CHECK-NEXT:[[ARRAYIDX3:%.*]] = getelementptr inbounds [10 x i16], ptr [[X]], i64 0, i64 1
+// CHECK-NEXT:store i16 111, ptr [[ARRAYIDX3]], align 2
+// CHECK-NEXT:[[TMP0:%.*]] = load ptr, ptr [[XPP]], align 8
+// CHECK-NEXT:[[TMP1:%.*]] = load ptr, ptr [[XPP]], align 8
+// CHECK-NEXT:[[ARRAYIDX4:%.*]] = getelementptr inbounds ptr, ptr [[TMP1]], i64 1
+// CHECK-NEXT:[[TMP2:%.*]] = load ptr, ptr [[XPP]], align 8
+// CHECK-NEXT:[[ARRAYIDX5:%.*]] = getelementptr inbounds ptr, ptr [[TMP2]], i64 1
+// CHECK-NEXT:[[TMP3:%.*]] = load ptr, ptr [[ARRAYIDX5]], align 8
+// CHECK-NEXT:[[ARRAYIDX6:%.*]] = getelementptr inbounds i16, ptr [[TMP3]], i64 1
+// CHECK-NEXT:[[TMP4:%.*]] = getelementptr inbounds [2 x ptr], ptr [[DOTOFFLOAD_BASEPTRS]], i32 0, i32 0
+// CHECK-NEXT:store ptr [[TMP0]], ptr [[TMP4]], align 8
+// CHECK-NEXT:[[TMP5:%.*]] = getelementptr inbounds [2 x ptr], ptr [[DOTOFFLOAD_PTRS]], i32 0, i32 0
+// CHECK-NEXT:store ptr [[ARRAYIDX4]], ptr [[TMP5]], align 8

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-05-25 Thread Jennifer Yu via Phabricator via cfe-commits

jyu2 accepted this revision.
jyu2 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/D141627/new/

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-05-25 Thread Shilei Tian via Phabricator via cfe-commits

tianshilei1992 updated this revision to Diff 525627.
tianshilei1992 added a comment.

add a test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/bug59160.c

Index: clang/test/OpenMP/bug59160.c
===
--- /dev/null
+++ clang/test/OpenMP/bug59160.c
@@ -0,0 +1,175 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --check-globals --include-generated-funcs --replace-value-regex "__omp_offloading_[0-9a-z]+_[0-9a-z]+" --prefix-filecheck-ir-name _
+// RUN: %clang_cc1 -DCK1 -verify -fopenmp -fopenmp-version=51 -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -DCK1 -verify -fopenmp-simd -fopenmp-version=51 -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
+// expected-no-diagnostics
+
+void zoo(void) {
+  short x[10];
+  short *(xp[10]);
+  xp[1] = [0];
+  short **xpp = [0];
+  x[1] = 111;
+#pragma omp target data map(tofrom: xpp[1][1]) use_device_addr(xpp[1][1])
+#pragma omp target has_device_addr(xpp[1][1])
+  {
+xpp[1][1] = 222;
+  }
+}
+//.
+// CHECK: @.offload_sizes = private unnamed_addr constant [2 x i64] [i64 8, i64 2]
+// CHECK: @.offload_maptypes = private unnamed_addr constant [2 x i64] [i64 67, i64 19]
+// CHECK: @0 = private unnamed_addr constant [23 x i8] c"
+// CHECK: @1 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 22, ptr @0 }, align 8
+// CHECK: @.__omp_offloading_34_735f4a3a_zoo_l13.region_id = weak constant i8 0
+// CHECK: @.offload_sizes.1 = private unnamed_addr constant [1 x i64] [i64 8]
+// CHECK: @.offload_maptypes.2 = private unnamed_addr constant [1 x i64] [i64 288]
+// CHECK: @.omp_offloading.entry_name = internal unnamed_addr constant [37 x i8] c"__omp_offloading_34_735f4a3a_zoo_l13\00"
+// CHECK: @.omp_offloading.entry.__omp_offloading_34_735f4a3a_zoo_l13 = weak constant %struct.__tgt_offload_entry { ptr @.__omp_offloading_34_735f4a3a_zoo_l13.region_id, ptr @.omp_offloading.entry_name, i64 0, i32 0, i32 0 }, section "omp_offloading_entries", align 1
+// CHECK: @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 0, ptr @.omp_offloading.requires_reg, ptr null }]
+//.
+// CHECK-LABEL: define {{[^@]+}}@zoo
+// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[X:%.*]] = alloca [10 x i16], align 2
+// CHECK-NEXT:[[XP:%.*]] = alloca [10 x ptr], align 8
+// CHECK-NEXT:[[XPP:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:[[DOTOFFLOAD_BASEPTRS:%.*]] = alloca [2 x ptr], align 8
+// CHECK-NEXT:[[DOTOFFLOAD_PTRS:%.*]] = alloca [2 x ptr], align 8
+// CHECK-NEXT:[[DOTOFFLOAD_MAPPERS:%.*]] = alloca [2 x ptr], align 8
+// CHECK-NEXT:[[DOTOFFLOAD_BASEPTRS7:%.*]] = alloca [1 x ptr], align 8
+// CHECK-NEXT:[[DOTOFFLOAD_PTRS8:%.*]] = alloca [1 x ptr], align 8
+// CHECK-NEXT:[[DOTOFFLOAD_MAPPERS9:%.*]] = alloca [1 x ptr], align 8
+// CHECK-NEXT:[[KERNEL_ARGS:%.*]] = alloca [[STRUCT___TGT_KERNEL_ARGUMENTS:%.*]], align 8
+// CHECK-NEXT:[[ARRAYIDX:%.*]] = getelementptr inbounds [10 x i16], ptr [[X]], i64 0, i64 0
+// CHECK-NEXT:[[ARRAYIDX1:%.*]] = getelementptr inbounds [10 x ptr], ptr [[XP]], i64 0, i64 1
+// CHECK-NEXT:store ptr [[ARRAYIDX]], ptr [[ARRAYIDX1]], align 8
+// CHECK-NEXT:[[ARRAYIDX2:%.*]] = getelementptr inbounds [10 x ptr], ptr [[XP]], i64 0, i64 0
+// CHECK-NEXT:store ptr [[ARRAYIDX2]], ptr [[XPP]], align 8
+// CHECK-NEXT:[[ARRAYIDX3:%.*]] = getelementptr inbounds [10 x i16], ptr [[X]], i64 0, i64 1
+// CHECK-NEXT:store i16 111, ptr [[ARRAYIDX3]], align 2
+// CHECK-NEXT:[[TMP0:%.*]] = load ptr, ptr [[XPP]], align 8
+// CHECK-NEXT:[[TMP1:%.*]] = load ptr, ptr [[XPP]], align 8
+// CHECK-NEXT:[[ARRAYIDX4:%.*]] = getelementptr inbounds ptr, ptr [[TMP1]], i64 1
+// CHECK-NEXT:[[TMP2:%.*]] = load ptr, ptr [[XPP]], align 8
+// CHECK-NEXT:[[ARRAYIDX5:%.*]] = getelementptr inbounds ptr, ptr [[TMP2]], i64 1
+// CHECK-NEXT:[[TMP3:%.*]] = load ptr, ptr [[ARRAYIDX5]], align 8
+// CHECK-NEXT:[[ARRAYIDX6:%.*]] = getelementptr inbounds i16, ptr [[TMP3]], i64 1
+// CHECK-NEXT:[[TMP4:%.*]] = getelementptr inbounds [2 x ptr], ptr [[DOTOFFLOAD_BASEPTRS]], i32 0, i32 0
+// CHECK-NEXT:store ptr [[TMP0]], ptr [[TMP4]], align 8
+// CHECK-NEXT:[[TMP5:%.*]] = getelementptr inbounds [2 x ptr], ptr [[DOTOFFLOAD_PTRS]], i32 0, i32 0
+// CHECK-NEXT:store ptr [[ARRAYIDX4]], ptr [[TMP5]], align 8
+// CHECK-NEXT:[[TMP6:%.*]] = getelementptr inbounds [2 x ptr], ptr [[DOTOFFLOAD_MAPPERS]], i64 0, i64 0
+// CHECK-NEXT:store ptr null, ptr [[TMP6]], align 8
+//

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-04-27 Thread Jennifer Yu via Phabricator via cfe-commits

jyu2 added a comment.

If it is okay with is_divece_ptr.  I am okay with it.
Please add a lit test for this?

Thanks.
Jennifer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-04-27 Thread Shilei Tian via Phabricator via cfe-commits

tianshilei1992 added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-04-22 Thread Shilei Tian via Phabricator via cfe-commits

tianshilei1992 added a comment.

After reading some threads 
(https://github.com/OpenMP/spec/issues/2178#issue-622053885 and 
https://github.com/OpenMP/spec/issues/1870), I think `has_device_addr` is not 
supposed to have any semantics about creating a mapping, especially in 
`https://github.com/OpenMP/spec/issues/2178#issue-622053885` it is used as a 
equivalent of `firstprivate`.
https://github.com/OpenMP/spec/issues/2178#issuecomment-631751755 mentions 
`is_device_ptr` has to convert the device pointer to device address, while 
`has_device_addr`, and since in LLVM OpenMP implementation device pointer is 
treated as device address, passing them as literal makes sense, so the fix is 
valid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-04-22 Thread Shilei Tian via Phabricator via cfe-commits

tianshilei1992 updated this revision to Diff 516086.
tianshilei1992 added a comment.
Herald added subscribers: jplehr, sunshaoce.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp


Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -8702,9 +8702,7 @@
   CGF.getTypeSize(CGF.getContext().VoidPtrTy), CGF.Int64Ty,
   /*isSigned=*/true));
   CombinedInfo.Types.push_back(
-  (Cap->capturesVariable()
-   ? OpenMPOffloadMappingFlags::OMP_MAP_TO
-   : OpenMPOffloadMappingFlags::OMP_MAP_LITERAL) |
+  OpenMPOffloadMappingFlags::OMP_MAP_LITERAL |
   OpenMPOffloadMappingFlags::OMP_MAP_TARGET_PARAM);
   CombinedInfo.Mappers.push_back(nullptr);
   return;


Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -8702,9 +8702,7 @@
   CGF.getTypeSize(CGF.getContext().VoidPtrTy), CGF.Int64Ty,
   /*isSigned=*/true));
   CombinedInfo.Types.push_back(
-  (Cap->capturesVariable()
-   ? OpenMPOffloadMappingFlags::OMP_MAP_TO
-   : OpenMPOffloadMappingFlags::OMP_MAP_LITERAL) |
+  OpenMPOffloadMappingFlags::OMP_MAP_LITERAL |
   OpenMPOffloadMappingFlags::OMP_MAP_TARGET_PARAM);
   CombinedInfo.Mappers.push_back(nullptr);
   return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-01-13 Thread Shilei Tian via Phabricator via cfe-commits

tianshilei1992 added a comment.

In D141627#4052323 , @abhinavgaba 
wrote:

>> In target data we already put a and b in use_device_addr. That indicates all 
>> use of a and b will be the corresponding device addresses. Therefore, in 
>> target directive, we should use is_device_address instead of 
>> has_device_addr. The correct way to use has_device_addr is, we already map 
>> the list items by using target data w/o use_device_addr. Then when we launch 
>> a kernel using target directive with has_device_addr, we tell the target 
>> region, the list items *should* be there, and use them, otherwise it is an 
>> error (we choose to error out for the undefined behavior).
>
> I think you are talking about `is_device_ptr` clause. There is no 
> `is_device_address` clause in OpenMP.

Oh that's correct. I directly copied from Jennifer's comment. ;-)

> The is_device_ptr clause is meant only for "ptrs" (pointers). For example:
>
>   int *p = omp_target_alloc(...);
>   #pragma omp target is_device_ptr(p)

That's true, but what about the case I mentioned? It is also supposed to use 
`is_device_ptr`.

>> On the other hand, has_device_addr indicates that the list items *should* 
>> have device address, which means there has to be an entry for that.
>
> Based on a brief discussion with some members of the OpenMP spec committee, 
> the idea for "has_device_addr" is to have the address passed-in directly (as 
> a literal, similar to `is_device_ptr`) into the target region, without any 
> map lookup. So, there is no requirement that the variable has to be mapped, 
> or tracked by libomptarget. That requirement is for `map(present:x)`.
> In terms of the code emitted, the original idea of passing the address in as 
> a LITERAL, similar to `is_device_ptr` is the right way to think about it.

No. I think you are mixing things up. The spec says:

> The has_device_addr clause indicates that its list items already have device 
> addresses and therefore they may be directly accessed from a target device.

It only indicates the list items already have device addresses. I don't think 
it has another level of meaning that, the list of variables listed are device 
addresses. The second part above is, "they may be directly accessed from a 
target device". My reading is, they may be directly accessed from a target 
device "without a mapping", which exactly the `map(present:x)` you suggested 
indicates. And yes, we don't need extra flag for that. `present` is exactly we 
need here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-01-13 Thread Abhinav Gaba via Phabricator via cfe-commits

abhinavgaba added subscribers: dreachem, kkwli0.
abhinavgaba added a comment.

> In target data we already put a and b in use_device_addr. That indicates all 
> use of a and b will be the corresponding device addresses. Therefore, in 
> target directive, we should use is_device_address instead of has_device_addr. 
> The correct way to use has_device_addr is, we already map the list items by 
> using target data w/o use_device_addr. Then when we launch a kernel using 
> target directive with has_device_addr, we tell the target region, the list 
> items *should* be there, and use them, otherwise it is an error (we choose to 
> error out for the undefined behavior).

I think you are talking about `is_device_ptr` clause. There is no 
`is_device_address` clause in OpenMP.

The is_device_ptr clause is meant only for "ptrs" (pointers). For example:

  int *p = omp_target_alloc(...);
  #pragma omp target is_device_ptr(p)



> On the other hand, has_device_addr indicates that the list items *should* 
> have device address, which means there has to be an entry for that.

Based on a brief discussion with some members of the OpenMP spec committee, the 
idea for "has_device_addr" is to have the address passed-in directly (as a 
literal, similar to `is_device_ptr`) into the target region, without any map 
lookup. So, there is no requirement that the variable has to be mapped, or 
tracked by libomptarget. That requirement is for `map(present:x)`.

One example use-case from @dreachem is this:

  #pragma omp requires unified_shared_memory
  
  int x ;
  
  printg("%p\n", ); // p1h
  printf("%d\n", omp_target_is_present(, omp_get_default_device())); // 0 (x 
is not "present", as per the OpenMP runtime)
  
  #pragma omp target has_device_addr(x)
  print("%p\n", ); // p1h (same address as on the host side)
  }

In this case, because of unified shared memory, `x` is accessible on device as 
well, even though it is not mapped, or made declare target etc. So we need to 
pass the address of x into the region, even though `omp_target_is_present` 
would return false for it.

In terms of the code emitted, the original idea of passing the address in as a 
LITERAL, similar to `is_device_ptr` is the right way to think about it.

  %x = alloca i32 ; Original allocation for x
  
  Map:
   
  
  Outlined function:
  define void @outlined...(ptr %x) {
 ...
 call i32 @printf(..., ptr %x)
 ...
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-01-13 Thread Shilei Tian via Phabricator via cfe-commits

tianshilei1992 added a comment.

In D141627#4051851 , @jyu2 wrote:

> That part of code is original add for is_device_address, so I just wonder, if 
> the change could break is_device_address?

Now I kinda think it is not right to mix `is_device_address` and 
`has_device_addr`.

Basically, `is_device_address` means the list items are device address, so the 
address should be taken as literal, directly passed to the kernel.

On the other hand, `has_device_addr` indicates that the list items *should* 
have device address, which means there has to be an entry for that. Note that 
it is different from the OpenMP `map` clause. OpenMP's `map` clause (w/o 
`always` of course) means if the list items are not mapped, do it, and transfer 
the data accordingly; otherwise, use the one in the map table. I think 
`has_device_addr` only means map table lookup. Use it if found, otherwise 
undefined behavior (per spec). We are not supposed to update mapping table.

So back this patch, or clang front end, I think the correct way to handle this 
is to create a new flag, indicating the mapping is supposed to exist. The 
runtime needs to be changed accordingly in a way that if the flag is set, it 
should error out if it doesn't find any mapping.

Meanwhile, I think the test case for `has_device_addr` is not correct.

  void xoo() {
short a[10], b[10];
a[1] = 111;
b[1] = 111;
  #pragma omp target data map(to : a[0 : 2], b[0 : 2]) use_device_addr(a, b)
  #pragma omp target has_device_addr(a) has_device_addr(b[0])
{
  a[1] = 222;
  b[1] = 222;
  // CHECK: 222 222
  printf("%hd %hd %p %p %p\n", a[1], b[1], , b, );
}
// CHECK:111
printf("%hd %hd %p %p %p\n", a[1], b[1], , b, ); // 111 111 p1d p2d p3d
  }

In `target data` we already put `a` and `b` in `use_device_addr`. That 
indicates all use of `a` and `b` will be the corresponding device addresses. 
Therefore, in `target` directive, we should use `is_device_address` instead of 
`has_device_addr`. The correct way to use `has_device_addr` is, we already map 
the list items by using `target data` w/o `use_device_addr`. Then when we 
launch a kernel using `target` directive with `has_device_addr`, we tell the 
`target` region, the list items *should* be there, and use them, otherwise it 
is an error (we choose to error out for the undefined behavior).

@jyu2 WDYT?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-01-13 Thread Jennifer Yu via Phabricator via cfe-commits

jyu2 added a comment.

That part of code is original add for is_device_address, so I just wonder, if 
the change could break is_device_address?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-01-12 Thread Abhinav Gaba via Phabricator via cfe-commits

abhinavgaba added a comment.

In D141627#4049085 , @tianshilei1992 
wrote:

> FWIW, I think `has_device_addr(b[0])` is not trying to take the value of 
> `b[0]` in this case. Instead, it's just to take the address of the first 
> element of `b`. Only pointer arithmetic will be involved. It's not 
> necessarily illegal to do it in that way.

That is true. It would be wrong if `b` was a pointer, for which `[0]` 
computation would have involved a load.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-01-12 Thread Shilei Tian via Phabricator via cfe-commits

tianshilei1992 added a comment.

FWIW, I think `has_device_addr(b[0])` is not trying to take the value of `b[0]` 
in this case. Instead, it's just to take the address of the first element of 
`b`. Only pointer arithmetic will be involved. It's not necessarily illegal to 
do it in that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-01-12 Thread Abhinav Gaba via Phabricator via cfe-commits

abhinavgaba added a comment.

In D141627#4048584 , @tianshilei1992 
wrote:

> I agree that b is not right here, but that doesn’t matter because I stepped 
> into the runtime library and it crashed when processing a.
>
> And why are they treated as to?

Treating `has_device_addr(a)` as `map(a)` is incorrect. I think it is just a 
vestige of the prior implementation, where `has_device_addr `was fully ignored 
and instead  `(map(tofrom))` kicked in for `a` (which is the implicit map for 
arrays).

The test likely passed on x86_64 plugin because it just re-mapped the output of 
`use_device_addr(a)`, which is a device address, again, but on architectures 
without unified memory, this re-mapping won't work, hence the failure you see 
with Cuda.

  #include 
  int main() {
  short a[10], b[10];
  a[1] = 111;
  b[1] = 111;
  printf("%hd %hd %p %p\n", a[1], b[1], , ); // 111 111 p1h p2h
#pragma omp target data map(to : a[0 : 2], b[0 : 2]) use_device_addr(a, b)
  {
  printf("%p %p\n", , ); // p1d p2d
#pragma omp target has_device_addr(a) has_device_addr(b)
  {
a[1] = 222;
b[1] = 222;
printf("%hd %hd %p %p\n", a[1], b[1], , ); // 222 222 p1d p2d
  }
  }
  // CHECK:111
  printf("%hd %hd %p %p\n", a[1], b[1], , ); // 111 111 p1h p2h
}



  $ clang -O0 -fopenmp -fopenmp-targets=x86_64 hda_test.c -fopenmp-version=51 
&& ./a.out
  111 111 0x7fff2a47ecb0 0x7fff2a47ec90 // 111 111 p1h p2h
  0x55f3cf685b10 0x55f3cf685c10 // p1d p2d: device versions of p1h, p2h
  222 222 0x55f3cf685d70 0x55f3cf685e70 // p1dd p2dd:  another different device 
version of the two, because of tthe remapping. These should have been "p1d p2d".
  111 111 0x7fff2a47ecb0 0x7fff2a47ec90 // 111 111 p1h p2h

  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-01-12 Thread Shilei Tian via Phabricator via cfe-commits

tianshilei1992 added a comment.

I agree that b is not right here, but that doesn’t matter because I stepped 
into the runtime library and it crashed when processing a.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-01-12 Thread Abhinav Gaba via Phabricator via cfe-commits

abhinavgaba added a comment.

In D134268#4048357 , @tianshilei1992 
wrote:

> Hi there, I'm trying to fix 
> https://github.com/llvm/llvm-project/issues/59160. The faulty case is 
> basically like the following:
>
>   void xoo() {
> short a[10], b[10];
> a[1] = 111;
> b[1] = 111;
>   #pragma omp target data map(to : a[0 : 2], b[0 : 2]) use_device_addr(a, b)
>   #pragma omp target has_device_addr(a) has_device_addr(b[0])
> {
>   a[1] = 222;
>   b[1] = 222;
>   // CHECK: 222 222
>   printf("%hd %hd %p %p %p\n", a[1], b[1], , b, );
> }
> // CHECK:111
> printf("%hd %hd %p %p %p\n", a[1], b[1], , b, ); // 111 111 p1d p2d 
> p3d
>   }
>
> It looks like at runtime, we are trying to copy a (device) pointer to a 
> device pointer by using host to device data transfer. I noticed that's 
> because we have `TO` flag marked for the argument. However, since `a` and `b` 
> are in `has_device_addr`, we are not supposed to map the two variables right?

The firstt thing is that in the test itself, `has_device_addr(b[0])` is 
incorrect. `b` inside the "target data" region refers to the device version of 
`b`, not the host version. So, it is illegal to do `b[0]` (without unified 
shared memory) because we cannot load from device `b` on the host (target data 
is executed on host).

After that is changed to `has_device_addr(b)`, the test will likely pass.

However, it is still true that `has_device_addr(a, b)` are being treated the 
same as `map(to:a, b)` for arrays, instead of passing the addresses ,  
directly into the kernel as LITERALs. I think @jyu2 was working on changing 
that, so she might be able to say what changes are needed in clang for that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141627

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

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

2023-01-12 Thread Shilei Tian via Phabricator via cfe-commits

tianshilei1992 created this revision.
tianshilei1992 added reviewers: ABataev, jdoerfert, abhinavgaba, jyu2.
Herald added subscribers: guansong, yaxunl.
Herald added a project: All.
tianshilei1992 requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

This patch fixes the issue that list items in `has_device_addr` are still mapped
to the target device because front end emits map type `OMP_MAP_TO`.

Fix #59160.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141627

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/target_is_device_ptr_codegen.cpp


Index: clang/test/OpenMP/target_is_device_ptr_codegen.cpp
===
--- clang/test/OpenMP/target_is_device_ptr_codegen.cpp
+++ clang/test/OpenMP/target_is_device_ptr_codegen.cpp
@@ -321,7 +321,7 @@
 
 // CK3-DAG: [[SIZES:@.+]] = {{.+}}constant [1 x i[[SZ:64|32]]] [i{{64|32}} 
{{8|4}}]
 // OMP_MAP_TARGET_PARAM = 0x20 | OMP_MAP_TO = 0x1 = 0x21
-// CK3-DAG: [[TYPES:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x21]]]
+// CK3-DAG: [[TYPES:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x120]]]
 void bar() {
   __attribute__((aligned(64))) double *ptr;
   // CK3-DAG: [[RET:%.+]] = call i32 @__tgt_target_kernel(ptr @{{.+}}, i64 
[[DEVICE:.+]], i32 -1, i32 0, ptr @.{{.+}}.region_id, ptr [[ARGS:%.+]])
Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -8679,9 +8679,7 @@
   CGF.getTypeSize(CGF.getContext().VoidPtrTy), CGF.Int64Ty,
   /*isSigned=*/true));
   CombinedInfo.Types.push_back(
-  (Cap->capturesVariable()
-   ? OpenMPOffloadMappingFlags::OMP_MAP_TO
-   : OpenMPOffloadMappingFlags::OMP_MAP_LITERAL) |
+  OpenMPOffloadMappingFlags::OMP_MAP_LITERAL |
   OpenMPOffloadMappingFlags::OMP_MAP_TARGET_PARAM);
   CombinedInfo.Mappers.push_back(nullptr);
   return;


Index: clang/test/OpenMP/target_is_device_ptr_codegen.cpp
===
--- clang/test/OpenMP/target_is_device_ptr_codegen.cpp
+++ clang/test/OpenMP/target_is_device_ptr_codegen.cpp
@@ -321,7 +321,7 @@
 
 // CK3-DAG: [[SIZES:@.+]] = {{.+}}constant [1 x i[[SZ:64|32]]] [i{{64|32}} {{8|4}}]
 // OMP_MAP_TARGET_PARAM = 0x20 | OMP_MAP_TO = 0x1 = 0x21
-// CK3-DAG: [[TYPES:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x21]]]
+// CK3-DAG: [[TYPES:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x120]]]
 void bar() {
   __attribute__((aligned(64))) double *ptr;
   // CK3-DAG: [[RET:%.+]] = call i32 @__tgt_target_kernel(ptr @{{.+}}, i64 [[DEVICE:.+]], i32 -1, i32 0, ptr @.{{.+}}.region_id, ptr [[ARGS:%.+]])
Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -8679,9 +8679,7 @@
   CGF.getTypeSize(CGF.getContext().VoidPtrTy), CGF.Int64Ty,
   /*isSigned=*/true));
   CombinedInfo.Types.push_back(
-  (Cap->capturesVariable()
-   ? OpenMPOffloadMappingFlags::OMP_MAP_TO
-   : OpenMPOffloadMappingFlags::OMP_MAP_LITERAL) |
+  OpenMPOffloadMappingFlags::OMP_MAP_LITERAL |
   OpenMPOffloadMappingFlags::OMP_MAP_TARGET_PARAM);
   CombinedInfo.Mappers.push_back(nullptr);
   return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

[PATCH] D141627: [Clang][OpenMP] Fix the issue that list items in `has_device_addr` are still mapped to the target device

22 matches

Site Navigation

Mail list logo

Footer information