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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
