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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

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

> In D71404#1782942 <https://reviews.llvm.org/D71404#1782942>, @jdoerfert wrote:
>
> > //To be honest, I did not follow this so if my response doesn't make sense let me know.//
>
>
> Apologies. I'll rephrase the problem now that it's a more reasonable time of day.
>
> > Could we have a generic atomic header with the template definition that is something like this:
> > 
> > ...
> > 
> > In the `target_impl.h` you can provide the specialized implementation.
> > 
> > Only one template version will be visible at any time. Would that solve the problem?
>
> I don't believe so.
>
> Consider common/src/loop.cu, which calls __kmpc_atomic_add. Today, that atomic add could be implemented under common/atomic.h and included as common/atomic.h or similar and all would work well.
>
> In the future, a third target wants to provide implementations for the atomics using C++ or OpenCL, so writes their own arch/src/atomic.h. This file would then be ignored by the source under common, because it's in the unexpected place.
>
> We can guard against this by writing the target specific implementation under nvptx/src/atomic.h, in which case common/src will include atomic.h and everything will work out as intended for the third target. This is why we have `#include "target_impl.h"` and `#include "common/debug.h"`, with careful include paths - to disambiguate.
>
> However, the amdgcn implementation would be very similar to the nvptx one. So this is a new use case for the shared source model:
>
> - Let nvptx use generic foo.h
> - Let amdgcn use generic foo.h
> - Let third party use specialised foo.h such that common code picks up the generic case or the specialised one if available.
>
>   This could be implemented by:
> - code duplication
> - making fooi.h and putting it under common, with an amdgcn stub which declares some functions then includes fooi.h
> - adding complexity to cmake
> - possibly by playing games with include resolution order
> - a defaults plus override model, e.g. D68310 <https://reviews.llvm.org/D68310>, where we hope the defaults are broadly applicable. Can fail on a forth target
> - always call unqualified headers (no common prefix) from everywhere, and include stubs that include shared text from the targets. So we pay with files like debug.h: #include "common/debug.h", but everything composes exactly correctly.
>
>   The general problem is how to statically compose source code such that the end result combines common and target specific code without collapsing under the complexity of the scheme. While using somewhat crude tools.
>
>   The last bullet point is the totally explicit representation of what one wants to happen, various other schemes move noise from the source into the build sytem.


We have two targets only right now, let's not complicate things too much. I'm fine with any solution that is reasonable and working now. I have a proposal below but other ways are fine as well.

If both targets agree on the atomics, put the code in common/atomics.h. Once we have third target that doesn't, we put the target code in {nvptx,amdgcn,third}/atomics.h and provide a pseudo/cpu target cpu/atomics.h with the generic openmp based template. In cmake we choose which include path you get, as we do now. (= I would not force no code duplication)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71404