tra added a comment.
Syntactically the patch looks OK to me, but I think the purpose and meaning of
the builtin type should be documented in more details. Based on this patch
alone it's not clear at all what it's supposed to be used for and how.
================
Comment at: include/clang/Basic/AttrDocs.td:4166
+style attribute __declspec(device_builtin_texture_type) can be added to the
+definition of a class template to indicate it is the HIP builtin texture type.
+It is ignored for CUDA and other languages.
----------------
What does it mean for user-defined template to be a builtin type? This sounds
like contradiction to me.
At the very least it should be documented somewhere what are the requirements
for such a template and what is it supposed to do in the end.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2424
+ !Global->hasAttr<CUDASharedAttr>() &&
+ !(isCUDATextureType(Global->getType()) && LangOpts.HIP))
return;
----------------
Nit: I'd check LangOpts.HIP first. No need chasing pointers in
isCUDATExtureType if it's not a HIP compilation.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:3781
+ (IsCUDASharedVar || IsCUDAShadowVar ||
+ (getLangOpts().CUDAIsDevice && isCUDATextureType(D->getType()))))
Init = llvm::UndefValue::get(getTypes().ConvertType(ASTTy));
----------------
This is not predicated by HIP compilation and will have effect on CUDA.
It's also not clear to me why texture is initialized as undef on device side.
Adding a comment about that would be great.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62738/new/
https://reviews.llvm.org/D62738
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
