[cfe-commits] [PATCH][CodeGen] Add padding to expanded struct function arguments if the ABI requires it.

[Michael Spencer]

On Sun, Nov 14, 2010 at 1:49 AM, John McCall <rjmccall at apple.com> wrote:
>
> On Nov 13, 2010, at 9:43 AM, Michael Spencer wrote:
>
>> On Sat, Nov 13, 2010 at 1:12 AM, John McCall <rjmccall at apple.com> wrote:
>>>
>>> On Nov 12, 2010, at 7:52 PM, Michael Spencer wrote:
>>>
>>>> On Fri, Nov 12, 2010 at 10:14 PM, John McCall <rjmccall at apple.com> wrote:
>>>>> On Oct 18, 2010, at 11:54 PM, Michael Spencer wrote:
>>>>>> The Microsoft ABI requires that structs passed by value as "expanded"
>>>>>> arguments maintain padding. This patch implements this and fixes
>>>>>> <http://llvm.org/bugs/show_bug.cgi?id=8398>.
>>>>>
>>>>> Sorry this has taken so long.  Can you explain how this approach is different from just not passing the struct as expanded at all, i.e. passing it byval?
>>>>>
>>>>> John.
>>>>
>>>> I thought byval was a pointer thing, would it work in this case?
>>>
>>> As I understand it, byval arguments are passed in memory by copying the pointed-to object into the appropriate position in the arguments, which is pretty much exactly what you want.  I think the pointer-is-secretly-a-struct thing is just a throwback to before we had first-class aggregates.
>>>
>>> I'm also pretty sure that expansion won't work because it screws up register-passing CCs, of which there are several we need to support on MS platforms.  For example:
>>>
>>>  struct A { int x; double d; };
>>>  void __fastcall foo(struct A, void *, void *);  // the pointers should be passed in registers, not the int and the padding pseudo-argument.
>>>
>>> John.
>>
>> OK, so the actual problem is in X86_32ABIInfo::classifyArgumentType.
>> It should not be expanding small structs with padding, or any struct
>> at all when using fastcc.
>
> _fastcall is x86_fastcall;  fastcc is a totally different beast.
>
> The way I read it, the Windows calling conventions *never* conceptually do struct expansion;  i.e. in no case do you ever pass a struct by breaking it into components and figuring out how to pass those individually.

Yep.

>  (In contrast, the standard Unix x86-64 CC will pass, e.g., struct { double d; void *p; } in an FP register and an integer register unless you're passing it as a vararg).  I think that's how the standard Unix x86 convention works, too.  That doesn't mean we never want to do struct expansion anyway — it generally produces cleaner IR and allows better optimization and codegen — but it does mean we have to be careful to only do it in cases where the backend will end up producing the right results.  That probably means a lot of experimentation, but that can happen later, since it's just QoI;  for now, never expanding for structs with padding sounds good.


Yeah, expanded argument code is much cleaner .

>
> The documentation for __fastcall that I've seen says that arguments are passed in registers when they're no larger than a pointer;  if that's true and small structs really are passed in a single register, then classifyArgumentType should return 'direct' with a coercion to the appropriate integer type.  It's probably okay to do this regardless of CC and clean it up later as QoI work, but however you see fit.
>
> Some useful test cases to investigate:
>
> void _fastcall foo(struct A { char a; short b; } var);
> void _fastcall foo(struct A { short a; short b; } var);
>
> John.

Thanks for the test cases. I also have quite a few other combinations
I am looking at. I agree that's it's good to expand when possible.
Expand with padding really is a different operation than just padding.
I will also end up with much better code.

- Michael Spencer