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

[John McCall]

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.

+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.

   case X87:
   case ComplexX87:
-    if (isRecordWithNonTrivialDestructorOrCopyConstructor(Ty))
+    if (getRecordArgABI(Ty, CGT))
       ++neededInt;

You should only increment neededInt here for CGCXXABI::RAA_Indirect.

@@ -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.

Otherwise, looks good.

John.