[Openmp-commits] [PATCH] D71404: [libomptarget][nfc] Introduce atomic wrapper function

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

In D71404#1782914 <https://reviews.llvm.org/D71404#1782914>, @JonChesterfield wrote:

> There's an outstanding design point here.
>
> Logically, the implementation is per target so should be in arch/src/target_atomic.h, with call sites including target_atomic.
>
> However, the nvptx and amdgcn implementations will be (somewhat spuriously) the same. So either copy & paste time, or each needs to include a header from common containing the implementation which is otherwise not included anywhere.
>
> This awkward redirection is necessary under the current scheme to allow a new arch to provide a header that is picked up by common/src/foo.cpp.
>
> I wonder if that means a different redirection scheme is better, e.g. where headers are used from common unless one with the same name is provided under the arch. That seems error prone however.


//To be honest, I did not follow this so if my response doesn't make sense let me know.//

Could we have a generic atomic header with the template definition that is something like this:

  template <typename T> T __kmpc_impl_atomic_exchange(T *Ptr, T Val) {
    T Tmp;
    #pragma omp atomic
    { Tmp = *Ptr; *Ptr = Val; }
    return Tmp;
  }

In the `target_impl.h` you can provide the specialized implementation.

  template <typename T> T __kmpc_impl_atomic_exchange(T *Ptr, T Val) {
    return atomicExch(Ptr, Val);
  }

Only one template version will be visible at any time. Would that solve the problem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71404