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

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

gtbercea added inline comments.


================
Comment at: libomptarget/src/device.cpp:178
     auto &HT = *lr.Entry;
-    IsNew = false;
-
-    if (UpdateRefCount)
-      ++HT.RefCount;
-
-    uintptr_t tp = HT.TgtPtrBegin + ((uintptr_t)HstPtrBegin - HT.HstPtrBegin);
-    DP("Mapping exists%s with HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD ", "
-        "Size=%ld,%s RefCount=%s\n", (IsImplicit ? " (implicit)" : ""),
-        DPxPTR(HstPtrBegin), DPxPTR(tp), Size,
-        (UpdateRefCount ? " updated" : ""),
-        (CONSIDERED_INF(HT.RefCount)) ? "INF" :
-            std::to_string(HT.RefCount).c_str());
-    rc = (void *)tp;
-  } else if ((lr.Flags.ExtendsBefore || lr.Flags.ExtendsAfter) && !IsImplicit) {
-    // Explicit extension of mapped data - not allowed.
-    DP("Explicit extension of mapping is not allowed.\n");
-  } else if (Size) {
-    // If it is not contained and Size > 0 we should create a new entry for it.
-    IsNew = true;
-    uintptr_t tp = (uintptr_t)RTL->data_alloc(RTLDeviceID, Size, HstPtrBegin);
-    DP("Creating new map entry: HstBase=" DPxMOD ", HstBegin=" DPxMOD ", "
-        "HstEnd=" DPxMOD ", TgtBegin=" DPxMOD "\n", DPxPTR(HstPtrBase),
-        DPxPTR(HstPtrBegin), DPxPTR((uintptr_t)HstPtrBegin + Size), DPxPTR(tp));
-    HostDataToTargetMap.push_front(HostDataToTargetTy((uintptr_t)HstPtrBase,
-        (uintptr_t)HstPtrBegin, (uintptr_t)HstPtrBegin + Size, tp));
+    uintptr_t tp = HT.HstPtrBegin + ((uintptr_t)HstPtrBegin - HT.HstPtrBegin);
+    DP("Return HstPtrBegin " DPxMOD " Size=%ld RefCount=%s\n", DPxPTR(tp),
----------------
gtbercea wrote:
> Hahnfeld wrote:
> > gtbercea wrote:
> > > grokos wrote:
> > > > This evaluates to just `HstPtrBegin`, the two `HT.HstPtrBegin` cancel out, so you can skip defining `tp` altogether.
> > > I tried and it leads to failing tests when handling structs.
> > Again I agree with the (static) analysis by @grokos. If that leads to failing tests, we need to understand why. What was the idea behind the calculation of `tp`?
> The answer is simple: this is how it was done before. Look at the existing pointer calculation I see no reason why it should have to be simplified. The simplification you propose has nothing to do with unified memory so if you think it needs to be done then you should open it up as a separate issue in a separate patch.
I misread the comment. I think it's no problem doing the simplification.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D65001