Handling SRet on Windows x86

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
59 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Handling SRet on Windows x86

Timur Iskhodzhanov
Hello Aaron, Anton, LLVM-dev,

While working on http://llvm.org/PR13676#c6
I found out that whenever I compile code with class methods returning
structures it get generated incompatible with MSVC.

Looking at lib/Target/X86/X86CallingConv.td, I found out that
CC_X86_32_ThisCall maps SRet to EAX but in fact it should push
the address of the return temp on stack.

The following patch fixes the issue on Windows:
---------------------------------
Index: lib/Target/X86/X86CallingConv.td
===================================================================
--- lib/Target/X86/X86CallingConv.td      (revision 164763)
+++ lib/Target/X86/X86CallingConv.td      (working copy)
@@ -335,7 +335,8 @@
   CCIfType<[i8, i16], CCPromoteToType<i32>>,

   // Pass sret arguments indirectly through EAX
-  CCIfSRet<CCAssignToReg<[EAX]>>,
+  CCIfSRet<CCAssignToStack<4, 4>>,

   // The first integer argument is passed in ECX
   CCIfType<[i32], CCAssignToReg<[ECX]>>,

---------------------------------
[hope this doesn't get wrapped in your email client]

Unfortunately, this patch also changes how SRet/ThisCall behaves
non-Windows systems too (right?).

I'd like to ask for advice:
a) Is it OK to change the SRet/ThisCall behaviour on non-Windows platforms?
    [I suppose no]

b) Should I be altering CC_X86_32_ThisCall
    OR should I introduce CC_X86_Win32_ThisCall instead?
    [Answer not clear to me - is there any platform besides Windows
    that uses thiscall?]

Probably I need to create CC_X86_Win32_ThisCall (and maybe
CC_X86_Win32_C later) by copying CC_X86_32_ThisCall similar to how
CC_X86_Win64_C is done - does that sound right?

Hints and suggestions on fixing this are welcome!

--
Thanks,
Timur Iskhodzhanov,
Google Russia
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

Óscar Fuentes
Timur Iskhodzhanov <[hidden email]> writes:

> Hello Aaron, Anton, LLVM-dev,
>
> While working on http://llvm.org/PR13676#c6
> I found out that whenever I compile code with class methods returning
> structures it get generated incompatible with MSVC.

http://llvm.org/bugs/show_bug.cgi?id=5058

[snip]

> Hints and suggestions on fixing this are welcome!

LLVM supports the g++ C++ ABI. Your change breaks that. Please note that
this is not a Windows issue, it is a MSVC++ issue, as g++ works fine on
Windows too.

There are other issues with paramenter passing and the MSVC++ ABI, see
for instance

http://llvm.org/bugs/show_bug.cgi?id=5064

IIRC LLVM needs susbstantial modifications for supporting function calls
compatible with MSVC++, or the frontend that uses LLVM has to do quite a
bit of hacking for working around LLVM's limitations.

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

Anton Korobeynikov
In reply to this post by Timur Iskhodzhanov
Hello Timur,

> I'd like to ask for advice:
> a) Is it OK to change the SRet/ThisCall behaviour on non-Windows platforms?
>     [I suppose no]
no

>
> b) Should I be altering CC_X86_32_ThisCall
>     OR should I introduce CC_X86_Win32_ThisCall instead?
>     [Answer not clear to me - is there any platform besides Windows
>     that uses thiscall?]
no

It seems for me that you're trying to solve the problem from the wrong
end. As far as I remember, there is a difference - "simple" (probable
POD-like stuff) are returned in the regs, while classes with
non-trivial ctors, etc. are passed / returned on stack. It's frontend
responsibility to emit proper IR in this case.

See http://llvm.org/bugs/show_bug.cgi?id=5064 and around. This seems
to be the correct description of what's going on.

--
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

Aaron Ballman-2
In reply to this post by Timur Iskhodzhanov
Something to keep in mind...when I was researching it, thiscall was a
bit odd (at least to me) in MSVC.  It pushes the structure onto the
stack from the caller, but it returns the pointer to the structure in
EAX and cleans up the pushed stack.  The caller then either reloads
the structure from its local stack, or it uses EAX to access it.  The
latter is especially true if it involves a temp.

~Aaron

On Tue, Oct 2, 2012 at 10:34 AM, Timur Iskhodzhanov <[hidden email]> wrote:

