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

[Aaron Ballman]

On Fri, Mar 29, 2013 at 5:16 PM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
> 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? :)

Very convincing!  Good catch; would you like to resolve this, or would
you like me to tackle it when I get the chance?

~Aaron