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

[Timur Iskhodzhanov]

Should be resolved in r178634.

2013/4/3 Aaron Ballman <aaron at aaronballman.com>:
> 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