[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Joel E. Denny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG002d61db2b77: [OpenMP] Fix `present` for exit from `omp 
target data` (authored by jdenny).

Changed prior to commit:
  https://reviews.llvm.org/D84422?vs=281330=283224#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.h
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  openmp/libomptarget/src/omptarget.cpp
  openmp/libomptarget/test/mapping/present/target_data_at_exit.c

Index: openmp/libomptarget/test/mapping/present/target_data_at_exit.c
===
--- /dev/null
+++ openmp/libomptarget/test/mapping/present/target_data_at_exit.c
@@ -0,0 +1,37 @@
+// RUN: %libomptarget-compile-aarch64-unknown-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-aarch64-unknown-linux-gnu 2>&1 \
+// RUN: | %fcheck-aarch64-unknown-linux-gnu
+
+// RUN: %libomptarget-compile-powerpc64-ibm-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-powerpc64-ibm-linux-gnu 2>&1 \
+// RUN: | %fcheck-powerpc64-ibm-linux-gnu
+
+// RUN: %libomptarget-compile-powerpc64le-ibm-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-powerpc64le-ibm-linux-gnu 2>&1 \
+// RUN: | %fcheck-powerpc64le-ibm-linux-gnu
+
+// RUN: %libomptarget-compile-x86_64-pc-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-x86_64-pc-linux-gnu 2>&1 \
+// RUN: | %fcheck-x86_64-pc-linux-gnu
+
+#include 
+
+int main() {
+  int i;
+
+#pragma omp target enter data map(alloc:i)
+
+  // i isn't present at the end of the target data region, but the "present"
+  // modifier is only checked at the beginning of a region.
+#pragma omp target data map(present, alloc: i)
+  {
+#pragma omp target exit data map(delete:i)
+  }
+
+  // CHECK-NOT: Libomptarget
+  // CHECK: success
+  // CHECK-NOT: Libomptarget
+  fprintf(stderr, "success\n");
+
+  return 0;
+}
Index: openmp/libomptarget/src/omptarget.cpp
===
--- openmp/libomptarget/src/omptarget.cpp
+++ openmp/libomptarget/src/omptarget.cpp
@@ -506,8 +506,14 @@
   DP("Mapping does not exist (%s)\n",
  (HasPresentModifier ? "'present' map type modifier" : "ignored"));
   if (HasPresentModifier) {
-// FIXME: This should not be an error on exit from "omp target data",
-// but it should be an error upon entering an "omp target exit data".
+// This should be an error upon entering an "omp target exit data".  It
+// should not be an error upon exiting an "omp target data" or "omp
+// target".  For "omp target data", Clang thus doesn't include present
+// modifiers for end calls.  For "omp target", we have not found a valid
+// OpenMP program for which the error matters: it appears that, if a
+// program can guarantee that data is present at the beginning of an
+// "omp target" region so that there's no error there, that data is also
+// guaranteed to be present at the end.
 MESSAGE("device mapping required by 'present' map type modifier does "
 "not exist for host address " DPxMOD " (%ld bytes)",
 DPxPTR(HstPtrBegin), DataSize);
Index: clang/test/OpenMP/target_data_codegen.cpp
===
--- clang/test/OpenMP/target_data_codegen.cpp
+++ clang/test/OpenMP/target_data_codegen.cpp
@@ -256,10 +256,16 @@
 double gc[100];
 
 // PRESENT=0x1000 | TARGET_PARAM=0x20 | TO=0x1 = 0x1021
-// CK1A: [[MTYPE00:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+// CK1A: [[MTYPE00Begin:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+
+// TARGET_PARAM=0x20 | TO=0x1 = 0x21
+// CK1A: [[MTYPE00End:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x21]]]
 
 // PRESENT=0x1000 | CLOSE=0x400 | TARGET_PARAM=0x20 | ALWAYS=0x4 | TO=0x1 = 0x1425
-// CK1A: [[MTYPE01:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]
+// CK1A: [[MTYPE01Begin:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]
+
+// CLOSE=0x400 | TARGET_PARAM=0x20 | ALWAYS=0x4 | TO=0x1 = 0x425
+// CK1A: [[MTYPE01End:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x425]]]
 
 // CK1A-LABEL: _Z3fooi
 void foo(int arg) {
@@ -267,7 +273,7 @@
   float lb[arg];
 
   // Region 00
-  // CK1A-DAG: call void @__tgt_target_data_begin_mapper(i64 -1, i32 1, i8** [[GEPBP:%.+]], i8** [[GEPP:%.+]], i[[sz:32|64]]* [[GEPS:%.+]], {{.+}}getelementptr {{.+}}[1 x i{{.+}}]* [[MTYPE00]]{{.+}})
+  // CK1A-DAG: call void @__tgt_target_data_begin_mapper(i64 -1, i32 1, i8** [[GEPBP:%.+]], i8** [[GEPP:%.+]], i[[sz:32|64]]* [[GEPS:%.+]], {{.+}}getelementptr {{.+}}[1 x i{{.+}}]* [[MTYPE00Begin]]{{.+}})
   // CK1A-DAG: [[GEPBP]] = getelementptr inbounds {{.+}}[[BP:%[^,]+]]
   // CK1A-DAG: [[GEPP]] = getelementptr inbounds 

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked an inline comment as done.
jdenny added a comment.

Thanks for the review.

As discussed during the 7/29 call, I'll wait to push until we're sure about 
what the OpenMP committee intended here.  I'm pursuing this and will report 
back when I have more information.


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

https://reviews.llvm.org/D84422

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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Alexey Bataev via Phabricator via cfe-commits]

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

LG


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

https://reviews.llvm.org/D84422

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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Joel E. Denny via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8843
 llvm::Value *, llvm::Value *,
-CGOpenMPRuntime::TargetDataInfo ) {
+CGOpenMPRuntime::TargetDataInfo , bool ForEndCall = false) {
+  assert((!ForEndCall || Info.separateBeginEndCalls()) &&

ABataev wrote:
> Do not append param here, use the one from `Info`
`Info.SeparateBeginEndCalls` and `ForEndCall` do not represent the same thing.  
If `Info.SeparateBeginEndCalls=true`, as in `emitTargetDataCalls` below, then 
`emitOffloadingArraysArgument` is called twice with the same `Info`, once with 
`ForEndCall=false` and once with `ForEndCall=true`.


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

https://reviews.llvm.org/D84422

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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8843
 llvm::Value *, llvm::Value *,
-CGOpenMPRuntime::TargetDataInfo ) {
+CGOpenMPRuntime::TargetDataInfo , bool ForEndCall = false) {
+  assert((!ForEndCall || Info.separateBeginEndCalls()) &&

Do not append param here, use the one from `Info`


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

https://reviews.llvm.org/D84422

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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 281330.
jdenny added a comment.

Replaced `SeparateBeginEnd` parameter with new `TargetDataInfo` field as 
requested.  Rebased.


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

https://reviews.llvm.org/D84422

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.h
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  openmp/libomptarget/src/omptarget.cpp
  openmp/libomptarget/test/mapping/present/target_data_at_exit.c

Index: openmp/libomptarget/test/mapping/present/target_data_at_exit.c
===
--- /dev/null
+++ openmp/libomptarget/test/mapping/present/target_data_at_exit.c
@@ -0,0 +1,37 @@
+// RUN: %libomptarget-compile-aarch64-unknown-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-aarch64-unknown-linux-gnu 2>&1 \
+// RUN: | %fcheck-aarch64-unknown-linux-gnu
+
+// RUN: %libomptarget-compile-powerpc64-ibm-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-powerpc64-ibm-linux-gnu 2>&1 \
+// RUN: | %fcheck-powerpc64-ibm-linux-gnu
+
+// RUN: %libomptarget-compile-powerpc64le-ibm-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-powerpc64le-ibm-linux-gnu 2>&1 \
+// RUN: | %fcheck-powerpc64le-ibm-linux-gnu
+
+// RUN: %libomptarget-compile-x86_64-pc-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-x86_64-pc-linux-gnu 2>&1 \
+// RUN: | %fcheck-x86_64-pc-linux-gnu
+
+#include 
+
+int main() {
+  int i;
+
+#pragma omp target enter data map(alloc:i)
+
+  // i isn't present at the end of the target data region, but the "present"
+  // modifier is only checked at the beginning of a region.
+#pragma omp target data map(present, alloc: i)
+  {
+#pragma omp target exit data map(delete:i)
+  }
+
+  // CHECK-NOT: Libomptarget
+  // CHECK: success
+  // CHECK-NOT: Libomptarget
+  fprintf(stderr, "success\n");
+
+  return 0;
+}
Index: openmp/libomptarget/src/omptarget.cpp
===
--- openmp/libomptarget/src/omptarget.cpp
+++ openmp/libomptarget/src/omptarget.cpp
@@ -484,8 +484,14 @@
   DP("Mapping does not exist (%s)\n",
  (HasPresentModifier ? "'present' map type modifier" : "ignored"));
   if (HasPresentModifier) {
-// FIXME: This should not be an error on exit from "omp target data",
-// but it should be an error upon entering an "omp target exit data".
+// This should be an error upon entering an "omp target exit data".  It
+// should not be an error upon exiting an "omp target data" or "omp
+// target".  For "omp target data", Clang thus doesn't include present
+// modifiers for end calls.  For "omp target", we have not found a valid
+// OpenMP program for which the error matters: it appears that, if a
+// program can guarantee that data is present at the beginning of an
+// "omp target" region so that there's no error there, that data is also
+// guaranteed to be present at the end.
 MESSAGE("device mapping required by 'present' map type modifier does "
 "not exist for host address " DPxMOD " (%ld bytes)",
 DPxPTR(HstPtrBegin), data_size);
Index: clang/test/OpenMP/target_data_codegen.cpp
===
--- clang/test/OpenMP/target_data_codegen.cpp
+++ clang/test/OpenMP/target_data_codegen.cpp
@@ -256,10 +256,16 @@
 double gc[100];
 
 // PRESENT=0x1000 | TARGET_PARAM=0x20 | TO=0x1 = 0x1021
-// CK1A: [[MTYPE00:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+// CK1A: [[MTYPE00Begin:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+
+// TARGET_PARAM=0x20 | TO=0x1 = 0x21
+// CK1A: [[MTYPE00End:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x21]]]
 
 // PRESENT=0x1000 | CLOSE=0x400 | TARGET_PARAM=0x20 | ALWAYS=0x4 | TO=0x1 = 0x1425
-// CK1A: [[MTYPE01:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]
+// CK1A: [[MTYPE01Begin:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]
+
+// CLOSE=0x400 | TARGET_PARAM=0x20 | ALWAYS=0x4 | TO=0x1 = 0x425
+// CK1A: [[MTYPE01End:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x425]]]
 
 // CK1A-LABEL: _Z3fooi
 void foo(int arg) {
@@ -267,7 +273,7 @@
   float lb[arg];
 
   // Region 00
-  // CK1A-DAG: call void @__tgt_target_data_begin_mapper(i64 -1, i32 1, i8** [[GEPBP:%.+]], i8** [[GEPP:%.+]], i[[sz:32|64]]* [[GEPS:%.+]], {{.+}}getelementptr {{.+}}[1 x i{{.+}}]* [[MTYPE00]]{{.+}})
+  // CK1A-DAG: call void @__tgt_target_data_begin_mapper(i64 -1, i32 1, i8** [[GEPBP:%.+]], i8** [[GEPP:%.+]], i[[sz:32|64]]* [[GEPS:%.+]], {{.+}}getelementptr {{.+}}[1 x i{{.+}}]* [[MTYPE00Begin]]{{.+}})
   // CK1A-DAG: [[GEPBP]] = getelementptr inbounds {{.+}}[[BP:%[^,]+]]
   // CK1A-DAG: [[GEPP]] = getelementptr inbounds {{.+}}[[P:%[^,]+]]
   // CK1A-DAG: [[GEPS]] = getelementptr inbounds {{.+}}[[S:%[^,]+]]
@@ -285,7 +291,7 @@
   // CK1A-32-DAG: [[CSVAL032]] = mul 

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8686
+CodeGenFunction , MappableExprsHandler::MapCombinedInfoTy 
,
+CGOpenMPRuntime::TargetDataInfo , bool SeparateBeginEnd) {
   CodeGenModule  = CGF.CGM;

Can this new flag be encapsulated in `Info`?


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

https://reviews.llvm.org/D84422

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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[George Rokos via Phabricator via cfe-commits]

grokos added a comment.

This looks much better now. I don't have any other comments. Since this patch 
is now essentially a clang-only patch, I'll let @ABataev accept it or post 
comments.


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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 281067.
jdenny edited the summary of this revision.
jdenny added a comment.

Rewrite patch as discussed: instead of generating different runtime calls for 
the end of an `omp target data` vs. the beginning of an `omp target exit data` 
so that the runtime can determine when to ignore `present`, change Clang to 
filter `present` from the map types at the end of an `omp target data`.


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

https://reviews.llvm.org/D84422

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.h
  clang/test/OpenMP/target_data_codegen.cpp
  openmp/libomptarget/src/omptarget.cpp
  openmp/libomptarget/test/mapping/present/target_data_at_exit.c

Index: openmp/libomptarget/test/mapping/present/target_data_at_exit.c
===
--- /dev/null
+++ openmp/libomptarget/test/mapping/present/target_data_at_exit.c
@@ -0,0 +1,37 @@
+// RUN: %libomptarget-compile-aarch64-unknown-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-aarch64-unknown-linux-gnu 2>&1 \
+// RUN: | %fcheck-aarch64-unknown-linux-gnu
+
+// RUN: %libomptarget-compile-powerpc64-ibm-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-powerpc64-ibm-linux-gnu 2>&1 \
+// RUN: | %fcheck-powerpc64-ibm-linux-gnu
+
+// RUN: %libomptarget-compile-powerpc64le-ibm-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-powerpc64le-ibm-linux-gnu 2>&1 \
+// RUN: | %fcheck-powerpc64le-ibm-linux-gnu
+
+// RUN: %libomptarget-compile-x86_64-pc-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-x86_64-pc-linux-gnu 2>&1 \
+// RUN: | %fcheck-x86_64-pc-linux-gnu
+
+#include 
+
+int main() {
+  int i;
+
+#pragma omp target enter data map(alloc:i)
+
+  // i isn't present at the end of the target data region, but the "present"
+  // modifier is only checked at the beginning of a region.
+#pragma omp target data map(present, alloc: i)
+  {
+#pragma omp target exit data map(delete:i)
+  }
+
+  // CHECK-NOT: Libomptarget
+  // CHECK: success
+  // CHECK-NOT: Libomptarget
+  fprintf(stderr, "success\n");
+
+  return 0;
+}
Index: openmp/libomptarget/src/omptarget.cpp
===
--- openmp/libomptarget/src/omptarget.cpp
+++ openmp/libomptarget/src/omptarget.cpp
@@ -484,8 +484,14 @@
   DP("Mapping does not exist (%s)\n",
  (HasPresentModifier ? "'present' map type modifier" : "ignored"));
   if (HasPresentModifier) {
-// FIXME: This should not be an error on exit from "omp target data",
-// but it should be an error upon entering an "omp target exit data".
+// This should be an error upon entering an "omp target exit data".  It
+// should not be an error upon exiting an "omp target data" or "omp
+// target".  For "omp target data", Clang thus doesn't include present
+// modifiers for end calls.  For "omp target", we have not found a valid
+// OpenMP program for which the error matters: it appears that, if a
+// program can guarantee that data is present at the beginning of an
+// "omp target" region so that there's no error there, that data is also
+// guaranteed to be present at the end.
 MESSAGE("device mapping required by 'present' map type modifier does "
 "not exist for host address " DPxMOD " (%ld bytes)",
 DPxPTR(HstPtrBegin), data_size);
Index: clang/test/OpenMP/target_data_codegen.cpp
===
--- clang/test/OpenMP/target_data_codegen.cpp
+++ clang/test/OpenMP/target_data_codegen.cpp
@@ -256,10 +256,16 @@
 double gc[100];
 
 // PRESENT=0x1000 | TARGET_PARAM=0x20 | TO=0x1 = 0x1021
-// CK1A: [[MTYPE00:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+// CK1A: [[MTYPE00Begin:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+
+// TARGET_PARAM=0x20 | TO=0x1 = 0x21
+// CK1A: [[MTYPE00End:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x21]]]
 
 // PRESENT=0x1000 | CLOSE=0x400 | TARGET_PARAM=0x20 | ALWAYS=0x4 | TO=0x1 = 0x1425
-// CK1A: [[MTYPE01:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]
+// CK1A: [[MTYPE01Begin:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]
+
+// CLOSE=0x400 | TARGET_PARAM=0x20 | ALWAYS=0x4 | TO=0x1 = 0x425
+// CK1A: [[MTYPE01End:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x425]]]
 
 // CK1A-LABEL: _Z3fooi
 void foo(int arg) {
@@ -267,7 +273,7 @@
   float lb[arg];
 
   // Region 00
-  // CK1A-DAG: call void @__tgt_target_data_begin_mapper(i64 -1, i32 1, i8** [[GEPBP:%.+]], i8** [[GEPP:%.+]], i[[sz:32|64]]* [[GEPS:%.+]], {{.+}}getelementptr {{.+}}[1 x i{{.+}}]* [[MTYPE00]]{{.+}})
+  // CK1A-DAG: call void @__tgt_target_data_begin_mapper(i64 -1, i32 1, i8** [[GEPBP:%.+]], i8** [[GEPP:%.+]], i[[sz:32|64]]* [[GEPS:%.+]], {{.+}}getelementptr {{.+}}[1 x i{{.+}}]* [[MTYPE00Begin]]{{.+}})
   // CK1A-DAG: [[GEPBP]] = getelementptr 

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D84422#2176802 , @grokos wrote:

> In D84422#2173500 , @jdenny wrote:
>
> > I've added a comment to the runtime code that performs the check.  As you 
> > can see, the check is performed regardless.  It's just a question of 
> > whether the runtime treats it as an error.  I don't think performance is an 
> > issue.
> >
> > My concern here is that it will be hard to justify changes to the runtime 
> > if I cannot formulate a use case.
>
>
> Thinking about it, I don't think there can be a case where something is 
> present upon entering a target region and not be present when we're exiting. 
> Whatever code comprises the target region is code executed on the device - it 
> cannot modify the state of host objects (i.e. libomptarget) in any possible 
> way. E.g. the kernel cannot invoke libomptarget functions, allocate memory, 
> map/unmap data etc.
>
> The only case where something like this would be possible is if we have 
> multiple host threads executing async offloading. In such a case, one thread 
> may launch a target region at a moment when the requested mapping is 
> `present` on the device and while the kernel is executing some other thread 
> performs a `target data exit` on the desired mapping. Upon exiting the 
> kernel, the mapping will no longer be present but this is clearly a race 
> condition (user's fault), so I don't think we should pay attention to such a 
> scenario.


Exactly.  As far as I can tell, the runtime simply needs a comment that 
explains this issue in the case of `omp target`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[George Rokos via Phabricator via cfe-commits]

grokos added a comment.

In D84422#2173500 , @jdenny wrote:

> I've added a comment to the runtime code that performs the check.  As you can 
> see, the check is performed regardless.  It's just a question of whether the 
> runtime treats it as an error.  I don't think performance is an issue.
>
> My concern here is that it will be hard to justify changes to the runtime if 
> I cannot formulate a use case.


Thinking about it, I don't think there can be a case where something is present 
upon entering a target region and not be present when we're exiting. Whatever 
code comprises the target region is code executed on the device - it cannot 
modify the state of host objects (i.e. libomptarget) in any possible way. E.g. 
the kernel cannot invoke libomptarget functions, allocate memory, map/unmap 
data etc.

The only case where something like this would be possible is if we have 
multiple host threads executing async offloading. In such a case, one thread 
may launch a target region at a moment when the requested mapping is `present` 
on the device and while the kernel is executing some other thread performs a 
`target data exit` on the desired mapping. Upon exiting the kernel, the mapping 
will no longer be present but this is clearly a race condition (user's fault), 
so I don't think we should pay attention to such a scenario.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked an inline comment as done.
jdenny added a comment.

In D84422#2173449 , @RaviNarayanaswamy 
wrote:

> In D84422#2173372 , @jdenny wrote:
>
> > In D84422#2172898 , @jdenny wrote:
> >
> > > Has anyone clarified the motivation for this behavior?
> >
> >
> > I meant, is there any insight into why the spec specifies this behavior?
> >
> > In D84422#2172926 , @grokos wrote:
> >
> > > Instead of introducing new API functions and making all these changes in 
> > > all these files, wouldn't it be easier if we just unset the `PRESENT` 
> > > flag from arg_types in clang when we generate the call to 
> > > `__tgt_target_data_end_*` if we are exiting from a scoped environment?
> >
> >
> > Ah, that does sound simpler.  Thanks.  I'll look into it.
> >
> > Suppressing the presence check on exit from `omp target` would require a 
> > runtime change in addition to the Clang change you suggest for `omp target 
> > data`.  However, I've so far failed to formulate a reasonable test case.  
> > Specifically, I don't yet see a way to guarantee that the data will 
> > definitely be present at the start of `omp target` but might not be present 
> > by the end.  Is it possible?  If not, then maybe we should leave the check 
> > in place for `omp target`.
>
>
> I would rather not have a check if not required by the spec as it would an 
> unnecessary overhead to performance.


I've added a comment to the runtime code that performs the check.  As you can 
see, the check is performed regardless.  It's just a question of whether the 
runtime treats it as an error.  I don't think performance is an issue.

My concern here is that it will be hard to justify changes to the runtime if I 
cannot formulate a use case.




Comment at: openmp/libomptarget/src/omptarget.cpp:511
+  // "omp target exit data" but not upon exiting an "omp target data".
+  if (HasPresentModifier && for_exit_data) {
 MESSAGE("device mapping required by 'present' map type modifier does "

This is where the runtime performs the check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

In D84422#2173372 , @jdenny wrote:

> In D84422#2172898 , @jdenny wrote:
>
> > Has anyone clarified the motivation for this behavior?
>
>
> I meant, is there any insight into why the spec specifies this behavior?
>
> In D84422#2172926 , @grokos wrote:
>
> > Instead of introducing new API functions and making all these changes in 
> > all these files, wouldn't it be easier if we just unset the `PRESENT` flag 
> > from arg_types in clang when we generate the call to 
> > `__tgt_target_data_end_*` if we are exiting from a scoped environment?
>
>
> Ah, that does sound simpler.  Thanks.  I'll look into it.
>
> Suppressing the presence check on exit from `omp target` would require a 
> runtime change in addition to the Clang change you suggest for `omp target 
> data`.  However, I've so far failed to formulate a reasonable test case.  
> Specifically, I don't yet see a way to guarantee that the data will 
> definitely be present at the start of `omp target` but might not be present 
> by the end.  Is it possible?  If not, then maybe we should leave the check in 
> place for `omp target`.


I would rather not have a check if not required by the spec as it would an 
unnecessary overhead to performance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D84422#2172898 , @jdenny wrote:

> Has anyone clarified the motivation for this behavior?


I meant, is there any insight into why the spec specifies this behavior?

In D84422#2172926 , @grokos wrote:

> Instead of introducing new API functions and making all these changes in all 
> these files, wouldn't it be easier if we just unset the `PRESENT` flag from 
> arg_types in clang when we generate the call to `__tgt_target_data_end_*` if 
> we are exiting from a scoped environment?


Ah, that does sound simpler.  Thanks.  I'll look into it.

Suppressing the presence check on exit from `omp target` would require a 
runtime change in addition to the Clang change you suggest for `omp target 
data`.  However, I've so far failed to formulate a reasonable test case.  
Specifically, I don't yet see a way to guarantee that the data will definitely 
be present at the start of `omp target` but might not be present by the end.  
Is it possible?  If not, then maybe we should leave the check in place for `omp 
target`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[George Rokos via Phabricator via cfe-commits]

grokos added a comment.

So let's proceed with the patch.

Instead of introducing new API functions and making all these changes in all 
these files, wouldn't it be easier if we just unset the `PRESENT` flag from 
arg_types in clang when we generate the call to `__tgt_target_data_end_*` if we 
are exiting from a scoped environment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D84422#2170702 , @RaviNarayanaswamy 
wrote:

> In D84422#2170667 , @grokos wrote:
>
> > So is the test case that motivated this patch illegal OpenMP code?
> >
> >   #pragma omp target enter data map(alloc:i)
> >   #pragma omp target data map(present, alloc: i)
> >   {
> > #pragma omp target exit data map(delete:i) // you cannot delete that 
> > object in the scope, illegal code?
> >   } // fails presence check here
>
>
> According to spec the test should work.  ie should not check for presence on 
> exit from a blocked openmp pragma scope.


It sounds like this patch's motivation is correct then.  Has anyone clarified 
the motivation for this behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

In D84422#2170667 , @grokos wrote:

> So is the test case that motivated this patch illegal OpenMP code?
>
>   #pragma omp target enter data map(alloc:i)
>   #pragma omp target data map(present, alloc: i)
>   {
> #pragma omp target exit data map(delete:i) // you cannot delete that 
> object in the scope, illegal code?
>   } // fails presence check here
>   ```Sent mail to OpenMP to clarify.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[George Rokos via Phabricator via cfe-commits]

grokos added a comment.

So is the test case that motivated this patch illegal OpenMP code?

  #pragma omp target enter data map(alloc:i)
  #pragma omp target data map(present, alloc: i)
  {
#pragma omp target exit data map(delete:i)
  } // fails presence check here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

In D84422#2170285 , @grokos wrote:

> What confuses me about this interpretation of the standard is the 
> inconsistency at `data exit`. So if we have an explicit `omp target exit data 
> map(present...)` then we should respect the "present" semantics, whereas when 
> we have a scoped data exit:
>
>   #pragma omp target data map(present,...)
>   {
> ...
>   } // implicit "exit data" here
>
>
> then "present" should be ignored.
>
> I agree that the paragraph from the standard leaves little room for other 
> interpretations, I'd just like to point out that it looks inconsistent - at 
> least to me.


When you  use present on a  variable on a scoped target data region,  you  
cannot delete that object in the scope.  I would say this  is a test case 
error.  It should still be present on exit, checking for that is maybe redundant


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

I don't know if the OpenMP committee has any documented rationale for this 
behavior.  I can say that the OpenACC committee is considering the same 
semantics.  However, the issues to consider are not identical.  For example, 
OpenACC has a separate structured reference counter, meaning it should be 
impossible for such data not to be present at the exit of a data construct 
unless you've shut down the runtime.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[George Rokos via Phabricator via cfe-commits]

grokos added a comment.

What confuses me about this interpretation of the standard is the inconsistency 
at `data exit`. So if we have an explicit `omp target exit data 
map(present...)` then we should respect the "present" semantics, whereas when 
we have a scoped data exit:

  #pragma omp target data map(present,...)
  {
...
  } // implicit "exit data" here

then "present" should be ignored.

I agree that the paragraph from the standard leaves little room for other 
interpretations, I'd just like to point out that it looks inconsistent - at 
least to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



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

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: grokos, ABataev, jdoerfert.
Herald added subscribers: llvm-commits, openmp-commits, cfe-commits, sstefan1, 
guansong, yaxunl.
Herald added projects: clang, OpenMP, LLVM.

Without this patch, the following example fails but shouldn't
according to my read of OpenMP TR8:

  #pragma omp target enter data map(alloc:i)
  #pragma omp target data map(present, alloc: i)
  {
#pragma omp target exit data map(delete:i)
  } // fails presence check here

OpenMP TR8 sec. 2.22.7.1 "map Clause", p. 321, L23-26 states:

> If the map clause appears on a target, target data, target enter
>  data or target exit data construct with a present map-type-modifier
>  then on entry to the region if the corresponding list item does not 
>  appear in the device data environment an error occurs and the 
>  program terminates.

I see no corresponding statement about the exit from a region.  Thus,
the `present` modifier should:

1. Check for presence upon entry into a `target exit data` construct.
2. Should not check for presence upon exit from a `target data` region, as in 
the above example.

The problem is that Clang calls the same set of
`__tgt_target_data_end_*` functions for these two cases, making them
indistinguishable in the runtime where the presence check is
implemented.  To fix that, this patch changes Clang to generate calls
to a new set of runtime functions, `__tgt_target_exit_data_*`, for the 
case of `target exit data`.

For symmetry, this patch makes a similar change for `target enter
data`, but that change isn't required for the above fix.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84422

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/declare_mapper_codegen.cpp
  clang/test/OpenMP/target_enter_data_codegen.cpp
  clang/test/OpenMP/target_enter_data_depend_codegen.cpp
  clang/test/OpenMP/target_exit_data_codegen.cpp
  clang/test/OpenMP/target_exit_data_depend_codegen.cpp
  clang/test/OpenMP/target_map_member_expr_array_section_codegen.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  openmp/libomptarget/include/omptarget.h
  openmp/libomptarget/src/exports
  openmp/libomptarget/src/interface.cpp
  openmp/libomptarget/src/omptarget.cpp
  openmp/libomptarget/src/private.h
  openmp/libomptarget/test/mapping/present/target_data_at_exit.c

Index: openmp/libomptarget/test/mapping/present/target_data_at_exit.c
===
--- /dev/null
+++ openmp/libomptarget/test/mapping/present/target_data_at_exit.c
@@ -0,0 +1,37 @@
+// RUN: %libomptarget-compile-aarch64-unknown-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-aarch64-unknown-linux-gnu 2>&1 \
+// RUN: | %fcheck-aarch64-unknown-linux-gnu
+
+// RUN: %libomptarget-compile-powerpc64-ibm-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-powerpc64-ibm-linux-gnu 2>&1 \
+// RUN: | %fcheck-powerpc64-ibm-linux-gnu
+
+// RUN: %libomptarget-compile-powerpc64le-ibm-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-powerpc64le-ibm-linux-gnu 2>&1 \
+// RUN: | %fcheck-powerpc64le-ibm-linux-gnu
+
+// RUN: %libomptarget-compile-x86_64-pc-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-x86_64-pc-linux-gnu 2>&1 \
+// RUN: | %fcheck-x86_64-pc-linux-gnu
+
+#include 
+
+int main() {
+  int i;
+
+#pragma omp target enter data map(alloc:i)
+
+  // i isn't present at the end of the target data region, but the "present"
+  // modifier is only checked at the beginning of a region.
+#pragma omp target data map(present, alloc: i)
+  {
+#pragma omp target exit data map(delete:i)
+  }
+
+  // CHECK-NOT: Libomptarget
+  // CHECK: success
+  // CHECK-NOT: Libomptarget
+  fprintf(stderr, "success\n");
+
+  return 0;
+}
Index: openmp/libomptarget/src/private.h
===
--- openmp/libomptarget/src/private.h
+++ openmp/libomptarget/src/private.h
@@ -24,8 +24,8 @@
 
 extern int target_data_end(DeviceTy , int32_t arg_num, void **args_base,
void **args, int64_t *arg_sizes, int64_t *arg_types,
-   void **arg_mappers,
-   __tgt_async_info *async_info_ptr);
+   void **arg_mappers, __tgt_async_info *async_info_ptr,
+   bool for_exit_data);
 
 extern int target_data_update(DeviceTy , int32_t arg_num,
   void **args_base, void **args,
Index: openmp/libomptarget/src/omptarget.cpp
===
--- openmp/libomptarget/src/omptarget.cpp
+++ openmp/libomptarget/src/omptarget.cpp
@@ -421,10 +421,30 @@
   return OFFLOAD_SUCCESS;
 }
 
+static int target_data_end_not_for_exit_data(DeviceTy , int32_t arg_num,
+ void **args_base, void **args,
+ int64_t *arg_sizes,
+