[Openmp-commits] [PATCH] D68310: Introduce an interface for target_impl that supports default implementations

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield marked an inline comment as done.
JonChesterfield added a comment.

In D68310#1690758 <https://reviews.llvm.org/D68310#1690758>, @jdoerfert wrote:

> Could you motivate this is more? The template solution is for sure more complex than the old proposal/solution and still requires the same cmake tricks so I'd like to hear why it is worth it.


Thanks! If the end result of the review is we stick with free functions that's fine with me. I can imagine using the types to move some complexity out of cmake but haven't thought that through. I think the primary drawbacks to the current system are relying on cmake to work with multiple architectures and the requirement to implement every function for every architecture.

I'd like the target_impl layer to have default implementations where they're meaningful. It would mean a target can introduce a new function, along with a default, and every other target would continue to work as before, without changing any of the other target code.

- popc can be implemented as a call to __builtin_popcount if one doesn't want to call the cuda function
- Lanemask, smid probably don't have good defaults
- Fences might do, in that localised ones could call through to more global ones
- The dozen or so atomic operations used in deviceRTL could all have default implementations that call the standard C++ functions, while using the cuda calls when preferred

It's worth noting that the current list of free functions hits most of the bullet points in the diff, except that I can't see a way to provide compile time default implementations without some extra plumbing. Above template is a suggestion for said plumbing.

> I'll also go through the bullet points you have (in order) and leave some feedback:
> 
> - Thanks! I'll try the same format.



> - I don't see why there should be "runtime indirection" if you don't use the template stuff.
>   - The most popular interface scheme in C++ involves virtual functions (usually with heap allocation). I don't trust devirtualisation, and eliding the heap allocation is messy.



> - I also do not see why the "old design" would not allow header only (it was actually proposed that way after all).
> - - It does. My preferred option is declarations in a header and implementations at link time, but that degraded codegen under nvcc.



> - I get the default implementations but (1) I doubt there are many (where do you expect default impls that are in the target_impl part?) and (2) we could deal with them with different headers, the same way as I proposed to work with different targets.
>   - (1) All the bit level functions, all atomics. Partly papering over cuda as opposed to nvptx, but they're closely related. (2) is interesting - deferred to below



> - Unclear why declarations would not do the same, actually declarations would probably catch more than templates do.
>   - I think the crtp approach moves some errors from link time to compile time. Mostly though I was referring to the private constructor / friend stuff - trying to make the interface harder to implement wrong. The `= delete` syntax was nice in that regard.



> - Unsure what "syntactically reasonable" means.
>   - Mostly that the interface shouldn't be woven out of macros and code generators



> - Familiar to C++ developers, is not a good argument when the C++ stuff actually doesn't solve the problem (still cmake magic involved)
>   - Not so much that the code should look like C++, more that the code shouldn't look totally alien to C++. E.g. there could be variable fields that map to arbitrary function calls, theadIdx.x style, but that would be a bad thing.

Above you said

> we could deal with them with different headers, the same way as I proposed to work with different targets.

Please could you expand on that? I think the current multitarget plan is a `common` folder containing as much code as we can manage, with a `target_impl.h` in each target, where `#include` paths are set by cmake to look in the target subdir when compiling things in the common subdir. I can see a path to default functions that involves a separate header per function, where the existence of files on disk and some cmake determines which set are pulled into an aggregate header. That's not necessarily what you have in mind though.



================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/target_api.h:34
+  INLINE static uint64_t packImpl(uint32_t lo, uint32_t hi);
+  INLINE static void unpackImpl(uint64_t, uint32_t &, uint32_t &) = delete;
+
----------------
jdoerfert wrote:
> It is odd to provide a default for pack but not for unpack, when would this ever be useful?
> 
> It also seems overly complicated to redirect here in the first place. Subclasses could as well provide pack/unpack directly, couldn't thy?
Sure. We'd either want defaults for both, or defaults for neither until a target is introduced that would use the defaults. Providing one of each is good for discussion but probably not how it should be committed.

The redirect costs some code in the base class but none in the subclass (well, other than the Impl suffix). I like the separation between the interface to the client and the interface to the target, but sure - there's lots of ways to wire things up. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68310