[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: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");
+  }
----------------
gtbercea wrote:
> 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.
I don't see the relation to what I asked: Sure, `omp_target_alloc` has to return fresh memory. But why does this matter for `omp_target_is_present`?


================
Comment at: libomptarget/test/api/api.c:98
+
+  // CHECK: Inside target data: A is not present
+  // CHECK: Inside target data: B is present
----------------
gtbercea wrote:
> 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.
> If A is not associated to a device instance then it won't be considered present.

Can you link me to a paragraph in the spec that mandates this implication? I only know of the following from the `Effect` of `omp_target_is_present`:

> This routine returns non-zero if the specified pointer would be found present on device device_num by a map clause; otherwise, it returns zero.

I think there's nothing that prevents a non-zero return value because in fact all pointers would be found present by a `map` clause under unified shared memory.


================
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];
+
----------------
gtbercea wrote:
> 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?
Yes, casting to `long long` might truncate, or at least I'm not aware that it guarantees to be of the same size as pointers.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D65001