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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:52
+    /// IndirectAliased - Similar to Indirect, but the pointer may not be
+    /// writable.
+    IndirectAliased,
----------------
Hmm.  I guess there's actually two different potential conventions here:

- The caller can provide the address of a known-immutable object that has the right value, and the callee has to copy it if it needs the object to have a unique address or wants to mutate it.

- The caller can provide the address of *any* object that has the right value, and the callee has to copy it if it needs the object to have a unique address, wants to mutate it, or needs the value to stick around across call boundaries.

The advantage of the second is that IRGen could avoid some copies on the caller side, which could be advantageous when the callee is sufficiently trivial.  The disadvantage is that the callee would have to defensively copy in more situations.

Probably we should use the former.  Please be explicit about it, though:

  Similar to Indirect, but the pointer may be to an object that is otherwise
  referenced.  The object is known to not be modified through any other
  references for the duration of the call, and the callee must not itself
  modify the object.  Because C allows parameter variables to be modified
  and guarantees that they have unique addresses, the callee must
  defensively copy the object into a local variable if it might be modified or
  its address might be compared.  Since those are uncommon, in principle
  this convention allows programs to avoid copies in more situations.
  However, it may introduce *extra* copies if the callee fails to prove that
  a copy is unnecessary and the caller naturally produces an unaliased
  object for the argument.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2213
         Attrs.addAlignmentAttr(Align.getQuantity());
 
       // byval disables readnone and readonly.
----------------
Please add a TODO here that we could add the `byref` attribute if we're willing to update the test cases.  Maybe whoever does that can add alignments at the same time.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2462
+        // may be aliased, copy it since the incoming argument may not be
+        // mutable.
         Address V = ParamAddr;
----------------
"copy it to ensure that the parameter variable is mutable and has a unique address, as C requires".

I've wanted Sema to track whether local variables are mutated or have their address taken for a long time; maybe someday we can do that and then take advantage of it here.  Just a random thought, sorry.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:4696
+    case ABIArgInfo::IndirectAliased:
+      // This should be similar to Indirect, but no valid use case right now.
+      llvm_unreachable("Call arguments not implemented for IndirectAliased");
----------------
Please just make this use the Indirect code.  If we gave it special attention, we could optimize it better, but conservatively doing what Indirect does should still work.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1997
   case ABIArgInfo::Ignore:
+  case ABIArgInfo::IndirectAliased:
     return false;
----------------
In principle, this can be `inreg` just as much as Indirect can.


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


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9383
   case ABIArgInfo::InAlloca:
+  case ABIArgInfo::IndirectAliased:
     llvm_unreachable("Unsupported ABI kind for va_arg");
----------------
No reason not to use the Indirect code here.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9754
+  case ABIArgInfo::IndirectAliased:
     llvm_unreachable("Unsupported ABI kind for va_arg");
   case ABIArgInfo::Ignore:
----------------
Same.


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

https://reviews.llvm.org/D79744