[llvm-commits] [llvm] r151123 - in /llvm/trunk: lib/Target/X86/X86CallingConv.td test/CodeGen/X86/thiscall-struct-return.ll

[Timur Iskhodzhanov]

2013/3/29 Timur Iskhodzhanov <timurrrr at google.com>:
> 2013/3/29 Aaron Ballman <aaron at aaronballman.com>:
>> On Thu, Mar 28, 2013 at 9:45 PM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>> 2012/2/21 Aaron Ballman <aaron at aaronballman.com>:
>>>> Author: aaronballman
>>>> Date: Tue Feb 21 21:04:40 2012
>>>> New Revision: 151123
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=151123&view=rev
>>>> Log:
>>>> Adding support for Microsoft's thiscall calling convention.  LLVM side of the patch.
>>>>
>>>> Added:
>>>>     llvm/trunk/test/CodeGen/X86/thiscall-struct-return.ll
>>>> Modified:
>>>>     llvm/trunk/lib/Target/X86/X86CallingConv.td
>>>>
>>>> Modified: llvm/trunk/lib/Target/X86/X86CallingConv.td
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86CallingConv.td?rev=151123&r1=151122&r2=151123&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Target/X86/X86CallingConv.td (original)
>>>> +++ llvm/trunk/lib/Target/X86/X86CallingConv.td Tue Feb 21 21:04:40 2012
>>>> @@ -331,8 +331,8 @@
>>>>    // Promote i8/i16 arguments to i32.
>>>>    CCIfType<[i8, i16], CCPromoteToType<i32>>,
>>>>
>>>> -  // The 'nest' parameter, if any, is passed in EAX.
>>>> -  CCIfNest<CCAssignToReg<[EAX]>>,
>>>> +  // Pass sret arguments indirectly through EAX
>>>> +  CCIfSRet<CCAssignToReg<[EAX]>>,
>>>>
>>>>    // The first integer argument is passed in ECX
>>>>    CCIfType<[i32], CCAssignToReg<[ECX]>>,
>>>>
>>>> Added: llvm/trunk/test/CodeGen/X86/thiscall-struct-return.ll
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/thiscall-struct-return.ll?rev=151123&view=auto
>>>> ==============================================================================
>>>> --- llvm/trunk/test/CodeGen/X86/thiscall-struct-return.ll (added)
>>>> +++ llvm/trunk/test/CodeGen/X86/thiscall-struct-return.ll Tue Feb 21 21:04:40 2012
>>>> @@ -0,0 +1,47 @@
>>> Hi Aaron,
>>>
>>> I was working on sret vs thiscall and I thought I've fixed it locally
>>> but then noticed this test (which I find wrong, see below) still
>>> passes for me.
>>>
>>>> +; RUN: llc < %s -mtriple=i386-PC-Win32 | FileCheck %s
>>> I'm afraid triples are case-sensitive :(
>>> As least I get different |md5sum when running my local build with
>>> -mtriple=i386-PC-Win32 and -mtriple=i386-pc-win32.
>>
>> Good catch!  I never noticed that!
>>
>>>
>>>> +%class.C = type { i8 }
>>>> +%struct.S = type { i32 }
>>>> +%struct.M = type { i32, i32 }
>>>> +
>>>> +declare void @_ZN1CC1Ev(%class.C* %this) unnamed_addr nounwind align 2
>>>> +declare x86_thiscallcc void @_ZNK1C5SmallEv(%struct.S* noalias sret %agg.result, %class.C* %this) nounwind align 2
>>>> +declare x86_thiscallcc void @_ZNK1C6MediumEv(%struct.M* noalias sret %agg.result, %class.C* %this) nounwind align 2
>>>> +
>>>> +define void @testv() nounwind {
>>>> +; CHECK: testv:
>>>> +; CHECK: leal
>>>> +; CHECK-NEXT: movl     %esi, (%esp)
>>>> +; CHECK-NEXT: calll _ZN1CC1Ev
>>>> +; CHECK: leal 8(%esp), %eax
>>>> +; CHECK-NEXT: movl %esi, %ecx
>>> I believe this is wrong, the address of the structure should be passed
>>> via stack.
>>
>> I seem to recall observing it being passed "both ways" in that it was
>> allocated on the stack, but that the function would use EAX to
>> determine where on the stack to find it.
> I don't think so.
>
> The first thing that happens inside any thiscall/sret methods I saw at -O2 is
>   mov eax, ***[esp]
> i.e. the eax is ignored.
> A similar thing happens after the frame pointer prolog at -O0.
> I never observed eax being read by a thiscall/sret method.
>
> What often happens in the caller of a thiscall is
>   lea eax, ***[esp/ebp]
>   push eax
>   lea ecx, ***[esp/ebp]
>   call ...
> i.e. eax is used just as a temporary register to push a pointer onto stack.
> This could as well be ecx though.

And this actually happens!

How about this?
-----------------
struct SRet {
  SRet() {}
};

class Class {
 public:
  struct SRet thiscall_method_void() { return SRet(); }
  struct SRet thiscall_method(struct SRet a);
};

int returns_in_eax() { return 0xDEADBEEF; }

extern "C" int printf(const char *fmt, ...);

__declspec(naked) struct SRet Class::thiscall_method(struct SRet a) {
  int x;
  __asm mov [x], eax;
  printf("EAX = %p\n", x);
  __asm ret 0;
}

void main() {
  struct Class c;
  struct SRet t = c.thiscall_method_void();
  returns_in_eax();
  c.thiscall_method(t);
}
-----------------

$ cl test.cpp && ./test.exe
EAX = DEADBEEF

As you can see, the EAX may contain any value at the point of call.

Is that convincing enough? :)

>>>> +; CHECK-NEXT: calll _ZNK1C5SmallEv
>>>> +entry:
>>>> +  %c = alloca %class.C, align 1
>>>> +  %tmp = alloca %struct.S, align 4
>>>> +  call void @_ZN1CC1Ev(%class.C* %c)
>>>> +  ; This call should put the return structure as a pointer
>>>> +  ; into EAX instead of returning directly in EAX.
>>> AFAICT, the value of EAX value after the call should match the top of
>>> the stack before the call.
>>
>> It's been a while since I've looked into this, but I recall thiscall
>> being an odd-man-out with how it handled structure returns in that it
>> would put the pointer to the structure into EAX instead of the single
>> four-byte value.
> That depends on whether the return type is a POD or not.
>
>> So yes, it makes sense that it should match the top
>> of the stack before the call (accounting for the allocated space).
>>
>> ~Aaron