> Hello Aaron, Anton, LLVM-dev,
>
> While working on http://llvm.org/PR13676#c6
> I found out that whenever I compile code with class methods returning
> structures it get generated incompatible with MSVC.
>
> Looking at lib/Target/X86/X86CallingConv.td, I found out that
> CC_X86_32_ThisCall maps SRet to EAX but in fact it should push
> the address of the return temp on stack.
>
> The following patch fixes the issue on Windows:
> ---------------------------------
> Index: lib/Target/X86/X86CallingConv.td
> ===================================================================
> --- lib/Target/X86/X86CallingConv.td      (revision 164763)
> +++ lib/Target/X86/X86CallingConv.td      (working copy)
> @@ -335,7 +335,8 @@
>    CCIfType<[i8, i16], CCPromoteToType<i32>>,
>
>    // Pass sret arguments indirectly through EAX
> -  CCIfSRet<CCAssignToReg<[EAX]>>,
> +  CCIfSRet<CCAssignToStack<4, 4>>,
>
>    // The first integer argument is passed in ECX
>    CCIfType<[i32], CCAssignToReg<[ECX]>>,
>
> ---------------------------------
> [hope this doesn't get wrapped in your email client]
>
> Unfortunately, this patch also changes how SRet/ThisCall behaves
> non-Windows systems too (right?).
>
> I'd like to ask for advice:
> a) Is it OK to change the SRet/ThisCall behaviour on non-Windows platforms?
>     [I suppose no]
>
> b) Should I be altering CC_X86_32_ThisCall
>     OR should I introduce CC_X86_Win32_ThisCall instead?
>     [Answer not clear to me - is there any platform besides Windows
>     that uses thiscall?]
>
> Probably I need to create CC_X86_Win32_ThisCall (and maybe
> CC_X86_Win32_C later) by copying CC_X86_32_ThisCall similar to how
> CC_X86_Win64_C is done - does that sound right?
>
> Hints and suggestions on fixing this are welcome!
>
> --
> Thanks,
> Timur Iskhodzhanov,
> Google Russia
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

Timur Iskhodzhanov
In reply to this post by Anton Korobeynikov
On Tue, Oct 2, 2012 at 7:03 PM, Anton Korobeynikov <[hidden email]> wrote:

> Hello Timur,
>
>> I'd like to ask for advice:
>> a) Is it OK to change the SRet/ThisCall behaviour on non-Windows platforms?
>>     [I suppose no]
> no
>
>>
>> b) Should I be altering CC_X86_32_ThisCall
>>     OR should I introduce CC_X86_Win32_ThisCall instead?
>>     [Answer not clear to me - is there any platform besides Windows
>>     that uses thiscall?]
> no
Can you please clarify which question you've answered here?
Sorry for making the ambiguous questions in the first place :)

> It seems for me that you're trying to solve the problem from the wrong
> end. As far as I remember, there is a difference - "simple" (probable
> POD-like stuff) are returned in the regs, while classes with
> non-trivial ctors, etc. are passed / returned on stack.
Sort of.

> It's frontend responsibility to emit proper IR in this case.
Isn't it what's SRet is supposed to be?

> See http://llvm.org/bugs/show_bug.cgi?id=5064 and around. This seems
> to be the correct description of what's going on.
FTR, http://llvm.org/bugs/show_bug.cgi?id=5058 seems to be more up-to-date.

Thanks for your reply!
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

Timur Iskhodzhanov
[+cfe-dev as this does seem like both LLVM+Clang issue]
[Sorry for an incomplete e-mail context, please see
http://llvm.org/PR13676#c6 if you're interested]

I've read these bugs and now I'm even more confused than I was before :)

What do you think about the following approach:
a) I'll create test cases for the major issues I've observed so far
   These test cases will check both -emit-llvm and llc output
   They'll have CHECKs for stuff that already works and
FIXME+CHECK-NOT for stuff that doesn't.
 I guess I should put these tests in clang/test/CodeGen[CXX] ?

b) As a short-term solution to avoid blocking progress for those who
are interested in a functioning Windows compiler I'll publish my patch
[which breaks the non-Windows compatibility but improves Windows
compat] in PR13676.

c) Having these test cases at hand, we can come up with a decent
long-term solution

Does that sound good to you?

On Tue, Oct 2, 2012 at 7:31 PM, Timur Iskhodzhanov <[hidden email]> wrote:

> On Tue, Oct 2, 2012 at 7:03 PM, Anton Korobeynikov <[hidden email]> wrote:
>> Hello Timur,
>>
>>> I'd like to ask for advice:
>>> a) Is it OK to change the SRet/ThisCall behaviour on non-Windows platforms?
>>>     [I suppose no]
>> no
>>
>>>
>>> b) Should I be altering CC_X86_32_ThisCall
>>>     OR should I introduce CC_X86_Win32_ThisCall instead?
>>>     [Answer not clear to me - is there any platform besides Windows
>>>     that uses thiscall?]
>> no
> Can you please clarify which question you've answered here?
> Sorry for making the ambiguous questions in the first place :)
>
>> It seems for me that you're trying to solve the problem from the wrong
>> end. As far as I remember, there is a difference - "simple" (probable
>> POD-like stuff) are returned in the regs, while classes with
>> non-trivial ctors, etc. are passed / returned on stack.
> Sort of.
>
>> It's frontend responsibility to emit proper IR in this case.
> Isn't it what's SRet is supposed to be?
>
>> See http://llvm.org/bugs/show_bug.cgi?id=5064 and around. This seems
>> to be the correct description of what's going on.
> FTR, http://llvm.org/bugs/show_bug.cgi?id=5058 seems to be more up-to-date.
>
> Thanks for your reply!
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

Aaron Ballman-2
On Tue, Oct 2, 2012 at 11:54 AM, Timur Iskhodzhanov <[hidden email]> wrote:

