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

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1997
   case ABIArgInfo::Ignore:
+  case ABIArgInfo::IndirectAliased:
     return false;
----------------
rjmccall wrote:
> In principle, this can be `inreg` just as much as Indirect can.
The IR verifier currently will reject byref + inreg


================
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 &&
----------------
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


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9383
   case ABIArgInfo::InAlloca:
+  case ABIArgInfo::IndirectAliased:
     llvm_unreachable("Unsupported ABI kind for va_arg");
----------------
rjmccall wrote:
> No reason not to use the Indirect code here.
I generally don't like speculatively adding handling for features I can't write a testcase for, but I've moved these


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

https://reviews.llvm.org/D79744