[ms-cxxabi] Set proper SRet flags for most functions; also handle empty struct arguments correctly

[Timur Iskhodzhanov]

2013/4/17 John McCall <rjmccall at apple.com>:
> On Apr 15, 2013, at 11:41 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>> 2013/4/13 John McCall <rjmccall at apple.com>:
>>> On Apr 12, 2013, at 12:38 PM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>>> 2013/4/12 John McCall <rjmccall at apple.com>:
>>>>> On Apr 12, 2013, at 8:44 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>>>>> 2013/4/9 John McCall <rjmccall at apple.com>
>>>>>>>
>>>>>>>> If that looks OK - I assume I should do the same for other ABIInfos ?
>>>>>>>> (I hope they are covered with tests well...)
>>>>>> See the attached patch where I've applied the same approach to other ABIs.
>>>>>>
>>>>>>>>> /// How should we pass a particular record type as an argument?
>>>>>>>>> enum RecordArgABI {
>>>>>>>>> /// Pass it using the normal C aggregate rules for the ABI, potentially
>>>>>>>>> /// introducing extra copies and passing some or all of it in registers.
>>>>>>>>> RAA_DirectInRegisters,
>>>>>>>>>
>>>>>>>>> /// Pass it on the stack using its defined layout.  The argument must be
>>>>>>>>> /// evaluated directly into the correct stack position in the arguments area,
>>>>>>>>> /// and the call machinery must not move it or introduce extra copies.
>>>>>>>>> RAA_DirectInMemory,
>>>>>>>>>
>>>>>>>>> /// Pass it as a pointer to temporary memory.
>>>>>>>>> RAA_Indirect
>>>>>>>>> };
>>>>>>>> I'm not sure I can effectively use the first two cases right now and
>>>>>>>> there should also be RAA_IndirectByval (see what I do for Microsoft
>>>>>>>> ABI), so I've created a slightly simpler enum.
>>>>>>>
>>>>>>> Despite how it looks in IR, "byval" is direct in memory.
>>>>>>>
>>>>>>> Just to be clear, the Itanium ABI would use RAA_Indirect if the copy constructor
>>>>>>> or destructor is non-trivial and RAA_DirectInRegisters otherwise,
>>>>>>
>>>>>> OK, after re-reading the RAA_DirectInRegisters comment a few times I
>>>>>> now understand it's exactly the same as I proposed for RAA_Default.
>>>>>> Not sure why I had not seen that before...
>>>>>
>>>>> RAA_Default is fine for the name, actually.  Go with that.
>>>> OK, done
>>>>
>>>>>> I now use the same enum you've suggested, but I also explicitly set
>>>>>> the RAA_DirectInRegisters's value to 0
>>>>>> so now I can write like this:
>>>>>>
>>>>>> if (RecordArgABI RAA = getRecordArgABI(...))
>>>>>>  ABIArgInfo::getIndirect(0, RAA == CGCXXABI::RAA_DirectInMemory);
>>>>>> // continue if RAA == RAA_DirectInRegisters.
>>>>>>
>>>>>> Is it OK to do so?
>>>>>>
>>>>>>> and the MS ABI would use RAA_DirectInMemory if there's any non-trivial
>>>>>>> constructor or destructor and RAA_DirectInRegisters otherwise.
>>>>>> I think you've meant just "MS ABI would use RAA_DirectInMemory.",
>>>>>> otherwise my tests fail.
>>>>>
>>>>> Are you saying that the MS ABI should just *always* use RAA_DirectInMemory?
>>>> Yes, at least I haven't seen counter-examples so far, though I have
>>>> like 20 different tests for passing structs as arguments that actually
>>>> run the code and verify it works.
>>>> (plus 30 more for returning structs)
>>>
>>> If that's the C ABI rule, it should be handled by the C ABI, i.e. the normal
>>> ABIInfo handling for the type.
>> Sounds reasonable - aaand I've found a bad test in Clang!
>> See test/CodeGen/x86_32-arguments-win32.c - these structs should be
>> passed via stack.
>>
>>> But, for example, it seems like X64 does not
>>> follow this rule, and will happily pass structs of size 1, 2, 4, or 8 in a register.
>> Good catch!
>> I've tested my patch on Win64 and yeah, the small structs are passed
>> in registers unless they have non-trivial copy constructors.
>> Interestingly, the presence of a destructor or operator= don't seem to
>> move an argrument from a register to stack (weird).
>>
>> I didn't test the "class method" part of the test much on Win64 as the
>> mangling is broken right now and it's very inconvenient to verify
>> stuff there.
>> I'll surely get back to this when we have Win64 more working in general.
>>
>> -> Fixed both things, please review.
>
> +  bool isReturnTypeIndirect(const CXXRecordDecl *RD) const {
> +    return !RD->isPOD();
> +  }
> +
>
> Please add a comment here explaining the exact definition of POD
> being used.  That's probably the C++03 definition, although it's probably
> a good idea to test around the edges of that.
I've added a test when a 1-int struct inherits from an Empty struct
(i.e. trivial, standard-layout but not C++03 POD) and verified we
generate a correct code in this case.
Thanks for the idea!

> +static bool isRecordReturnIndirect(QualType T, CodeGen::CodeGenTypes &CGT) {
> +static CGCXXABI::RecordArgABI getRecordArgABI(QualType T,
> +                                              CodeGen::CodeGenTypes &CGT) {
>
> Please make overloads of these that take a const RecordType* and then
> take advantage of that in the places that currently do so, e.g.:
>
> @@ -664,9 +662,7 @@ ABIArgInfo X86_32ABIInfo::classifyReturnType(QualType RetTy,
>
>    if (isAggregateTypeForABI(RetTy)) {
>      if (const RecordType *RT = RetTy->getAs<RecordType>()) {
> -      // Structures with either a non-trivial destructor or a non-trivial
> -      // copy constructor are always indirect.
> -      if (hasNonTrivialDestructorOrCopyConstructor(RT))
> +      if (isRecordReturnIndirect(RetTy, CGT))
>
> ^^^ this should use the RecordType overload.
Done.

>    case X87:
>    case ComplexX87:
> -    if (isRecordWithNonTrivialDestructorOrCopyConstructor(Ty))
> +    if (getRecordArgABI(Ty, CGT))
>        ++neededInt;
>
> You should only increment neededInt here for CGCXXABI::RAA_Indirect.
Done!
(I don't like the smell of "this was not covered by tests" though...)

> @@ -3728,12 +3728,10 @@ ABIArgInfo AArch64ABIInfo::classifyGenericType(QualType Ty,
>      return tryUseRegs(Ty, FreeIntRegs, RegsNeeded, /*IsInt=*/ true);
>    }
>
> -  // Structures with either a non-trivial destructor or a non-trivial
> -  // copy constructor are always indirect.
> -  if (isRecordWithNonTrivialDestructorOrCopyConstructor(Ty)) {
> +  if (CGCXXABI::RecordArgABI RAA = getRecordArgABI(Ty, CGT)) {
>      if (FreeIntRegs > 0)
>        --FreeIntRegs;
> -    return ABIArgInfo::getIndirect(0, /*ByVal=*/false);
> +    return ABIArgInfo::getIndirect(0, RAA == CGCXXABI::RAA_DirectInMemory);
>    }
>
> Same thing here;  only decrement FreeIntRegs for RAA_Indirect.
Done too.

> Otherwise, looks good.
Committed the updated version as r179681.

Thanks a lot for the thorough review of such a patch!


> John.