> [+cfe-dev as this does seem like both LLVM+Clang issue]
> [Sorry for an incomplete e-mail context, please see
> http://llvm.org/PR13676#c6 if you're interested]
>
> I've read these bugs and now I'm even more confused than I was before :)
>
> What do you think about the following approach:
> a) I'll create test cases for the major issues I've observed so far
>    These test cases will check both -emit-llvm and llc output
>    They'll have CHECKs for stuff that already works and
> FIXME+CHECK-NOT for stuff that doesn't.
>  I guess I should put these tests in clang/test/CodeGen[CXX] ?

Makes sense, assuming we do comparisons against what MSVC 10/11 and
gcc emit to determine CHECK vs CHECK-NOT.

> b) As a short-term solution to avoid blocking progress for those who
> are interested in a functioning Windows compiler I'll publish my patch
> [which breaks the non-Windows compatibility but improves Windows
> compat] in PR13676.

Assuming the patch addresses all of your tests, then that makes sense.

> c) Having these test cases at hand, we can come up with a decent
> long-term solution

Also good.

Thanks for looking into this!

~Aaron
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

David Blaikie
In reply to this post by Timur Iskhodzhanov
On Tue, Oct 2, 2012 at 8:54 AM, Timur Iskhodzhanov <[hidden email]> wrote:
> [+cfe-dev as this does seem like both LLVM+Clang issue]
> [Sorry for an incomplete e-mail context, please see
> http://llvm.org/PR13676#c6 if you're interested]
>
> I've read these bugs and now I'm even more confused than I was before :)
>
> What do you think about the following approach:
> a) I'll create test cases for the major issues I've observed so far
>    These test cases will check both -emit-llvm and llc output

Just an aside: generally Clang tests should just verify the emitted
LLVM bitcode. If you want to test what machine code/assembly that
compiles down to, that should be an LLVM test that starts at LLVM
bitcode and goes down to machine code/assembly.

>    They'll have CHECKs for stuff that already works and
> FIXME+CHECK-NOT for stuff that doesn't.
>  I guess I should put these tests in clang/test/CodeGen[CXX] ?
>
> b) As a short-term solution to avoid blocking progress for those who
> are interested in a functioning Windows compiler I'll publish my patch
> [which breaks the non-Windows compatibility but improves Windows
> compat] in PR13676.
>
> c) Having these test cases at hand, we can come up with a decent
> long-term solution
>
> Does that sound good to you?
>
> On Tue, Oct 2, 2012 at 7:31 PM, Timur Iskhodzhanov <[hidden email]> wrote:
>> On Tue, Oct 2, 2012 at 7:03 PM, Anton Korobeynikov <[hidden email]> wrote:
>>> Hello Timur,
>>>
>>>> I'd like to ask for advice:
>>>> a) Is it OK to change the SRet/ThisCall behaviour on non-Windows platforms?
>>>>     [I suppose no]
>>> no
>>>
>>>>
>>>> b) Should I be altering CC_X86_32_ThisCall
>>>>     OR should I introduce CC_X86_Win32_ThisCall instead?
>>>>     [Answer not clear to me - is there any platform besides Windows
>>>>     that uses thiscall?]
>>> no
>> Can you please clarify which question you've answered here?
>> Sorry for making the ambiguous questions in the first place :)
>>
>>> It seems for me that you're trying to solve the problem from the wrong
>>> end. As far as I remember, there is a difference - "simple" (probable
>>> POD-like stuff) are returned in the regs, while classes with
>>> non-trivial ctors, etc. are passed / returned on stack.
>> Sort of.
>>
>>> It's frontend responsibility to emit proper IR in this case.
>> Isn't it what's SRet is supposed to be?
>>
>>> See http://llvm.org/bugs/show_bug.cgi?id=5064 and around. This seems
>>> to be the correct description of what's going on.
>> FTR, http://llvm.org/bugs/show_bug.cgi?id=5058 seems to be more up-to-date.
>>
>> Thanks for your reply!
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

Anton Korobeynikov
In reply to this post by Timur Iskhodzhanov
Hello Timur,

>>> b) Should I be altering CC_X86_32_ThisCall
>>>     OR should I introduce CC_X86_Win32_ThisCall instead?
>>>     [Answer not clear to me - is there any platform besides Windows
>>>     that uses thiscall?]
>> no
> Can you please clarify which question you've answered here?
To both. You're assuming that Windows == MSVC. This is not true.

>> It's frontend responsibility to emit proper IR in this case.
> Isn't it what's SRet is supposed to be?
Yes and no. Yes, when source language / compiler semantics is
low-level enough, so it can be represented via single attribute. And
no - when there will be additional frontend job to "lift" the stuff.

--
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

Anton Korobeynikov
In reply to this post by Timur Iskhodzhanov
> b) As a short-term solution to avoid blocking progress for those who
> are interested in a functioning Windows compiler I'll publish my patch
> [which breaks the non-Windows compatibility but improves Windows
> compat] in PR13676.
It does not improve Windows compatibility, because it will break
mingw. It will only improve msvc compatibility, though I really doubt
that it will cover everything besides trivial cases.

--
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

