[PATCH] D84743: [Clang][AMDGCN] Universal device offloading macros header

Tue Jul 28 23:22:26 PDT 2020

jdoerfert added a comment.

In D84743#2181044 <https://reviews.llvm.org/D84743#2181044>, @MaskRay wrote:

> In D84743#2181031 <https://reviews.llvm.org/D84743#2181031>, @jdoerfert wrote:
>
>> In D84743#2179441 <https://reviews.llvm.org/D84743#2179441>, @tra wrote:
>>
>>> I'm not sure it's particularly useful, to be honest. CUDA code still needs to be compatible with NVCC so it can't be used in portable code like TF or other currently used CUDA libraries.
>>> It could be useful internally, though, so I'm fine with it for that purpose.
>>
>> FWIW, I was only thinking about `clang/lib/Header` usage. *Potentially* documented for user of clang.
>
> Honestly I am a bit uneasy with the new clang/lib/Header file. It will be part of the clang resource directory and users on every target will be able to `#include <offload_macros.h>` it.
> This is also a namespace pollution - used incorrectly, people can trip over it if they have files of the same name.

There are two levels to it though. 1) `clang/lib/Header`, and 2) `clang/lib/Header/XXXXXX`. We do not expose XXXXX to every user on every target, so when we do we really want the headers to be first. So for 2) the names should match existing system headers we want to wrap. For 1) the header names should be "in the compiler namespace". That said, the file above is not and I didn't notice before. The existing CUDA overloads that live in 1) start with `__cuda`, which should be sufficient for users not to trip over them. I mean, they could trip over a lot of things that starts with `__`. I imagine we have a `__gpu_...` set of header soon to avoid duplicating or polluting the `__cuda` ones (more). Now that I finished all this, also the `XXXXXX` above needs to be renamed into `__XXXXX`, but that seems easy enough to do.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84743