[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:1997
   case ABIArgInfo::Ignore:
+  case ABIArgInfo::IndirectAliased:
     return false;
----------------
arsenm wrote:
> arsenm wrote:
> > rjmccall wrote:
> > > arsenm wrote:
> > > > rjmccall wrote:
> > > > > In principle, this can be `inreg` just as much as Indirect can.
> > > > The IR verifier currently will reject byref + inreg
> > > Why?  `inreg` is essentially orthogonal.
> > Mostly inherited from the other similar attribute handling. It can be lifted if there's a use
> Plus the name here is isArgInAlloca; this is not necessarily passed in an alloca
I agree that we don't need to update this.


================
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:
> > 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.


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

https://reviews.llvm.org/D79744