Timur Iskhodzhanov
In reply to this post by David Blaikie
On Tue, Oct 2, 2012 at 8:28 PM, David Blaikie <[hidden email]> wrote:

> On Tue, Oct 2, 2012 at 8:54 AM, Timur Iskhodzhanov <[hidden email]> wrote:
>> [+cfe-dev as this does seem like both LLVM+Clang issue]
>> [Sorry for an incomplete e-mail context, please see
>> http://llvm.org/PR13676#c6 if you're interested]
>>
>> I've read these bugs and now I'm even more confused than I was before :)
>>
>> What do you think about the following approach:
>> a) I'll create test cases for the major issues I've observed so far
>>    These test cases will check both -emit-llvm and llc output
>
> Just an aside: generally Clang tests should just verify the emitted
> LLVM bitcode.
I know about that, see below.

> If you want to test what machine code/assembly that
> compiles down to, that should be an LLVM test that starts at LLVM
> bitcode and goes down to machine code/assembly.
Are there any serious reasons not to do combined -emit-llvm+llc tests
for such issues that need both LLVM and Clang support?
I understand usually it's easy to split but in this particular case it
seems like the generated bitcode might need to be changed during Clang
patching.
In this particular case I believe having a combined test is much more
convenient.
WDYT?

>>    They'll have CHECKs for stuff that already works and
>> FIXME+CHECK-NOT for stuff that doesn't.
>>  I guess I should put these tests in clang/test/CodeGen[CXX] ?
>>
>> b) As a short-term solution to avoid blocking progress for those who
>> are interested in a functioning Windows compiler I'll publish my patch
>> [which breaks the non-Windows compatibility but improves Windows
>> compat] in PR13676.
>>
>> c) Having these test cases at hand, we can come up with a decent
>> long-term solution
>>
>> Does that sound good to you?
>>
>> On Tue, Oct 2, 2012 at 7:31 PM, Timur Iskhodzhanov <[hidden email]> wrote:
>>> On Tue, Oct 2, 2012 at 7:03 PM, Anton Korobeynikov <[hidden email]> wrote:
>>>> Hello Timur,
>>>>
>>>>> I'd like to ask for advice:
>>>>> a) Is it OK to change the SRet/ThisCall behaviour on non-Windows platforms?
>>>>>     [I suppose no]
>>>> no
>>>>
>>>>>
>>>>> b) Should I be altering CC_X86_32_ThisCall
>>>>>     OR should I introduce CC_X86_Win32_ThisCall instead?
>>>>>     [Answer not clear to me - is there any platform besides Windows
>>>>>     that uses thiscall?]
>>>> no
>>> Can you please clarify which question you've answered here?
>>> Sorry for making the ambiguous questions in the first place :)
>>>
>>>> It seems for me that you're trying to solve the problem from the wrong
>>>> end. As far as I remember, there is a difference - "simple" (probable
>>>> POD-like stuff) are returned in the regs, while classes with
>>>> non-trivial ctors, etc. are passed / returned on stack.
>>> Sort of.
>>>
>>>> It's frontend responsibility to emit proper IR in this case.
>>> Isn't it what's SRet is supposed to be?
>>>
>>>> See http://llvm.org/bugs/show_bug.cgi?id=5064 and around. This seems
>>>> to be the correct description of what's going on.
>>> FTR, http://llvm.org/bugs/show_bug.cgi?id=5058 seems to be more up-to-date.
>>>
>>> Thanks for your reply!
>> _______________________________________________
>> LLVM Developers mailing list
>> [hidden email]         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

David Blaikie
On Tue, Oct 2, 2012 at 11:02 AM, Timur Iskhodzhanov <[hidden email]> wrote:

> On Tue, Oct 2, 2012 at 8:28 PM, David Blaikie <[hidden email]> wrote:
>> On Tue, Oct 2, 2012 at 8:54 AM, Timur Iskhodzhanov <[hidden email]> wrote:
>>> [+cfe-dev as this does seem like both LLVM+Clang issue]
>>> [Sorry for an incomplete e-mail context, please see
>>> http://llvm.org/PR13676#c6 if you're interested]
>>>
>>> I've read these bugs and now I'm even more confused than I was before :)
>>>
>>> What do you think about the following approach:
>>> a) I'll create test cases for the major issues I've observed so far
>>>    These test cases will check both -emit-llvm and llc output
>>
>> Just an aside: generally Clang tests should just verify the emitted
>> LLVM bitcode.
> I know about that, see below.
>
>> If you want to test what machine code/assembly that
>> compiles down to, that should be an LLVM test that starts at LLVM
>> bitcode and goes down to machine code/assembly.
> Are there any serious reasons not to do combined -emit-llvm+llc tests
> for such issues that need both LLVM and Clang support?
> I understand usually it's easy to split but in this particular case it
> seems like the generated bitcode might need to be changed during Clang
> patching.

I'm not sure I follow your reasoning here. That Clang will produce
different bitcode wouldn't change the desire to test these components
separately.

* when the tests fail we have a more precise idea about what went wrong.
* less duplicated test work - we should be testing the LLVM bitcode
features once in the LLVM suite, not once for each different client
use case
* better chance of catching regressions - if we test LLVM in the Clang
suite, those working only on LLVM might regress Clang without
realizing it

