[PATCH] Add a "probe-stack" attribute

Wed Nov 5 20:02:08 PST 2014

Poke.

On Tue, Oct 14, 2014 at 10:41 PM, John Kåre Alsaker
<john.mailinglists at gmail.com> wrote:
> I've CC'd Nadav Rotem. Hopefully he can find someone to review the x86 changes.
>
> I'll leave out the optimizations for now because of the issue raised.
> I've attached the patches I'd like to have merged.
>
> - John Kåre
>
> On Thu, Sep 18, 2014 at 1:53 AM, Philip Reames
> <listmail at philipreames.com> wrote:
>> 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
>>>>
>>>
>>>
>>