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

[Ting Wang via Phabricator via cfe-commits]

tingwang 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,
----------------
rjmccall wrote:
> 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.
Thank you!

I checked the `_Complex char` case on PPC64: complex element size smaller than argument slot is handled by `complexTempStructure()` (https://github.com/llvm/llvm-project/blob/51d33afcbe0a81bb8508d5685f38dc9fdb2b60c9/clang/lib/CodeGen/TargetInfo.cpp#L5451), and the right-adjustment is taken care by that logic. Both AIX and PPC64 use `complexTempStructure()` to produce variadic callee arguments in this case.

In case `_Complex char` is encapsulated inside structure, then the whole is considered as an aggregate, and is addressed by this fix. I will add a test case to illustrate.

Hope these addressed your concern.



================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5461
 
   // Otherwise, just use the general rule.
+  // TODO: a better approach may refer to SystemZABI use same logic for caller
----------------
rjmccall wrote:
> Please add this comment explaining the use of ForceRightAdjust:
> 
> > The PPC64 ABI passes some arguments in integer registers, even to variadic functions.  To allow `va_list` to use the simple "`void*`" representation, variadic calls allocate space in the argument area for the integer argument registers, and variadic functions spill their integer argument registers to this area in their prologues.  When aggregates smaller than a register are passed this way, they are passed in the least significant bits of the register, which means that after spilling on big-endian targets they will be right-aligned in their argument slot.  This is uncommon; for a variety of reasons, other big-endian targets don't end up right-aligning aggregate types this way, and so right-alignment only applies to fundamental types.  So on PPC64, we must force the use of right-alignment even for aggregates.
> 
> I'm not sure what your TODO is hoping for.  You'd like to re-use logic between the frontend's va_arg emission and the backend's variadic argument emission?  That would be very tricky.
Sure, I will add the comment. Thank you.

Maybe I misunderstood some previous comment. I will drop the TODO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133338