[PATCH] Add a "probe-stack" attribute

Wed Sep 17 16:53:29 PDT 2014

John & Anton,

I would be perfectly happy to have another reviewer take over here if 
you'd prefer and can find someone willing to do so.  So far, no one else 
has spoken up.  I'm not prepared to commit the code as of the last 
revision I saw.  I'm happy to work with you towards that goal, but 
recognize the (reasonable) frustration of working with a reviewer who is 
not familiar with the code being changed and is thus forced to be 
conservative about understanding each piece.

My apologies if the process has been unnecessarily frustrating. It's 
entirely possible I could have handled this better.  If you have 
specific suggestions, feel free to send me a private email off list.  
I'm always happy to hear constructive criticism and learn from past 
experiences.

p.s. There was a potential issue brought up with the optimization 
change.  I'm just calling your attention to the matter in case you 
missed it.  http://reviews.llvm.org/D5018#inline-43027

Philip

On 09/16/2014 02:30 AM, Anton Korobeynikov wrote:
> I would suggest that someone from code owners review these changes.
>
> On Tue, Sep 16, 2014 at 4:44 AM, John Kåre Alsaker
> <john.mailinglists at gmail.com> wrote:
>> 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.
>>>>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>