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