> In this particular case I believe having a combined test is much more
> convenient.
> WDYT?
>
>>>    They'll have CHECKs for stuff that already works and
>>> FIXME+CHECK-NOT for stuff that doesn't.
>>>  I guess I should put these tests in clang/test/CodeGen[CXX] ?
>>>
>>> b) As a short-term solution to avoid blocking progress for those who
>>> are interested in a functioning Windows compiler I'll publish my patch
>>> [which breaks the non-Windows compatibility but improves Windows
>>> compat] in PR13676.
>>>
>>> c) Having these test cases at hand, we can come up with a decent
>>> long-term solution
>>>
>>> Does that sound good to you?
>>>
>>> On Tue, Oct 2, 2012 at 7:31 PM, Timur Iskhodzhanov <[hidden email]> wrote:
>>>> On Tue, Oct 2, 2012 at 7:03 PM, Anton Korobeynikov <[hidden email]> wrote:
>>>>> Hello Timur,
>>>>>
>>>>>> I'd like to ask for advice:
>>>>>> a) Is it OK to change the SRet/ThisCall behaviour on non-Windows platforms?
>>>>>>     [I suppose no]
>>>>> no
>>>>>
>>>>>>
>>>>>> b) Should I be altering CC_X86_32_ThisCall
>>>>>>     OR should I introduce CC_X86_Win32_ThisCall instead?
>>>>>>     [Answer not clear to me - is there any platform besides Windows
>>>>>>     that uses thiscall?]
>>>>> no
>>>> Can you please clarify which question you've answered here?
>>>> Sorry for making the ambiguous questions in the first place :)
>>>>
>>>>> It seems for me that you're trying to solve the problem from the wrong
>>>>> end. As far as I remember, there is a difference - "simple" (probable
>>>>> POD-like stuff) are returned in the regs, while classes with
>>>>> non-trivial ctors, etc. are passed / returned on stack.
>>>> Sort of.
>>>>
>>>>> It's frontend responsibility to emit proper IR in this case.
>>>> Isn't it what's SRet is supposed to be?
>>>>
>>>>> See http://llvm.org/bugs/show_bug.cgi?id=5064 and around. This seems
>>>>> to be the correct description of what's going on.
>>>> FTR, http://llvm.org/bugs/show_bug.cgi?id=5058 seems to be more up-to-date.
>>>>
>>>> Thanks for your reply!
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> [hidden email]         http://llvm.cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

Timur Iskhodzhanov
On Tue, Oct 2, 2012 at 8:44 PM, Anton Korobeynikov <[hidden email]> wrote:
>>>> b) Should I be altering CC_X86_32_ThisCall
>>>>     OR should I introduce CC_X86_Win32_ThisCall instead?
>>>>     [Answer not clear to me - is there any platform besides Windows
>>>>     that uses thiscall?]
>>> no
>> Can you please clarify which question you've answered here?
> To both. You're assuming that Windows == MSVC. This is not true.
Ah, got it.
Sounds like we might need to introduce CC_X86_Win32_MSVC_ThisCall then?..

On Tue, Oct 2, 2012 at 10:22 PM, David Blaikie <[hidden email]> wrote:

> On Tue, Oct 2, 2012 at 11:02 AM, Timur Iskhodzhanov <[hidden email]> wrote:
>> On Tue, Oct 2, 2012 at 8:28 PM, David Blaikie <[hidden email]> wrote:
>>> On Tue, Oct 2, 2012 at 8:54 AM, Timur Iskhodzhanov <[hidden email]> wrote:
>>>> [+cfe-dev as this does seem like both LLVM+Clang issue]
>>>> [Sorry for an incomplete e-mail context, please see
>>>> http://llvm.org/PR13676#c6 if you're interested]
>>>>
>>>> I've read these bugs and now I'm even more confused than I was before :)
>>>>
>>>> What do you think about the following approach:
>>>> a) I'll create test cases for the major issues I've observed so far
>>>>    These test cases will check both -emit-llvm and llc output
>>>
>>> Just an aside: generally Clang tests should just verify the emitted
>>> LLVM bitcode.
>> I know about that, see below.
>>
>>> If you want to test what machine code/assembly that
>>> compiles down to, that should be an LLVM test that starts at LLVM
>>> bitcode and goes down to machine code/assembly.
>> Are there any serious reasons not to do combined -emit-llvm+llc tests
>> for such issues that need both LLVM and Clang support?
>> I understand usually it's easy to split but in this particular case it
>> seems like the generated bitcode might need to be changed during Clang
>> patching.
>
> I'm not sure I follow your reasoning here. That Clang will produce
> different bitcode wouldn't change the desire to test these components
> separately.
>
> * when the tests fail we have a more precise idea about what went wrong.
> * less duplicated test work - we should be testing the LLVM bitcode
> features once in the LLVM suite, not once for each different client
> use case
> * better chance of catching regressions - if we test LLVM in the Clang
> suite, those working only on LLVM might regress Clang without
> realizing it
I got your point.
I'll start with combined clang+llc tests locally without committing it
and split into separate clang and llc tests before sending out a final
patch for review then.
I do understand the convenience of "more precise idea of what went
wrong" but I'm not sure it'll help much during early development as
it's not clear what is the "right" LL code is for a give assembly :)
There might be many options (e.g. SRet or not SRet? ThisCall or
MSVC_ThisCall? etc)

