[Openmp-commits] [PATCH] D105990: [OpenMP][NFC] Simplify targetDataEnd conditions for CopyMember

[George Rokos via Phabricator via Openmp-commits]

grokos added inline comments.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:726-727
         if ((DelEntry || Always || CopyMember) &&
             !(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY &&
               TgtPtrBegin == HstPtrBegin)) {
           DP("Moving %" PRId64 " bytes (tgt:" DPxMOD ") -> (hst:" DPxMOD ")\n",
----------------
jdenny wrote:
> grokos wrote:
> > `cond2`: This is equivalent to `cond1`:
> > 
> > `!(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY && TgtPtrBegin == HstPtrBegin)` is equivalent to
> > `!(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) || TgtPtrBegin != HstPtrBegin`.
> > 
> > `TgtPtrBegin != HstPtrBegin` if USM is not enabled OR if we have used the `close` modifier to allocate data on the device instead of host RAM, so:
> > `TgtPtrBegin != HstPtrBegin` is equivalent to
> > `!(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) || HasCloseModifier==true`.
> > 
> > It follows that `cond2` is equivalent to:
> > `!(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) || HasCloseModifier`
> > which is `cond1`.
> > 
> > `DelEntry`, `Always` and `CopyMember` are relevant only if `cond2` is true. So we can delete the `if(cond1)` above and leave only this bit:
> > ```
> > if (IsLast)
> >   CopyMember = true;
> > ```
> > But then, this would be equal to setting `CopyMember = IsLast`:
> >   - If `IsLast` is false, then `CopyMember` will be false (that's how it was declare a few lines above`.
> >   - If `IsLast` is true, `CopyMember` is set to true, which is the value of `IsLast`.
> > In all cases, `CopyMember = IsLast`.
> > So we can get rid of `CopyMember` entirely and leave the code like this:
> > ```
> > if (ArgTypes[I] & OMP_TGT_MAPTYPE_FROM) {
> >   bool Always = ArgTypes[I] & OMP_TGT_MAPTYPE_ALWAYS;
> > 
> >   if ((DelEntry || Always || IsLast) &&
> >       !(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY &&
> >         TgtPtrBegin == HstPtrBegin)) {
> > ```
> Looks right to me, but it's possible I don't know of some additional condition in which `TgtPtrBegin == HstPtrBegin`.
> 
> Assuming that, as you say, all of the following are equivalent, which is best to use in the code?
> 
> * `!(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY && TgtPtrBegin == HstPtrBegin)`
> * `TgtPtrBegin != HstPtrBegin`
> * `!(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) || HasCloseModifier`
> 
> The second is most succinct, but the last is clearest in my opinion because it spells out the cases.  Maybe we should use the second but assert that it's equivalent to the last?
The second is *almost* equivalent to the last with one exception: in the most unlikely coincidence ever, a device address may happen to be the same number as a host address (i.e. although we have no USM support, a call to e.g. `cudaMalloc` may return a device pointer which happens to have the same value as a host pointer returned by `malloc`). Super unlikely, but let's stick with the last condition.


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

https://reviews.llvm.org/D105990