[Openmp-commits] [PATCH] D106510: [OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in runtime (2/2)

[Joel E. Denny via Phabricator via Openmp-commits]

jdenny added a comment.

Thanks for reviewing.



================
Comment at: openmp/libomptarget/src/device.cpp:200
     auto &HT = *LR.Entry;
-    assert(HT.getRefCount() > 0 && "expected existing RefCount > 0");
-    if (UpdateRefCount)
+    IsNew = false;
+    const char *RefCountAction;
----------------
grokos wrote:
> `IsNew` is already `false` at this point, no need to reassign.
I'll remove it.


================
Comment at: openmp/libomptarget/src/device.cpp:343
+      else {
+        HT.decRefCount(UseHoldRefCount);
+        RefCountAction = " (reset)";
----------------
grokos wrote:
> Currently, we don't decrement the RefCount at this point. Is this a bug in the existing code?
Without this patch, the only way I know of that `ForceDelete=true` but `IsLast=false` is if the reference count is infinite.  Decrementing the reference count would have no effect then (and neither did resetting), so I don't think there's an existing bug.

With this patch, a new possibility is that the hold reference count is non-zero.  Because, as a result, `IsLast=false`, we're not to ready to unmap, so the final decrement of the dynamic reference counter after its reset needs to happen here.


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

https://reviews.llvm.org/D106510