Does that sound reasonable this way?

>> In this particular case I believe having a combined test is much more
>> convenient.
>> WDYT?
>>
>>>>    They'll have CHECKs for stuff that already works and
>>>> FIXME+CHECK-NOT for stuff that doesn't.
>>>>  I guess I should put these tests in clang/test/CodeGen[CXX] ?
>>>>
>>>> b) As a short-term solution to avoid blocking progress for those who
>>>> are interested in a functioning Windows compiler I'll publish my patch
>>>> [which breaks the non-Windows compatibility but improves Windows
>>>> compat] in PR13676.
>>>>
>>>> c) Having these test cases at hand, we can come up with a decent
>>>> long-term solution
>>>>
>>>> Does that sound good to you?
>>>>
>>>> On Tue, Oct 2, 2012 at 7:31 PM, Timur Iskhodzhanov <[hidden email]> wrote:
>>>>> On Tue, Oct 2, 2012 at 7:03 PM, Anton Korobeynikov <[hidden email]> wrote:
>>>>>> Hello Timur,
>>>>>>
>>>>>>> I'd like to ask for advice:
>>>>>>> a) Is it OK to change the SRet/ThisCall behaviour on non-Windows platforms?
>>>>>>>     [I suppose no]
>>>>>> no
>>>>>>
>>>>>>>
>>>>>>> b) Should I be altering CC_X86_32_ThisCall
>>>>>>>     OR should I introduce CC_X86_Win32_ThisCall instead?
>>>>>>>     [Answer not clear to me - is there any platform besides Windows
>>>>>>>     that uses thiscall?]
>>>>>> no
>>>>> Can you please clarify which question you've answered here?
>>>>> Sorry for making the ambiguous questions in the first place :)
>>>>>
>>>>>> It seems for me that you're trying to solve the problem from the wrong
>>>>>> end. As far as I remember, there is a difference - "simple" (probable
>>>>>> POD-like stuff) are returned in the regs, while classes with
>>>>>> non-trivial ctors, etc. are passed / returned on stack.
>>>>> Sort of.
>>>>>
>>>>>> It's frontend responsibility to emit proper IR in this case.
>>>>> Isn't it what's SRet is supposed to be?
>>>>>
>>>>>> See http://llvm.org/bugs/show_bug.cgi?id=5064 and around. This seems
>>>>>> to be the correct description of what's going on.
>>>>> FTR, http://llvm.org/bugs/show_bug.cgi?id=5058 seems to be more up-to-date.
>>>>>
>>>>> Thanks for your reply!
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> [hidden email]         http://llvm.cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Handling SRet on Windows x86

Anton Korobeynikov
> Ah, got it.
> Sounds like we might need to introduce CC_X86_Win32_MSVC_ThisCall then?..
No, we should not. It should be properly expanded in frontend.

--
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Handling SRet on Windows x86

Timur Iskhodzhanov
How can a frontend tell LLVM to put a function argument on stack/register/etc?

On Thu, Oct 4, 2012 at 5:08 PM, Anton Korobeynikov <[hidden email]> wrote:
>> Ah, got it.
>> Sounds like we might need to introduce CC_X86_Win32_MSVC_ThisCall then?..
> No, we should not. It should be properly expanded in frontend.
>
> --
> With best regards, Anton Korobeynikov
> Faculty of Mathematics and Mechanics, Saint Petersburg State University
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Handling SRet on Windows x86

David Blaikie
In reply to this post by Timur Iskhodzhanov
On Thu, Oct 4, 2012 at 5:19 AM, Timur Iskhodzhanov <[hidden email]> wrote:

