[PATCH] D139213: [llvm][CUDA] Make NVVM Reflect pass look inside ptr casting

[Hugh Delaney via Phabricator via llvm-commits]

hdelan added inline comments.


================
Comment at: llvm/test/CodeGen/NVPTX/nvvm-reflect-cast.ll:16
+; COMMON-NOT: call i32 @__nvvm_reflect
+  %reflect = tail call i32 bitcast (i32 (i8*)* @__nvvm_reflect to i32 (i8 addrspace(4)*)*)(i8 addrspace(4)* noundef getelementptr inbounds ([12 x i8], [12 x i8] addrspace(4)* @"$str", i64 0, i64 0))
+; SM20: ret i32 200  
----------------
hdelan wrote:
> tra wrote:
> > hdelan wrote:
> > > tra wrote:
> > > > Interesting. 
> > > > 
> > > > Wouldn't it be better/easier to generate ASC(4->0) of the string argument instead of casting the  `__nvvm_reflect` type?
> > > > Or declare an overload of __nvvm_reflect() accepting the pointer in AS(4)?
> > > > 
> > > > I'm OK with this change, but I want to understand the full picture, and have the motivation/context for the change recorded. Otherwise it's not clear why one would need this at all.
> > > This isn't possible when dealing with llvm intrinsics that do not map to clang builtins. When the first `__nvvm_reflect` call occurs in a module, a declaration is made based on how it is called. This make a declaration for AS4, AS1 etc. Then if say `__nvvm_reflect` is called again in the same module from a different AS, ptr casting will be employed so that the existing declaration will work with the new addrspace of the parameter.
> > > 
> > > When using the clang builtin this doesn't happen since use of `__nvvm_reflect` builtin clearly maps to the llvm intrinsic, which isn't tied to a particular address space.
> > > When the first __nvvm_reflect call occurs in a module, a declaration is made based on how it is called
> > 
> > Are you saying that the code relies on implicit declaration of `__nvvm_reflect` and does not provide it explicitly? 
> > What if someone calls `__nvvm_reflect(1)` ?
> > 
> > I do not see how the declaration which would be provided by compiler for  `__nvvm_reflect` if it were a builtin would be different from providing such a declaration in the user code.
> > 
> > > When using the clang builtin this doesn't happen since use of __nvvm_reflect builtin clearly maps to the llvm intrinsic, which isn't tied to a particular address space.
> > 
> > I'm not sure what does it have to do with the intrinsics at all. All you need is a declaration for a function `extern "C" int__nvvm_reflect(const char*);` 
> > https://godbolt.org/z/MoqMYfjcx
> > 
> > My understanding was that the problem you needed to solve is that constant strings lived in AS(4) so the only piece missing is ability of NVVMReflect pass to look through addrspacecast AS(4 or whatever) ->AS(0) which would be wrapping the string constant in your case.
> > 
> > What do I miss?
> And we cannot do any casting within openCL since strings must be in the `__constant` address space
A clarification: The code would rely on some `extern "C" __nvvm_reflect()` declaration in user code. The problem is that in openCL strings are in the `__constant` address space, so in order to use `__nvvm_reflect` you need to declare `extern "C" __nvvm_reflect(__constant char *)`, whereas in non openCL code you can declare `extern "C" __nvvm_reflect(char *)`. 

Whichever IR declaration appears first in the module will be taken as the one declaration of `__nvvm_reflect` something that would look like: 
`declare dso_local i32 @__nvvm_reflect(i8* noundef)`. 

When another TU makes use of `__nvvm_reflect` from a different address space, CodeGen will attempt to make this other `extern "C"` declaration fit with the pre existing IR declaration, which necessitates ptr casting either from addrspace. This generates a call matching this line here in the test. In order for `__nvvm_reflect` to work as desired we want to get rid of this pointer casting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139213/new/

https://reviews.llvm.org/D139213