[Openmp-commits] [PATCH] D60223: [OpenMP][libomptarget] Add support for target link variables when unified memory is enabled

[Alexandre Eichenberger via Phabricator via Openmp-commits]

AlexEichenberger requested changes to this revision.
AlexEichenberger marked 2 inline comments as done.
AlexEichenberger added a comment.
This revision now requires changes to proceed.

see comments



================
Comment at: libomptarget/include/omptarget.h:51
+  /// Target link
+  OMP_TGT_MAPTYPE_USE_HOST_VAR    = 0x400,
   // member of struct, member given by [16 MSBs] - 1
----------------
ABataev wrote:
> Do you really need this flag? Can you use something different? For example, `OMP_TGT_MAPTYPE_LITERAL` or `OMP_TGT_MAPTYPE_PRIVATE`?
I don't believe it is needed, as all is handled while registering the device library. If anything, you could just use the literal to naturally move the host pointer address to the device. This is if you want to avoid going through the indirection already set in place for globals mapped with "link" clause


================
Comment at: libomptarget/plugins/cuda/src/rtl.cpp:443
+        // be overwritten.
+        cuMemcpyHtoD(cuptr, e->addr, 8);
+        DP("Copy linked variable host address (" DPxMOD ")"
----------------
ABataev wrote:
> `8`->`sizeof(void*)`
Shouldn't this code be guarded by some "if the runtime is in unified memory mode,"  and if we see TARGET_LINK and the runtime is not in unified memory mode, should't there be some error as things are likely to go seriously wrong? 


================
Comment at: libomptarget/src/rtl.cpp:180
       }
-
-      if (entry->flags & OMP_DECLARE_TARGET_LINK) {
----------------
Same comment here... if the runtime is in non-unified memory mode, should't it be an error to see a link?


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D60223