[all-commits] [llvm/llvm-project] 8e4836: [OpenMP] Use IsHostPtr where needed for targetDataEnd

[Joel E. Denny via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 8e4836b2a296e4e78cc86f52014c48d9ad5aaf1a
      https://github.com/llvm/llvm-project/commit/8e4836b2a296e4e78cc86f52014c48d9ad5aaf1a
  Author: Joel E. Denny <jdenny.ornl at gmail.com>
  Date:   2021-09-01 (Wed, 01 Sep 2021)

  Changed paths:
    M openmp/libomptarget/src/device.cpp
    M openmp/libomptarget/src/device.h
    M openmp/libomptarget/src/omptarget.cpp
    A openmp/libomptarget/test/unified_shared_memory/associate_ptr.c
    M openmp/libomptarget/test/unified_shared_memory/close_enter_exit.c
    A openmp/libomptarget/test/unified_shared_memory/close_member.c

  Log Message:
  -----------
  [OpenMP] Use IsHostPtr where needed for targetDataEnd

As discussed in D105990, without this patch, `targetDataEnd`
determines whether to transfer data or delete a device mapping (as
opposed to assuming it's in shared memory) using two different
conditions, each of which is broken for some cases:

1. `!(UNIFIED_SHARED_MEMORY && TgtPtrBegin == HstPtrBegin)`: The
   broken case is rare: the device and host might happen to use the
   same address for their mapped allocations.  I don't know how to
   write a test that's likely to reveal this case, but this patch does
   fix it, as discussed below.
2. `!UNIFIED_SHARED_MEMORY || HasCloseModifier`: There are at least
   two broken cases:
    1. The `close` modifier might have been specified on an `omp
      target enter data` but not the corresponding `omp target exit
      data`, which thus might falsely assume a mapping is in shared
      memory.  The test `unified_shared_memory/close_enter_exit.c`
      already has a missing deletion as a result, and this patch adds
      a check for that.  This patch also adds the new test
      `close_member.c` to reveal a missing transfer and deletion.
    2. Use of discrete memory might have been forced by
      `omp_target_associate_ptr`, as in the test
      `unified_shared_memory/api.c`.  In the current `targetDataEnd`
      implementation, this condition turns out not be used for this
      case: because the reference count is infinite, a transfer is
      possible only with an `always` modifier, and this condition is
      never used in that case.  To ensure it's never used for that
      case in the future, this patch adds the test
      `unified_shared_memory/associate_ptr.c`.

Fortunately, `DeviceTy::getTgtPtrBegin` already has a solution: it
reports whether the allocation was found in shared memory via the
variable `IsHostPtr`.

After this patch, `HasCloseModifier` is no longer used in
`targetDataEnd`, and I wonder if the `close` modifier is ever useful
on an `omp target data end`.

Reviewed By: grokos

Differential Revision: https://reviews.llvm.org/D107925


  Commit: fa6c275505632846260f610b71656ddde1d88598
      https://github.com/llvm/llvm-project/commit/fa6c275505632846260f610b71656ddde1d88598
  Author: Joel E. Denny <jdenny.ornl at gmail.com>
  Date:   2021-09-01 (Wed, 01 Sep 2021)

  Changed paths:
    M openmp/libomptarget/src/omptarget.cpp

  Log Message:
  -----------
  [OpenMP][NFC] Eliminate CopyMember from targetDataEnd

This patch is based on comments in D105990.  It is NFC according to
the following observations:

1. `CopyMember` is computed as `!IsHostPtr && IsLast`.
2. `DelEntry` is true only if `IsLast` is true.

We apply those observations in order:

```
if ((DelEntry || Always || CopyMember) && !IsHostPtr)

if ((DelEntry || Always || IsLast) && !IsHostPtr)

if ((Always || IsLast) && !IsHostPtr)
```

Reviewed By: grokos

Differential Revision: https://reviews.llvm.org/D107926


  Commit: d11bab0b73ec485f0b1e8dc38af0be72fcda1e34
      https://github.com/llvm/llvm-project/commit/d11bab0b73ec485f0b1e8dc38af0be72fcda1e34
  Author: Joel E. Denny <jdenny.ornl at gmail.com>
  Date:   2021-09-01 (Wed, 01 Sep 2021)

  Changed paths:
    M openmp/libomptarget/src/device.cpp
    M openmp/libomptarget/src/device.h
    M openmp/libomptarget/src/omptarget.cpp
    M openmp/libomptarget/test/unified_shared_memory/associate_ptr.c

  Log Message:
  -----------
  [OpenMP] Use IsHostPtr where needed for targetDataBegin

As discussed in D105990, without this patch, `targetDataBegin`
determines whether to transfer data (as opposed to assuming it's in
shared memory) using the condition `!UseUSM || HasCloseModifier`.
However, this condition is broken if use of discrete memory was forced
by `omp_target_associate_ptr`.  This patch extends
`unified_shared_memory/associate_ptr.c` to reveal this case, and it
fixes it using `!IsHostPtr` in `DeviceTy::getTargetPointer` to replace
this condition.

Reviewed By: grokos

Differential Revision: https://reviews.llvm.org/D107927


  Commit: 786a140650990ce9f760da5ffeb5fad0b93687d9
      https://github.com/llvm/llvm-project/commit/786a140650990ce9f760da5ffeb5fad0b93687d9
  Author: Joel E. Denny <jdenny.ornl at gmail.com>
  Date:   2021-09-01 (Wed, 01 Sep 2021)

  Changed paths:
    M openmp/libomptarget/src/omptarget.cpp

  Log Message:
  -----------
  [OpenMP] Use IsHostPtr where needed in rest of omptarget.cpp

As started in D107925, this patch replaces the remaining occurrences
of `UNIFIED_SHARED_MEMORY && TgtPtrBegin == HstPtrBegin` in
`omptarget.cpp` with `IsHostPtr`.  The former condition is broken in
the rare case that the device and host happen to use the same address
for their mapped allocations.  I don't know how to write a test that's
likely to reveal this case.

Reviewed By: grokos

Differential Revision: https://reviews.llvm.org/D107928


Compare: https://github.com/llvm/llvm-project/compare/39f780b51d7c...786a14065099