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

[Artem Belevich via Phabricator via llvm-commits]

tra 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:
> hdelan wrote:
> > tra wrote:
> > > hdelan wrote:
> > > > hdelan wrote:
> > > > > tra wrote:
> > > > > > 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
> > > > > > Nothing stops you declaring nvvm_reflect with a `__constant` argument: https://godbolt.org/z/jTbvcsa3a
> > > > > > NVVM reflect does not care as long as we can get the string value.
> > > > > 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.
> > > > Sorry they should be `extern "C" int __nvvm_reflect(...)`
> > > So the problem is:
> > > - OpenCL has string constants in its own AS and can't cast it to generic AS (I wasn't aware of that).
> > > - Declaring OpenCL-specific `__nvvm_reflect(__constant char*)` variant makes compilation work for OpenCL w/o having to add any casts. 
> > > - However, it results in different signatures on LLVM IR level, 
> > > - Different signatures result in necessity of typecasting between the two __nvvm_reflect() variants.
> > > - and that's why you had to strip casts from the function, and not the argument.
> > > 
> > > OK. I think it mostly makes sense now.
> > > 
> > > So, if `__nvvm_reflect` were a builtin, accepting `const char *`, how would it work with OpenCL? You would still not be allowed to pass it a string constant, right?
> > > 
> > > Can we side-step the problem by using `extern "C" int __nvvm_reflect(...)` everywhere? This way compiler will happily pass whatever  pointer you give it, though the downside will be that there will be no diagnostics if you try to pass is a weird argument. It also only works for C++ variant of OpenCL. Otherwise variadic function would still need an argument.
> > > 
> > > Having mismatched `__nvvm_reflect` IR declarations and resulting casts is probably benign, given that it can never be a real function and the casts will go away after NVVMReflect is done.
> > > 
> > > How about if we side-step the problem altogether and use something as simple as this: https://godbolt.org/z/oY7coWz35
> > > 
> > > ```
> > > int __nvvm_reflect(const char *);
> > > 
> > > int wrapper(__constant const char *arg) {
> > >     uintptr_t p = (uintptr_t)arg;
> > >     return __nvvm_reflect((const char *) p);
> > > }
> > > kernel void k(global int* in1, global int* in2, global int* out) {
> > >      out[0] = wrapper("__CUDA_ARCH");
> > > }
> > > ```
> > > 
> > This kind of pointer casting that you attach above generates the IR:
> > 
> > ```
> > tail call i32 @__nvvm_reflect(i8* noundef inttoptr (i64 ptrtoint ([12 x i8] addrspace(4)* @.str5 to i64) to i8*))
> > ```
> > 
> > Which forces the assertion in `NVVMReflectPass` to fail:
> > 
> > ```
> > assert(isa<ConstantDataSequential>(Operand))
> > ```
> > 
> > So the logic of the pass will need to change either to accommodate your proposed approach, or to allow for the casting to be removed from around the function call, as I propose.
> > 
> Which approach do we think is preferable? Allowing `__nvvm_reflect` to be declared as an `extern "C"` func with any addrspace `const char *` parameter, or changing the pass to allow `__nvvm_reflect` to take non a `ConstantDataSequential` string (as is made happen with pointer casting)?
> `tail call i32 @__nvvm_reflect(i8* noundef inttoptr (i64 ptrtoint ([12 x i8] addrspace(4)* @.str5 to i64) to i8*))`

That was the idea.

> `assert(isa<ConstantDataSequential>(Operand))`
> So the logic of the pass will need to change either to accommodate your proposed approach, o

Correct. We'll need to dig through to the string constant. I think this is a marginally better approach compared to ending up with different declarations for `__nvvm_reflect`.

I wonder if there's a way to get bitcast instead of inttotr/ptrtoint in the source code. Bitcast would be better (less ops to see through), but I'm not sure how to get it in C++ or opencl.

Also, after tinkering with this approach for a bit, I've realized that `ptrtoint->inttoptr` has another issue -- there may be additional loads/stores in-between that we'll also need to deal with.  https://godbolt.org/z/cWTvf33fY

It's probably doable, but it may not be worth complicating things just for that.
I guess the simplest thing we can do is to introduce `__nvvm_reflect_ocl()` which we can then declare with an argument in AS(4) without conflict with the signature for the regular `__nvvm_reflect()`.

So, my preferences would be, in order:
- allow __nvvm_reflect() with arguments bitcast'ed or ASC'ed from constants in other AS'es, if we can figure out how to produce bitcast from OpenCL code.
- introduce __nvvm_reflect_ocl(...) with an argument in AS(4) so it works with OpenCL strings.
- allow __nvvm_reflect() with arguments behind ptrtoint/inttoptr and, possibly, intermediate stores/loads



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139213