> On Tue, Oct 2, 2012 at 8:44 PM, Anton Korobeynikov <[hidden email]> wrote:
>>>>> b) Should I be altering CC_X86_32_ThisCall
>>>>>     OR should I introduce CC_X86_Win32_ThisCall instead?
>>>>>     [Answer not clear to me - is there any platform besides Windows
>>>>>     that uses thiscall?]
>>>> no
>>> Can you please clarify which question you've answered here?
>> To both. You're assuming that Windows == MSVC. This is not true.
> Ah, got it.
> Sounds like we might need to introduce CC_X86_Win32_MSVC_ThisCall then?..
>
> On Tue, Oct 2, 2012 at 10:22 PM, David Blaikie <[hidden email]> wrote:
>> On Tue, Oct 2, 2012 at 11:02 AM, Timur Iskhodzhanov <[hidden email]> wrote:
>>> On Tue, Oct 2, 2012 at 8:28 PM, David Blaikie <[hidden email]> wrote:
>>>> On Tue, Oct 2, 2012 at 8:54 AM, Timur Iskhodzhanov <[hidden email]> wrote:
>>>>> [+cfe-dev as this does seem like both LLVM+Clang issue]
>>>>> [Sorry for an incomplete e-mail context, please see
>>>>> http://llvm.org/PR13676#c6 if you're interested]
>>>>>
>>>>> I've read these bugs and now I'm even more confused than I was before :)
>>>>>
>>>>> What do you think about the following approach:
>>>>> a) I'll create test cases for the major issues I've observed so far
>>>>>    These test cases will check both -emit-llvm and llc output
>>>>
>>>> Just an aside: generally Clang tests should just verify the emitted
>>>> LLVM bitcode.
>>> I know about that, see below.
>>>
>>>> If you want to test what machine code/assembly that
>>>> compiles down to, that should be an LLVM test that starts at LLVM
>>>> bitcode and goes down to machine code/assembly.
>>> Are there any serious reasons not to do combined -emit-llvm+llc tests
>>> for such issues that need both LLVM and Clang support?
>>> I understand usually it's easy to split but in this particular case it
>>> seems like the generated bitcode might need to be changed during Clang
>>> patching.
>>
>> I'm not sure I follow your reasoning here. That Clang will produce
>> different bitcode wouldn't change the desire to test these components
>> separately.
>>
>> * when the tests fail we have a more precise idea about what went wrong.
>> * less duplicated test work - we should be testing the LLVM bitcode
>> features once in the LLVM suite, not once for each different client
>> use case
>> * better chance of catching regressions - if we test LLVM in the Clang
>> suite, those working only on LLVM might regress Clang without
>> realizing it
> I got your point.
> I'll start with combined clang+llc tests locally without committing it
> and split into separate clang and llc tests before sending out a final
> patch for review then.
> I do understand the convenience of "more precise idea of what went
> wrong" but I'm not sure it'll help much during early development as
> it's not clear what is the "right" LL code is for a give assembly :)

That's good/fine. Though I'd keep in mind that the LLVM-level
functionality you're testing may already be covered by existing test
cases (so if you get to the end, find the bitcode you want to test,
make sure it's actually adding new/valuable testing to LLVM, otherwise
just skip it & rely on the existing test coverage if it suffices)

- David

> There might be many options (e.g. SRet or not SRet? ThisCall or
> MSVC_ThisCall? etc)
>
> Does that sound reasonable this way?
>
>>> In this particular case I believe having a combined test is much more
>>> convenient.
>>> WDYT?
>>>
>>>>>    They'll have CHECKs for stuff that already works and
>>>>> FIXME+CHECK-NOT for stuff that doesn't.
>>>>>  I guess I should put these tests in clang/test/CodeGen[CXX] ?
>>>>>
>>>>> b) As a short-term solution to avoid blocking progress for those who
>>>>> are interested in a functioning Windows compiler I'll publish my patch
>>>>> [which breaks the non-Windows compatibility but improves Windows
>>>>> compat] in PR13676.
>>>>>
>>>>> c) Having these test cases at hand, we can come up with a decent
>>>>> long-term solution
>>>>>
>>>>> Does that sound good to you?
>>>>>
>>>>> On Tue, Oct 2, 2012 at 7:31 PM, Timur Iskhodzhanov <[hidden email]> wrote:
>>>>>> On Tue, Oct 2, 2012 at 7:03 PM, Anton Korobeynikov <[hidden email]> wrote:
>>>>>>> Hello Timur,
>>>>>>>
>>>>>>>> I'd like to ask for advice:
>>>>>>>> a) Is it OK to change the SRet/ThisCall behaviour on non-Windows platforms?
>>>>>>>>     [I suppose no]
>>>>>>> no
>>>>>>>
>>>>>>>>
>>>>>>>> b) Should I be altering CC_X86_32_ThisCall
>>>>>>>>     OR should I introduce CC_X86_Win32_ThisCall instead?
>>>>>>>>     [Answer not clear to me - is there any platform besides Windows
>>>>>>>>     that uses thiscall?]
>>>>>>> no
>>>>>> Can you please clarify which question you've answered here?
>>>>>> Sorry for making the ambiguous questions in the first place :)
>>>>>>
>>>>>>> It seems for me that you're trying to solve the problem from the wrong
>>>>>>> end. As far as I remember, there is a difference - "simple" (probable
>>>>>>> POD-like stuff) are returned in the regs, while classes with
>>>>>>> non-trivial ctors, etc. are passed / returned on stack.
>>>>>> Sort of.
>>>>>>
>>>>>>> It's frontend responsibility to emit proper IR in this case.
>>>>>> Isn't it what's SRet is supposed to be?
>>>>>>
>>>>>>> See http://llvm.org/bugs/show_bug.cgi?id=5064 and around. This seems
>>>>>>> to be the correct description of what's going on.
>>>>>> FTR, http://llvm.org/bugs/show_bug.cgi?id=5058 seems to be more up-to-date.
>>>>>>
>>>>>> Thanks for your reply!
>>>>> _______________________________________________
>>>>> LLVM Developers mailing list
>>>>> [hidden email]         http://llvm.cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Handling SRet on Windows x86

Timur Iskhodzhanov
In reply to this post by Timur Iskhodzhanov
ping?

On Thu, Oct 4, 2012 at 5:10 PM, Timur Iskhodzhanov <[hidden email]> wrote:

