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

[George Rokos via Phabricator via Openmp-commits]

grokos added inline comments.


================
Comment at: libomptarget/src/device.cpp:174
+  if (RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY &&
+      (IsImplicit || !lr.Flags.IsContained || Size)) {
+    DP("Return HstPtrBegin " DPxMOD " Size=%ld RefCount=%s\n",
----------------
gtbercea wrote:
> grokos wrote:
> > gtbercea wrote:
> > > gtbercea wrote:
> > > > gtbercea wrote:
> > > > > grokos wrote:
> > > > > > I still don't understand this condition (even in the simplified form). Can you explain what we are testing?
> > > > > The condition comes from the 1st and 3rd if conditions. I have updated the condition to its more explicit version.
> > > > > 
> > > > > But, from what I can tell, IsContained can never be true in this case, ExtendesBefore/After I think cannot be true either.
> > > > > 
> > > > > IsImplicit can vary, Size also.
> > > > > 
> > > > > So I would reduce the condition to:
> > > > > 
> > > > > (IsImplicit || Size)
> > > > > 
> > > > > Does this now make sense?
> > > > > 
> > > > > 
> > > > Actually it should just be Size I think. Testing it now.
> > > Condition has now been fixed. This is it in its simplest form.
> > I still think the condition is not correct - it can lead to invalid mappings escaping the runtime check. E.g. what if we use unified memory and we try to explicitly "map more"? The condition will evaluate to true (since we try to extend the mapping, `isContained` will be false and Size will be > 0), but the mapping is illegal and should be caught by the runtime.
> > 
> > I believe the whole `if` branch this patch introduces should be removed. Instead, we must let the original code perform the checks and in the last else-branch (line 185 in the original code "`else if (Size)`" we should check whether or not we use unified shared memory; if yes, then simply return the host address, if no, then proceed with the allocation of memory on the device. In other words, let this function find out whether the requested mapping already exists on the device; if it does we are done, if it doesn't then find out whether we should allocate device memory or use the host version of data.
> Moved it.
OK, it looks good now. Make sure to rebase the next patch (the `close` modifier) and extend this condition to check for `!close` before returning the host address (`RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY && !HasCloseModifier`).


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D65001