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

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

jdenny added a comment.

Thanks for the review.  I'd feel more comfortable with one small simplification at a time.  That is, a subsequent patch (which I'm happy to write if you'd like) could make the further simplification you're proposing and adjust targetDataBegin to match.  Does that seem reasonable?



================
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",
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105990