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

[Gheorghe-Teodor Bercea via Phabricator via Openmp-commits]

gtbercea marked 6 inline comments as done.
gtbercea added inline comments.


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


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


================
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");
+  }
----------------
Hahnfeld wrote:
> 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"?
I said it and I'll say it again: when the user manually requests data to be allocated on the device, data must be allocated on the device. In this case the use of the omp_target_alloc will allocate data on the device even if unified shared memory is active.


================
Comment at: libomptarget/test/api/api.c:98
+
+  // CHECK: Inside target data: A is not present
+  // CHECK: Inside target data: B is present
----------------
Hahnfeld wrote:
> because this sounds really odd: `A` can be accessed on the device, but is not "present"?
This is just a presence test using the OpenMP API. If A is not associated to a device instance then it won't be considered present. Again, if the user wants to handle all of this manually the option is there and unified memory can't make this not an option for the user.


================
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];
+
----------------
Hahnfeld wrote:
> Can we make this `int`?
Yes


================
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];
+
----------------
Hahnfeld wrote:
> Why are the pointers cast to `long long`? Can we just compare them as `void *`?
Is there a problem I'm missing if they are cast to long long?


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D65001