[PATCH] D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:327
+///   if the argument is smaller than a slot, set this flag will force
+///   right-adjust the argument in its slot irrespective of the type.
 static Address emitVoidPtrDirectVAArg(CodeGenFunction &CGF,
----------------
Argh, Phabricator dropped one of my comments, and it's the one that explains why I CC'ed Tim Northover.

I'm a little worried about the existing uses of this function because this function is sensitive to the type produced by `ConvertTypeForMem`.  `ConvertTypeForMem` *mostly* only generates IR struct types for C structs and unions, but there are a few places where it generates an IR struct for some fundamental type that stores multiple values.  Most of those types are at least as large as an argument slot (e.g. they contain pointers), unless there's some weird target with huge slots.  However, some of them are not; I think the most important example is `_Complex T`, which of course gets translated into a struct containing two `T`s.  So if `T` is smaller than half an argument slot, we're not going to right-align `_Complex T` on big-endian targets other than PPC64, and I don't know if that's right.

That would affect `_Complex _Float16` on 64-bit targets; on 32-bit targets, I think you'd need something obscure like `_Complex char` to exercise it.

Now, if Clang generates arguments for one of these types using a single value that's also of IR struct type, and the backend considers that when deciding whether to right-align arguments, then maybe those two decisions cancel out and we've at least got call/va_arg compatibility, even if it's not necessarily what's formally specified by the appropriate psABI.  But `DirectTy` is definitely not necessarily the type that call-argument lowering will use, so I'm a little worried.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133338