[PATCH] Add a "probe-stack" attribute

[John Kåre Alsaker]

On Fri, Aug 22, 2014 at 2:01 AM, Philip Reames <listmail at philipreames.com>
wrote:

>
> On 08/18/2014 12:40 PM, John Kåre Alsaker wrote:
>
>  On Mon, Aug 18, 2014 at 8:31 PM, Philip Reames <listmail at philipreames.com
> > wrote:
>
>>
>> On 08/15/2014 01:55 PM, John Kåre Alsaker wrote:
>>
>>  On Fri, Aug 15, 2014 at 6:47 PM, Philip Reames <
>> listmail at philipreames.com> wrote:
>>
>>> Sorry for the delay.
>>>
>>> Patch 0002 LGTM.  Please submit
>>> Patch 0003 LGTM.  Please submit.
>>>
>> Yes, please submit.
>>
>>  Do you have commit access?  Or do you want me to submit on your behalf?
>>
> I'd like you to submit them.
>
> I've landed the first couple of minor changes from the series.  Mostly as
> a show of good faith since the later items still need work.  If I missed
> something you think should have landed (i.e. got a LGTM), let me know.
>
> I split the 5 page optimization into it's own review and added a couple of
> folks with Windows experience.  I was all the way up to submitting it and
> realized I didn't feel comfortable doing so.  The thread is "Fast-path for
> stack probes on smaller frames" at http://reviews.llvm.org/D5018.
>
>
>
>>
>>
>>
>>>
>>> Patch 0001 LGTM, but doesn't do anything by itself.  Please hold it
>>> until either 0004 or 0006 are ready to land.
>>>
>>> Patch 0004
>>> - It seems like you're strill mixing several functional changes in
>>> 0004.  Can you enumerate (in email) what this patch is trying to
>>> accomplish?  The trivial implementation I expected is to add
>>> MF.shouldProbeStack in a couple places and call it done.  Why is the rest
>>> needed?
>>>
>> It's just generalizing the code for Win64 stack probes to x86-32 and
>> x86-64. The __probestack calling convention on both x86-32 and x86-64 is
>> the same as __chkstk on Win64.
>>
>>  Why?  Is there a good reason now to use the x86-32 chkstk convention on
>> x86-32 probestack?  What's the motivation for being different?
>>
> I'm being consistent across x86 while MSVC is not.
>
> Breaking compatibility with the platform compiler is almost never
> acceptable and requires a substantial justification.  If you want to make
> the argument that that is the right direction to take, you'll need to find
> someone else to review your work.  I am nowhere *near* knowledgeable enough
> in this area to approve such a change.
>
It does not break anything. __probestack is a new non-Windows function. On
Windows the existing methods are called as before (and "probe-stack" is a
no-op).


>
> What is your actual use case?  Are you trying to do stack probing on
> non-windows platforms?  Are you using a custom runtime on Windows?  I'm
> asking for background so I can make constructive suggestions.
>
>
>
>>    In addition it doesn't assume RAX never is live in.
>>
>>  Why?  Is this a bug fix?
>>
> That depends on if it ever can be live in. There were already logic for
> EAX which I generalized.
>
> Is this change needed for anything else?  I would prefer not to change
> code without reason.  Doing so only increases risk.
>
I don't know if the existing assumption that RAX is never live-in holds now
that this code can be used on non-Windows platforms.


>       It also disables the redzone (which never is used on Windows) since
>> the probing code doesn't account for that.
>>
>>  Ok.  I see the value in this.
>>
>> This sounds like at least three distinct changes. Please separate them
>> and we'll review each in turn.
>>
>> Sorry for being so pedantic about separation here.  I'm trying to review
>> code I don't really know well.  As a result, I'm trying to be extremely
>> carefully about making sure I understand each piece.  If you can find
>> another reviewer who knows this code better, I'd be happy to step aside.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140822/2a423960/attachment.html>