[Openmp-commits] [PATCH] D65340: [OpenMP][libomptarget] Add support for close map modifier

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

I think this still needs some work.



================
Comment at: libomptarget/src/device.cpp:171-172
   // maps are respected.
   // TODO: In addition to the mapping rules above, when the close map
   // modifier is implemented, foce the mapping of the variable to the device.
+  if (RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY && !IsCloseModifier &&
----------------
Please update this comment.


================
Comment at: libomptarget/src/device.cpp:204-205
+      uintptr_t tp = 0;
+      if (!(RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) ||
+          (RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY && IsCloseModifier))
+        tp = (uintptr_t)RTL->data_alloc(RTLDeviceID, Size, HstPtrBegin);
----------------
Why do we need this check? If logic doesn't fail me, this should always be true with the condition above.


================
Comment at: libomptarget/src/device.h:141
+      bool &IsNew, bool &IsHostPtr, bool IsImplicit, bool UpdateRefCount = true,
+      bool IsCloseModifier = false);
   void *getTgtPtrBegin(void *HstPtrBegin, int64_t Size);
----------------
I'd call this `HasCloseModifier` (here and in other places)


================
Comment at: libomptarget/src/omptarget.cpp:297
       bool copy = false;
-      if (!(Device.RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY)) {
-        if (IsNew || (arg_types[i] & OMP_TGT_MAPTYPE_ALWAYS)) {
+      bool Close = arg_types[i] & OMP_TGT_MAPTYPE_CLOSE;
+      if (!(Device.RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) ||
----------------
Please use `IsCloseModifier` / `HasCloseModifier` from above.


================
Comment at: libomptarget/src/omptarget.cpp:299
+      if (!(Device.RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) ||
+          (Device.RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY &&
+           Close)) {
----------------
Can you remove the second `Device.RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY`? Either it's not set, or it's set and the condition needs to check if the mapping has the `close` modifier.


================
Comment at: libomptarget/src/omptarget.cpp:400
         bool Always = arg_types[i] & OMP_TGT_MAPTYPE_ALWAYS;
+        bool Close = arg_types[i] & OMP_TGT_MAPTYPE_CLOSE;
         bool CopyMember = false;
----------------
Same here, use `IsCloseModifier` / `HasCloseModifier` from above.


================
Comment at: libomptarget/src/omptarget.cpp:403
+        if (!(Device.RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) ||
+            (Device.RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY &&
+             Close)) {
----------------
Same, logic can be simplified.


================
Comment at: libomptarget/src/omptarget.cpp:417-419
+        if ((DelEntry || Always || CopyMember || Close) &&
             !(Device.RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY &&
+              TgtPtrBegin == HstPtrBegin && !Close)) {
----------------
Can we have `TgtPtrBegin == HstPtrBegin` when the mapping has the `close` modifier?


================
Comment at: libomptarget/test/unified_shared_memory/close_modifier.c:17
+  long long host_data, device_data;
+  double *alloc = (double *)malloc(N*sizeof(double));
+  double data[N];
----------------
Please use `int` as in the other tests.


================
Comment at: libomptarget/test/unified_shared_memory/close_modifier.c:20
+
+  for(int i=0; i<N; ++i){
+    alloc[i] = 10.0;
----------------
Please format.


================
Comment at: libomptarget/test/unified_shared_memory/close_modifier.c:25-26
+
+  host_data = (long long) &data[0];
+  host_alloc = (long long) &alloc[0];
+
----------------
Please don't cast to `long long`!


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D65340