[PATCH] Add a "probe-stack" attribute

[Philip Reames]

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 <mailto: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 <mailto: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.

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.
>
>>     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/20140821/18f601fa/attachment.html>