tra 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();
----------------
hliao wrote:
> 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.
But you've already checked CUDAIsDevice so you already know that you want to
replace the type.
`if (getTargetCodeGenInfo().getCUDADeviceBuiltinTextureDeviceType() !=
nullptr)` appears to be redundant and can probably be dropped.
================
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 "
----------------
hliao wrote:
> 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.
I agree that it's probably not something that should be used by users.
Still, such use should be reported as an error and should *not* crash the
compiler. Asserts are for clang/llvm developers to catch the bugs in the
compiler itself, not for the end users misusing something they should not.
================
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)) {
----------------
hliao wrote:
> 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.
I'm still not sure what exactly you want to do here.
If the assumption is that all `addrspacecast` ops you may see are from global
to generic AS, this assumption is not always valid. I can [[
https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments
| annotate any pointer with an arbitrary address space ]] which may then be
cast to generic. Or something else.
If you accept Src as is, without special-casing addrspacecast, what's going to
happen?
AFAICT `nvvm_texsurf_handle_internal` does not really care about specific AS.
================
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
----------------
hliao wrote:
> 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__) */
> ```
My concern is -- what else is going to get defined? There are ~60 references to
__CUDACC__ in CUDA-10.1 headers. The wrappers are fragile enough that there's a
good chance something may break. It does not help that my CUDA build bot
decided to die just after we switched to work-from-home, so there will be no
early warning if something goes wrong.
If all we need are the macros above, we may just define them.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6944-6945
+ handleSimpleAttributeWithExclusions<CUDADeviceBuiltinSurfaceTypeAttr,
+ CUDADeviceBuiltinTextureTypeAttr>(S, D,
+ AL);
+ break;
----------------
hliao wrote:
> 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
Sorry. It was an artifact of messed up fonts in my browser. Apparently I've
ended up using proportional font.
<rant> Why, oh why almost all fonts listed as 'fixed-width' on the chromebook
are actually *not* ?! Even the ones that are fixed-width are prone to use
ligatures and mess formatting. 'ffff' is still longer than 'fifi' for me.</rant>
This code looks much better with fixed-width font.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76365/new/
https://reviews.llvm.org/D76365
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
