[PATCH] D68578: [HIP] Fix device stub name
Artem Belevich via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 18 10:25:47 PST 2020
tra added inline comments.
================
Comment at: clang/include/clang/AST/GlobalDecl.h:40
+ Stub = 1,
+};
+
----------------
rjmccall wrote:
> tra wrote:
> > rjmccall wrote:
> > > The attribute here is `CUDAGlobalAttr`; should this be named in terms of CUDA, or is the CUDA model sufficiently different from HIP that the same implementation concept doesn't apply?
> > I believe the attribute serves the same purpose in both CUDA and HIP and could be renamed appropriately in a separate patch.
> >
> > While the changes in this patch are not required for CUDA, CUDA would benefit from them. We could use a generic GPU prefix and migrate CUDA to the same model later. A TODO comment about that would be nice.
> I'd just like consistency. If they're serving the same purpose, then as someone with no dog in this fight, I would give precedence to CUDA over HIP in names since it's both the older language and was implemented first in Clang (even if only partially, IIUC). I don't think a generic name works unless we can meaningfully generalize it to all languages with a similar feature, e.g. OpenCL and so on.
Naming, the hardest problem in computer science. :-)
I personally would prefer generalization-with-exclusions over specific name which is inconsistently commingles things that's really specific to that name and things that are more widely used. Alas, right now CUDA is the example of the latter case -- some parts are CUDA-specific and a lot are shared with HIP.
For the new features we've been sort of sticking with using CUDA/HIP for specific parts and GPU for shared functionality, but as things are a lot of shared bits are still 'CUDA' and it's hard to tell them apart. As you point it out, renaming the incumbent names would be a pain, so here we are.
I think using `GPUKernelKind` with a comment that it reflects HIP & CUDA kernels would be somewhat better choice than `CUDAKernelKind` which would be somewhat confusing at this point given that CUDA actually does not use it yet. I'm also fine with keeping it `HIPKernelKind` and postpone the naming decision until CUDA-related parts are actually implemented.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68578/new/
https://reviews.llvm.org/D68578
More information about the cfe-commits
mailing list