[PATCH] D79744: clang: Use byref for aggregate kernel arguments

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:8816
+  // FIXME: Should use byref when promoting pointers in structs, but this
+  // requires adding implementing the coercion.
+  if (!getContext().getLangOpts().OpenCL && LTy == OrigLTy &&
----------------
arsenm wrote:
> rjmccall wrote:
> > arsenm wrote:
> > > rjmccall wrote:
> > > > I don't see why you'd use `byref` when promoting pointers in structs.  Maybe it works as a hack with your backend, but it seems *extremely* special-case and should not be hacked into the general infrastructure.
> > > The whole point is to reinterpret the address space of the pointers in memory since we know if it's a kernel argument it has to be an addrspace(1) pointer or garbage. We can't infer the address space of a generic pointer loaded from memory.
> > > 
> > > byref doesn't change that, it just makes the fact that these are passed in memory explicit
> > `byref` is interpreted by your backend passes as an instruction that the argument value is actually the address of an object that's passed to the kernel by value, so you need to expand the memory in the kernel argument marshalling.  Why would that be something you'd want to trigger when passing a struct with a pointer in it?  You're not going to recursively copy and pass down the pointee values of those pointers.
> Because all arguments are really passed byref, we're just not at the point yet where we can switch all IR arguments to use byref for all arguments. All of the relevant properties are really always on the in-memory value. 
> 
> The promotion this is talking about is really orthogonal to the IR mechanism used for passing kernel arguments. This promotion is because the language only exposes generic pointers. In the context of a pointer inside a struct passed as a kernel argument, we semantically know the address space of any valid pointers must be global. You could not produce a valid generic pointer from another address space here. The pointers/structs are still the same size and layout, but coercing the in-memory address space is semantically more useful to the optimizer
I understand that the promotion is orthogonal to the IR mechanism used for passing kernel arguments, which is exactly why I'm asking why there's a comment saying that we should "use byref when promoting pointers in struct", because I have no idea what that's supposed to mean when the pointer is just a part of the value being passed.

It sounds like what you want is to maybe customize the code that's emitted to copy a byref parameter into a parameter variable when the parameter type is a struct containing a pointer you want to promote.  But that doesn't really have anything to do with `byref`; if you weren't using `byref`, you'd still want a similar customization when creating the parameter variable.  So it seems to me that the comment is still off-target.


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

https://reviews.llvm.org/D79744