[libc-commits] [PATCH] D155812: [libc][amdgpu] Tolerate different install directories for hsa.h

Thu Jul 20 03:48:29 PDT 2023

jplehr accepted this revision.
jplehr added a comment.
This revision is now accepted and ready to land.

In D155812#4518208 <https://reviews.llvm.org/D155812#4518208>, @JonChesterfield wrote:

> In D155812#4518152 <https://reviews.llvm.org/D155812#4518152>, @jplehr wrote:
>
>> Potentially dumb question: Should this complexity be put in one place that can be used from everywhere?
>
> There's some institutional resistance / fear of adding a header file which is used by multiple projects within the monorepo. This is particularly annoying for the GPU runtimes.
>
> So far I've been able to get a "maybe, if we absolutely must" response to pure header files that don't have any source component, but the current preference is for copy&paste everywhere. There are some hazards around people packaging parts of the monorepo separately to the whole. There used to be licensing barriers.
>
> The HSA headers are a stable C interface which is fairly annoying to program against. We've ended up with callbacks-wrapped-in-templates-that-pass-lambdas copy&pasted between libc and the openmp-plugin. Possibly also amdgpu-arch, haven't checked recently. I'm currently leaning towards writing "hsa.hpp" as a wrapper that puts a C++ interface over it, and deals with this #include stuff as well, which gets copy&pasted between openmp and libc whenever someone notices divergence.
>
> Interesting question is whether that should be unified with dynamic_hsa as well.

Thanks for elaborating. Given that history and potential way forward, I think this patch looks good as it matches what's in the nextgen plugin. So we keep it (at least initially) consistent.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155812