> How can a frontend tell LLVM to put a function argument on stack/register/etc?
>
> On Thu, Oct 4, 2012 at 5:08 PM, Anton Korobeynikov <[hidden email]> wrote:
>>> Ah, got it.
>>> Sounds like we might need to introduce CC_X86_Win32_MSVC_ThisCall then?..
>> No, we should not. It should be properly expanded in frontend.
>>
>> --
>> With best regards, Anton Korobeynikov
>> Faculty of Mathematics and Mechanics, Saint Petersburg State University
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Handling SRet on Windows x86

Timur Iskhodzhanov
In reply to this post by Timur Iskhodzhanov
Anton,

[+Eric, Nick,
the e-mail thread context has been broken a few times, so you should
probably look at the llvmdev archives.
It all starts here:
http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-October/053961.html ]

So I've decided to make a new attempt to fix this issue.

We've discussed my patches with Eric and came up with the following idea:
a) IsTargetWindows should be true for all the targets that execute on
the Windows kernel, including Cygwin, MinGW and MSVC ABIs.

However, "Windows 32-bit ABI" usually implies "MSVC 32-bit ABI", so
it's Cygwin and MinGW (that are similar more similar to Itanium ABI
than to MSVC ABI) that should be exceptions from the general Win32
handling code.

That is,
b) We should add a IsTargetWin32 which is true if the MSVC 32-bit ABI
is used and false for Cygwin+MinGW.
c) We should add CC_X86_Win32_C and CC_X86_Win32_ThisCall and use them
only for MSVC 32-bit ABI.
d) Cygwin and MinGW should use the CC_X86_32_C

This way, Clang takes care of setting the SRet attribute wherever appropriate
and LLVM takes care of putting such return values onto stack in the
Win32/MSVC32 ABI.

What do you think about such a proposal?

One more data point: the ABI is wrong even in C!
http://llvm.org/bugs/show_bug.cgi?id=15556

--
Timur
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Handling SRet on Windows x86

Timur Iskhodzhanov
2013/3/20 Timur Iskhodzhanov <[hidden email]>:

> Anton,
>
> [+Eric, Nick,
> the e-mail thread context has been broken a few times, so you should
> probably look at the llvmdev archives.
> It all starts here:
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-October/053961.html ]
>
> So I've decided to make a new attempt to fix this issue.
>
> We've discussed my patches with Eric and came up with the following idea:
> a) IsTargetWindows should be true for all the targets that execute on
> the Windows kernel, including Cygwin, MinGW and MSVC ABIs.
>
> However, "Windows 32-bit ABI" usually implies "MSVC 32-bit ABI", so
> it's Cygwin and MinGW (that are similar more similar to Itanium ABI
> than to MSVC ABI) that should be exceptions from the general Win32
> handling code.
>
> That is,
> b) We should add a IsTargetWin32 which is true if the MSVC 32-bit ABI
> is used and false for Cygwin+MinGW.
FTR, there's already such a method but it is (mis?)used only in
lib/Target/X86/X86FrameLowering.cpp for segmented stacks.
Are segmented stacks actually used with MSVC or is it just the
conditions there should use isTargetMingw() && !is64Bit() instead?

> c) We should add CC_X86_Win32_C and CC_X86_Win32_ThisCall and use them
> only for MSVC 32-bit ABI.
> d) Cygwin and MinGW should use the CC_X86_32_C
>
> This way, Clang takes care of setting the SRet attribute wherever appropriate
> and LLVM takes care of putting such return values onto stack in the
> Win32/MSVC32 ABI.
>
> What do you think about such a proposal?
>
> One more data point: the ABI is wrong even in C!
> http://llvm.org/bugs/show_bug.cgi?id=15556
>
> --
> Timur
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Handling SRet on Windows x86

Eric Christopher



On Wed, Mar 20, 2013 at 5:21 PM, Timur Iskhodzhanov <[hidden email]> wrote:
2013/3/20 Timur Iskhodzhanov <[hidden email]>:
> Anton,
>
> [+Eric, Nick,
> the e-mail thread context has been broken a few times, so you should
> probably look at the llvmdev archives.
> It all starts here:
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-October/053961.html ]
>
> So I've decided to make a new attempt to fix this issue.
>
> We've discussed my patches with Eric and came up with the following idea:
> a) IsTargetWindows should be true for all the targets that execute on
> the Windows kernel, including Cygwin, MinGW and MSVC ABIs.
>
> However, "Windows 32-bit ABI" usually implies "MSVC 32-bit ABI", so
> it's Cygwin and MinGW (that are similar more similar to Itanium ABI
> than to MSVC ABI) that should be exceptions from the general Win32
> handling code.
>
> That is,
> b) We should add a IsTargetWin32 which is true if the MSVC 32-bit ABI
> is used and false for Cygwin+MinGW.
FTR, there's already such a method but it is (mis?)used only in
lib/Target/X86/X86FrameLowering.cpp for segmented stacks.
Are segmented stacks actually used with MSVC or is it just the
conditions there should use isTargetMingw() && !is64Bit() instead?


cc'ing graydon about segmented stacks because I know rust uses them.

-eric 


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
123