[PATCH] Add a "probe-stack" attribute

[John Kåre Alsaker]

I would prefer to see the important feature land first (that is the first 4
commits here
https://github.com/Zoxc/llvm/compare/llvm-mirror:master...stprobe) since I
actually need those and not the optimization.

On Fri, Aug 22, 2014 at 7:20 PM, Philip Reames <listmail at philipreames.com>
wrote:

>
> 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
> > 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).
>
> 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/20140916/38b12014/attachment.html>