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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

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.

I'll also go through the bullet points you have (in order) and leave some feedback:

- I don't see why there should be "runtime indirection" if you don't use the template stuff.
- I also do not see why the "old design" would not allow header only (it was actually proposed that way after all).
- 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.
- Unclear why declarations would not do the same, actually declarations would probably catch more than templates do.
- Unsure what "syntactically reasonable" means.
- Familiar to C++ developers, is not a good argument when the C++ stuff actually doesn't solve the problem (still cmake magic involved)



================
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;
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68310