[PATCH] Add a "probe-stack" attribute

[Philip Reames]

On 08/22/2014 12:43 AM, John Kåre Alsaker wrote:
> On Fri, Aug 22, 2014 at 2:01 AM, Philip Reames 
> <listmail at philipreames.com <mailto: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 <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.
>
> 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).
Thank you for the clarification.  I had misunderstood your point.

I'll take another look at submitting this once the 5-page opt goes in.
>
>
>     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.
OK, that makes sense.
>
>>>         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/aea47f35/attachment.html>