[PATCH] Add a "probe-stack" attribute

[Anton Korobeynikov]

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
>



-- 
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University