jlebar added a comment. Thank you for the review!
================ Comment at: clang/include/clang/Basic/Attr.td:604 +// CUDA attributes are spelled __attribute__((attr)) or __declspec(__attr__). + ---------------- aaron.ballman wrote: > jlebar wrote: > > aaron.ballman wrote: > > > jlebar wrote: > > > > aaron.ballman wrote: > > > > > For my own edification, do you have a link to some documentation of > > > > > what CUDA attributes are spelled with `__declspec`? > > > > I do not believe such documentation exists. It is an implementation > > > > detail in the CUDA headers -- users never write > > > > `__attribute__((device))` or `__declspec(__device__)`. Instead, they > > > > write `__device__`. > > > Then why are we introducing `__declspec(__device__)` rather than a > > > keyword attribute `__device__`? > > > > > > My biggest concern is: I would like to verify that these actually should > > > be supported as a `__declspec` attribute. From my simple testing in MSVC, > > > it does not appear to support `__declspec(__device__)`, but perhaps I am > > > doing it wrong (I'm mostly unfamiliar with CUDA). If this isn't something > > > MSVC supports, then it is the first attribute we're supporting with a > > > __declspec spelling that is not actually a Microsoft attribute, which is > > > something worth discussing. > > > Then why are we introducing __declspec(__device__) rather than a keyword > > > attribute __device__? > > > > `__device__` is a macro defined in the CUDA headers, which must include and > > we are not able to modify. > Okay, it makes sense as to why we can't have a keyword spelling, but it also > doesn't answer why we need the `__declspec` spelling for it. Are you saying > that there are CUDA headers out there where this attribute is spelled with > `__attribute__` and others with `__declspec`? If so, then this change makes a > bit more sense to me. (Phab bug; I can't delete this comment. Please ignore.) ================ Comment at: clang/include/clang/Basic/Attr.td:610 let LangOpts = [CUDA]; let Documentation = [Undocumented]; } ---------------- aaron.ballman wrote: > jlebar wrote: > > aaron.ballman wrote: > > > jlebar wrote: > > > > aaron.ballman wrote: > > > > > Now would be a good time to add the documentation for these > > > > > attributes, otherwise there's a lot less chance users will know what > > > > > ways they can spell the attributes (or what the attribute do). > > > > See above, these are an implementation detail. > > > We can still document that we support the attributes under their macro > > > names, or do users not typically think of these macros as being > > > attributes? I am mostly concerned about discoverability of the attributes > > > -- how is a user to know what Clang does or does not support? > > > how is a user to know what Clang does or does not support? > > > > These macros are fundamental to CUDA support. The statement "you can > > compile CUDA code with clang" will immediately imply to every CUDA > > developer in existence the statement "you can use `__device__` in your > > code". > Yet we add new CUDA attributes periodically, and CUDA comes out with new > versions and new features. > > Looking at the CUDA docs, I see `__managed__`, but I don't see such an > attribute in Clang. How is a user to know whether we do/don't support such a > construct? This is a fair point, I agree on this basis that we should add documentation here. https://reviews.llvm.org/D28321 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
