[Openmp-commits] [PATCH] D65001: [OpenMP][libomptarget] Add support for unified memory for regular maps

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added inline comments.


================
Comment at: libomptarget/test/api/api.c:9
+
+#pragma omp requires unified_shared_memory
+
----------------
Please add a manual call to the runtime function such that this test works with older versions of Clang.


================
Comment at: libomptarget/test/api/api.c:22
+int main(int argc, char *argv[]) {
+  const int device = 1;
+
----------------
Is there a particular reason to use the second device? Can we just take `omp_get_default_device`?


================
Comment at: libomptarget/test/api/api.c:69
+  for(int i=0; i<N; ++i) {
+    if (A[i] != (double)(i+2)) fail++;
+  }
----------------
Can we change the type to `int` to avoid any problems with floating point numbers?


================
Comment at: libomptarget/test/api/api.c:77-79
+  //
+  // target_is_present and target_associate/disassociate_ptr
+  //
----------------
I'd suggest to make this different tests, and make clear that the test exercises `unified_shared_memory` (maybe a new directory)


================
Comment at: libomptarget/test/api/api.c:82-88
+  // CHECK: B is not present, associating it...
+  // CHECK: omp_target_associate_ptr B succeeded
+  if (!omp_target_is_present(B, device)) {
+    printf("B is not present, associating it...\n");
+    int rc = omp_target_associate_ptr(B, d_B, N*sizeof(double), 0, device);
+    printf("omp_target_associate_ptr B %s\n", !rc ? "succeeded" : "failed");
+  }
----------------
And I still don't understand why we need `omp_target_is_present` to return false here. What would be the disadvantage of saying "all data is present on all devices"?


================
Comment at: libomptarget/test/api/api.c:98
+
+  // CHECK: Inside target data: A is not present
+  // CHECK: Inside target data: B is present
----------------
because this sounds really odd: `A` can be accessed on the device, but is not "present"?


================
Comment at: libomptarget/test/offloading/requires_unified_shared_memory_local.c:17-18
+  long long host_data, device_data;
+  double *alloc = (double *)malloc(N*sizeof(double));
+  double data[N];
+
----------------
Can we make this `int`?


================
Comment at: libomptarget/test/offloading/requires_unified_shared_memory_local.c:25-26
+
+  host_data = (long long) &data[0];
+  host_alloc = (long long) &alloc[0];
+
----------------
Why are the pointers cast to `long long`? Can we just compare them as `void *`?


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D65001