[PATCH] D76365: [cuda][hip] Add CUDA builtin surface/texture reference support.
Michael Liao via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 20 15:12:44 PDT 2020
hliao marked 5 inline comments as done.
hliao added inline comments.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:701-713
+ if (getLangOpts().CUDAIsDevice) {
+ // As CUDA builtin surface/texture types are replaced, skip generating TBAA
+ // access info.
+ if (AccessType->isCUDADeviceBuiltinSurfaceType()) {
+ if (getTargetCodeGenInfo().getCUDADeviceBuiltinSurfaceDeviceType() !=
+ nullptr)
+ return TBAAAccessInfo();
----------------
tra wrote:
> Would `isCUDADeviceBuiltinTextureType()` be sufficient criteria for skipping TBAA regeneration?
> Or does it need to be 'it is the texture type and it will be replaced with something else'? What is 'something else' is the same type?
>
>
The replacement only happens in the device compilation. On the host-side, the original type is still used.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4101-4127
+ if (const ClassTemplateSpecializationDecl *TD =
+ dyn_cast<ClassTemplateSpecializationDecl>(RD)) {
+ Linkage = llvm::GlobalValue::InternalLinkage;
+ const TemplateArgumentList &Args = TD->getTemplateInstantiationArgs();
+ if (RD->hasAttr<CUDADeviceBuiltinSurfaceTypeAttr>()) {
+ assert(Args.size() == 2 &&
+ "Unexpcted number of template arguments of CUDA device "
----------------
tra wrote:
> This is the part I'm not comfortable with.
> It's possible for the user to use the attribute on other types that do not match the expectations encoded here.
> We should not be failing with an assert here because that's *user* error, not a compiler bug.
>
> Expectations we have for the types should be enforced by Sema and compiler should produce proper diagnostics.
>
`device_builtin_surface_type` and `device_builtin_texture_type` should only be used internally. Regular users of either CUDA or HIP must not use them as they need special internal handling and coordination beyond the compiler itself.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:6471-6472
+ // Lookup `addrspacecast` through the constant pointer if any.
+ if (auto *ASC = llvm::dyn_cast_or_null<llvm::AddrSpaceCastOperator>(C))
+ C = llvm::cast<llvm::Constant>(ASC->getPointerOperand());
+ if (auto *GV = llvm::dyn_cast_or_null<llvm::GlobalVariable>(C)) {
----------------
tra wrote:
> What's the expectation here? Do we care which address spaces we're casting to/from?
We need to check whether we copy from that global variable directly. As all pointers are generic ones, the code here is to look through the `addrspacecast` constant expression for the original global variable.
================
Comment at: clang/lib/Headers/__clang_cuda_runtime_wrapper.h:82-94
#undef __CUDACC__
#if CUDA_VERSION < 9000
#define __CUDABE__
#else
+#define __CUDACC__
#define __CUDA_LIBDEVICE__
#endif
----------------
tra wrote:
> Please add comments on why __CUDACC__ is needed for driver_types.h here? AFAICT, driver_types.h does not have any conditionals that depend on __CUDACC__. What happens if it's not defined.
>
>
`driver_types.h` includes `host_defines.h`, where macros `__device_builtin_surface_type__` and `__device_builtin_texture_type__` are conditional defined if `__CUDACC__`.
The following is extracted from `cuda/crt/host_defines.h`
```
#if !defined(__CUDACC__)
#define __device_builtin__
#define __device_builtin_texture_type__
#define __device_builtin_surface_type__
#define __cudart_builtin__
#else /* defined(__CUDACC__) */
#define __device_builtin__ \
__location__(device_builtin)
#define __device_builtin_texture_type__ \
__location__(device_builtin_texture_type)
#define __device_builtin_surface_type__ \
__location__(device_builtin_surface_type)
#define __cudart_builtin__ \
__location__(cudart_builtin)
#endif /* !defined(__CUDACC__) */
```
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6944-6945
+ handleSimpleAttributeWithExclusions<CUDADeviceBuiltinSurfaceTypeAttr,
+ CUDADeviceBuiltinTextureTypeAttr>(S, D,
+ AL);
+ break;
----------------
tra wrote:
> Nit: Formatting is a bit odd here. Why is AL on a separate line?
it's formatted by `clang-format`, which is run in pre-merge checks
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76365/new/
https://reviews.llvm.org/D76365
More information about the cfe-commits
mailing list