tra added inline comments.
================ Comment at: clang/include/clang/AST/GlobalDecl.h:40 + Stub = 1, +}; + ---------------- rjmccall wrote: > tra wrote: > > 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. > Maybe `KernelReferenceKind`? It's probably a common concept across all > heterogenous-computing models. SGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68578/new/ https://reviews.llvm.org/D68578 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
