[PATCH] D62738: [HIP] Support texture type

[Artem Belevich via Phabricator via cfe-commits]

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