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

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

jdenny 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:
> jdenny wrote:
> > grokos wrote:
> > > 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.
> > Ah, of course.  Thanks.
> > 
> > What should happen if `close` is specified on an `omp target enter data` but not on the corresponding `omp target exit data`?  (See test close_enter_exit.c.)  To handle that case, don't we have to use the first version of the above condition in targetDataEnd?
> > 
> > I wonder how `close` can ever be meaningful when specified on an `omp target exit data`.
> Another case where the first condition appears to be required is when `omp_target_associate_ptr` circumvents unified shared memory (as in the test `unified_shared_memory/api.c`).  I think both `targetDataEnd` and `targetDataBegin` are wrong for this case.
How quickly I forgot your point.  The first condition cannot be used to handle either of these situations because the addresses might happen to be the same even with discrete memory.  We'll need a new condition.


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

https://reviews.llvm.org/D105990