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

[hdelan]

Instead of adding an __nvvm_reflect clang builtin (https://reviews.llvm.org/D137154), it was decided that it is better to adjust the NVVMReflect pass so that __nvvm_reflect can be used when wrapped in pointer casting. This allows __nvvm_reflect to be used in openCL.

[tra]

It would be great to add a test demonstrating what the code is intended to do.

[tra]

llvm/include/llvm/IR/InstrTypes.h

1407

Does it need a type check as it's done in getCalledFunction() above after a similar cast?

[hdelan]

llvm/include/llvm/IR/InstrTypes.h

1407

The type checking above is checking that the operands match for the function and the call. However because we are removing ptr casts the operands will be different for the call and the func with removed ptr casts

[hdelan]

• Adding test

[hdelan]

Test added

[tra]

llvm/test/CodeGen/NVPTX/nvvm-reflect-cast.ll

16 ↗

(On Diff #479696)

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.

[hdelan]

llvm/test/CodeGen/NVPTX/nvvm-reflect-cast.ll

16 ↗

(On Diff #479696)

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.

[tra]

llvm/test/CodeGen/NVPTX/nvvm-reflect-cast.ll

16 ↗

(On Diff #479696)

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?

[hdelan]

llvm/test/CodeGen/NVPTX/nvvm-reflect-cast.ll

16 ↗

(On Diff #479696)

And we cannot do any casting within openCL since strings must be in the __constant address space

[hdelan]

llvm/test/CodeGen/NVPTX/nvvm-reflect-cast.ll

16 ↗

(On Diff #479696)

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.

[hdelan]

llvm/test/CodeGen/NVPTX/nvvm-reflect-cast.ll

16 ↗

(On Diff #479696)

Sorry they should be extern "C" int __nvvm_reflect(...)

[tra]

llvm/test/CodeGen/NVPTX/nvvm-reflect-cast.ll

16 ↗

(On Diff #479696)

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.

[tra]

llvm/test/CodeGen/NVPTX/nvvm-reflect-cast.ll

16 ↗

(On Diff #479696)

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");
}

[hdelan]

llvm/test/CodeGen/NVPTX/nvvm-reflect-cast.ll

16 ↗

(On Diff #479696)

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.

[hdelan]

llvm/test/CodeGen/NVPTX/nvvm-reflect-cast.ll

16 ↗

(On Diff #479696)

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)?

[tra]

llvm/test/CodeGen/NVPTX/nvvm-reflect-cast.ll

16 ↗

(On Diff #479696)

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

[hdelan]

Changing the approach to define __nvvm_reflect_ocl which can take a const char * in
AS(4) so it can be used in openCL.

[hdelan]

Change description in test

[hdelan]

Changing NVVMReflect pass to allow for nvvm_reflect_ocl to be substituted out,
which allows for using nvvm_reflect_ocl with an AS(4) const char * param.

[hdelan]

Hi @tra , after some unsuccessful attempts at changing ptr casting to bitcasting, I have implemented your second preference. That is introduce __nvvm_reflect_ocl(...) with an argument in AS(4) so it works with OpenCL strings. Let me know your thoughts

[tra]

LGTM. Thank you for trying all these different variants and helping me understand the requirements and constraints you have on the OpenCL side.

[hdelan]

Fantastic thanks for your help. I do not have permission to commit so could you please commit this patch on my behalf? Thanks

[tra]

In D139213#3995612, @hdelan wrote:

I do not have permission to commit so could you please commit this patch on my behalf? Thanks

Will do.

[tra]

Should I attribute the commit to your email @codeplay.com ? Or do you prefer something else?

[hdelan]

In D139213#3995633, @tra wrote:

Should I attribute the commit to your email @codeplay.com ? Or do you prefer something else?

Yes that would be great. Thank you

[tra]

This patch does not apply cleanly to my sources. Is it based on the public LLVM tree? Can you rebase it on top of the main branch?

[hdelan]

This comment was removed by hdelan.

[hdelan]

Clean diff

[nikic]

llvm/test/CodeGen/NVPTX/nvvm-reflect-ocl.ll

10 ↗

(On Diff #485968)

Please use opaque pointers for new tests.

[hdelan]

Using opaque pointers

[tra]

LGTM. I'll attempt to land it later today.

[hdelan]

Great thanks.

llvm/include/llvm/IR/InstrTypes.h

public:
   /// invocation or the function signature does not match the call signature.
   Function *getCalledFunction() const {
     if (auto *F = dyn_cast_or_null<Function>(getCalledOperand()))
       if (F->getValueType() == getFunctionType())
         return F;
     return nullptr;
   }
 
+  /// Returns the function called and removes the pointer casting around it
+  Function *getCalledFunctionRemovingPtrCasts() const {
+    if (auto *Operand = getCalledOperand())
+      return dyn_cast_or_null<Function>(Operand->stripPointerCasts());
+    return nullptr;
+  }
+
   /// Return true if the callsite is an indirect call.
   bool isIndirectCall() const;
 
   /// Determine whether the passed iterator points to the callee operand's Use.
   bool isCallee(Value::const_user_iterator UI) const {
     return isCallee(&UI.getUse());
   }
 

llvm/lib/Target/NVPTX/NVVMReflect.cpp

static bool runNVVMReflect(Function &F, unsigned SmVersion) {
   //
   // In this case, we get a Constant with a GlobalVariable operand and we need
   // to dig deeper to find its initializer with the string we'll use for lookup.
   for (Instruction &I : instructions(F)) {
     CallInst *Call = dyn_cast<CallInst>(&I);
     if (!Call)
       continue;
     Function *Callee = Call->getCalledFunction();
+    if (!Callee)
+      Callee = Call->getCalledFunctionRemovingPtrCasts();
     if (!Callee || (Callee->getName() != NVVM_REFLECT_FUNCTION &&
                     Callee->getIntrinsicID() != Intrinsic::nvvm_reflect))
       continue;
 
     // FIXME: Improve error handling here and elsewhere in this pass.
     assert(Call->getNumOperands() == 2 &&
            "Wrong number of operands to __nvvm_reflect function");