<div dir="ltr">patch 1 lgtm<div>patch 2 lgtm</div><div><br></div><div>I can apply those.</div><div><br></div><div>Patches 3 and 4 handle edge cases worth having tests, for, though.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 7, 2015 at 6:37 PM, John Kåre Alsaker <span dir="ltr"><<a href="mailto:john.mailinglists@gmail.com" target="_blank">john.mailinglists@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I've rebased the patches.<br>
<br>
On Thu, Nov 6, 2014 at 5:02 AM, John Kåre Alsaker<br>
<div class="HOEnZb"><div class="h5"><<a href="mailto:john.mailinglists@gmail.com">john.mailinglists@gmail.com</a>> wrote:<br>
> Poke.<br>
><br>
> On Tue, Oct 14, 2014 at 10:41 PM, John Kåre Alsaker<br>
> <<a href="mailto:john.mailinglists@gmail.com">john.mailinglists@gmail.com</a>> wrote:<br>
>> I've CC'd Nadav Rotem. Hopefully he can find someone to review the x86 changes.<br>
>><br>
>> I'll leave out the optimizations for now because of the issue raised.<br>
>> I've attached the patches I'd like to have merged.<br>
>><br>
>> - John Kåre<br>
>><br>
>> On Thu, Sep 18, 2014 at 1:53 AM, Philip Reames<br>
>> <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>> wrote:<br>
>>> John & Anton,<br>
>>><br>
>>> I would be perfectly happy to have another reviewer take over here if you'd<br>
>>> prefer and can find someone willing to do so.  So far, no one else has<br>
>>> spoken up.  I'm not prepared to commit the code as of the last revision I<br>
>>> saw.  I'm happy to work with you towards that goal, but recognize the<br>
>>> (reasonable) frustration of working with a reviewer who is not familiar with<br>
>>> the code being changed and is thus forced to be conservative about<br>
>>> understanding each piece.<br>
>>><br>
>>> My apologies if the process has been unnecessarily frustrating. It's<br>
>>> entirely possible I could have handled this better.  If you have specific<br>
>>> suggestions, feel free to send me a private email off list.  I'm always<br>
>>> happy to hear constructive criticism and learn from past experiences.<br>
>>><br>
>>> p.s. There was a potential issue brought up with the optimization change.<br>
>>> I'm just calling your attention to the matter in case you missed it.<br>
>>> <a href="http://reviews.llvm.org/D5018#inline-43027" target="_blank">http://reviews.llvm.org/D5018#inline-43027</a><br>
>>><br>
>>> Philip<br>
>>><br>
>>><br>
>>><br>
>>><br>
>>> On 09/16/2014 02:30 AM, Anton Korobeynikov wrote:<br>
>>>><br>
>>>> I would suggest that someone from code owners review these changes.<br>
>>>><br>
>>>> On Tue, Sep 16, 2014 at 4:44 AM, John Kåre Alsaker<br>
>>>> <<a href="mailto:john.mailinglists@gmail.com">john.mailinglists@gmail.com</a>> wrote:<br>
>>>>><br>
>>>>> I would prefer to see the important feature land first (that is the first<br>
>>>>> 4<br>
>>>>> commits here<br>
>>>>> <a href="https://github.com/Zoxc/llvm/compare/llvm-mirror:master...stprobe" target="_blank">https://github.com/Zoxc/llvm/compare/llvm-mirror:master...stprobe</a>) since<br>
>>>>> I<br>
>>>>> actually need those and not the optimization.<br>
>>>>><br>
>>>>> On Fri, Aug 22, 2014 at 7:20 PM, Philip Reames<br>
>>>>> <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>><br>
>>>>> wrote:<br>
>>>>>><br>
>>>>>><br>
>>>>>> On 08/22/2014 12:43 AM, John Kåre Alsaker wrote:<br>
>>>>>><br>
>>>>>> On Fri, Aug 22, 2014 at 2:01 AM, Philip Reames<br>
>>>>>> <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>><br>
>>>>>> wrote:<br>
>>>>>>><br>
>>>>>>><br>
>>>>>>> On 08/18/2014 12:40 PM, John Kåre Alsaker wrote:<br>
>>>>>>><br>
>>>>>>> On Mon, Aug 18, 2014 at 8:31 PM, Philip Reames<br>
>>>>>>> <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>> wrote:<br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>>> On 08/15/2014 01:55 PM, John Kåre Alsaker wrote:<br>
>>>>>>>><br>
>>>>>>>> On Fri, Aug 15, 2014 at 6:47 PM, Philip Reames<br>
>>>>>>>> <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>> wrote:<br>
>>>>>>>>><br>
>>>>>>>>> Sorry for the delay.<br>
>>>>>>>>><br>
>>>>>>>>> Patch 0002 LGTM.  Please submit<br>
>>>>>>>>> Patch 0003 LGTM.  Please submit.<br>
>>>>>>>><br>
>>>>>>>> Yes, please submit.<br>
>>>>>>>><br>
>>>>>>>> Do you have commit access?  Or do you want me to submit on your<br>
>>>>>>>> behalf?<br>
>>>>>>><br>
>>>>>>> I'd like you to submit them.<br>
>>>>>>><br>
>>>>>>> I've landed the first couple of minor changes from the series.  Mostly<br>
>>>>>>> as<br>
>>>>>>> a show of good faith since the later items still need work.  If I<br>
>>>>>>> missed<br>
>>>>>>> something you think should have landed (i.e. got a LGTM), let me know.<br>
>>>>>>><br>
>>>>>>> I split the 5 page optimization into it's own review and added a couple<br>
>>>>>>> of folks with Windows experience.  I was all the way up to submitting<br>
>>>>>>> it and<br>
>>>>>>> realized I didn't feel comfortable doing so.  The thread is "Fast-path<br>
>>>>>>> for<br>
>>>>>>> stack probes on smaller frames" at <a href="http://reviews.llvm.org/D5018" target="_blank">http://reviews.llvm.org/D5018</a>.<br>
>>>>>>><br>
>>>>>>><br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>>>><br>
>>>>>>>>> Patch 0001 LGTM, but doesn't do anything by itself.  Please hold it<br>
>>>>>>>>> until either 0004 or 0006 are ready to land.<br>
>>>>>>>>><br>
>>>>>>>>> Patch 0004<br>
>>>>>>>>> - It seems like you're strill mixing several functional changes in<br>
>>>>>>>>> 0004.  Can you enumerate (in email) what this patch is trying to<br>
>>>>>>>>> accomplish?<br>
>>>>>>>>> The trivial implementation I expected is to add MF.shouldProbeStack<br>
>>>>>>>>> in a<br>
>>>>>>>>> couple places and call it done.  Why is the rest needed?<br>
>>>>>>>><br>
>>>>>>>> It's just generalizing the code for Win64 stack probes to x86-32 and<br>
>>>>>>>> x86-64. The __probestack calling convention on both x86-32 and x86-64<br>
>>>>>>>> is the<br>
>>>>>>>> same as __chkstk on Win64.<br>
>>>>>>>><br>
>>>>>>>> Why?  Is there a good reason now to use the x86-32 chkstk convention<br>
>>>>>>>> on<br>
>>>>>>>> x86-32 probestack?  What's the motivation for being different?<br>
>>>>>>><br>
>>>>>>> I'm being consistent across x86 while MSVC is not.<br>
>>>>>>><br>
>>>>>>> Breaking compatibility with the platform compiler is almost never<br>
>>>>>>> acceptable and requires a substantial justification.  If you want to<br>
>>>>>>> make<br>
>>>>>>> the argument that that is the right direction to take, you'll need to<br>
>>>>>>> find<br>
>>>>>>> someone else to review your work.  I am nowhere *near* knowledgeable<br>
>>>>>>> enough<br>
>>>>>>> in this area to approve such a change.<br>
>>>>>><br>
>>>>>> It does not break anything. __probestack is a new non-Windows function.<br>
>>>>>> On<br>
>>>>>> Windows the existing methods are called as before (and "probe-stack" is<br>
>>>>>> a<br>
>>>>>> no-op).<br>
>>>>>><br>
>>>>>> Thank you for the clarification.  I had misunderstood your point.<br>
>>>>>><br>
>>>>>> I'll take another look at submitting this once the 5-page opt goes in.<br>
>>>>>><br>
>>>>>><br>
>>>>>>><br>
>>>>>>> What is your actual use case?  Are you trying to do stack probing on<br>
>>>>>>> non-windows platforms?  Are you using a custom runtime on Windows?  I'm<br>
>>>>>>> asking for background so I can make constructive suggestions.<br>
>>>>>>><br>
>>>>>>><br>
>>>>>>>> In addition it doesn't assume RAX never is live in.<br>
>>>>>>>><br>
>>>>>>>> Why?  Is this a bug fix?<br>
>>>>>>><br>
>>>>>>> That depends on if it ever can be live in. There were already logic for<br>
>>>>>>> EAX which I generalized.<br>
>>>>>>><br>
>>>>>>> Is this change needed for anything else?  I would prefer not to change<br>
>>>>>>> code without reason.  Doing so only increases risk.<br>
>>>>>><br>
>>>>>> I don't know if the existing assumption that RAX is never live-in holds<br>
>>>>>> now that this code can be used on non-Windows platforms.<br>
>>>>>><br>
>>>>>> OK, that makes sense.<br>
>>>>>><br>
>>>>>><br>
>>>>>>>> It also disables the redzone (which never is used on Windows) since<br>
>>>>>>>> the<br>
>>>>>>>> probing code doesn't account for that.<br>
>>>>>>>><br>
>>>>>>>> Ok.  I see the value in this.<br>
>>>>>>>><br>
>>>>>>>> This sounds like at least three distinct changes. Please separate them<br>
>>>>>>>> and we'll review each in turn.<br>
>>>>>>>><br>
>>>>>>>> Sorry for being so pedantic about separation here.  I'm trying to<br>
>>>>>>>> review<br>
>>>>>>>> code I don't really know well.  As a result, I'm trying to be<br>
>>>>>>>> extremely<br>
>>>>>>>> carefully about making sure I understand each piece.  If you can find<br>
>>>>>>>> another reviewer who knows this code better, I'd be happy to step<br>
>>>>>>>> aside.<br>
>>>>>>><br>
>>>>>>><br>
>>>>>><br>
>>>>><br>
>>>>> _______________________________________________<br>
>>>>> llvm-commits mailing list<br>
>>>>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>>>><br>
>>>><br>
>>>><br>
>>><br>
</div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>