<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Aug 22, 2014 at 2:01 AM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div class="">
<br>
<div>On 08/18/2014 12:40 PM, John Kåre
Alsaker wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">On Mon, Aug 18, 2014 at 8:31 PM,
Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div> <br>
<div>On 08/15/2014 01:55 PM, John Kåre Alsaker wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">On Fri, Aug 15, 2014 at
6:47 PM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Sorry
for the delay.<br>
<br>
Patch 0002 LGTM. Please submit<br>
Patch 0003 LGTM. Please submit.<br>
</blockquote>
<div>Yes, please submit.</div>
</div>
</div>
</div>
</blockquote>
</div>
Do you have commit access? Or do you want me to submit
on your behalf?</div>
</blockquote>
<div>I'd like you to submit them.</div>
</div>
</div>
</div>
</blockquote></div>
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. <br>
<br>
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
<a href="http://reviews.llvm.org/D5018" target="_blank">http://reviews.llvm.org/D5018</a>.<div class=""><br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div><br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
Patch 0001 LGTM, but doesn't do anything by
itself. Please hold it 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 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?<br>
</blockquote>
<div>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.</div>
</div>
</div>
</div>
</blockquote>
</div>
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?</div>
</blockquote>
<div>I'm being consistent across x86 while MSVC is not.</div>
</div>
</div>
</div>
</blockquote></div>
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.<br></div></blockquote><div>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).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
<br>
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. <br><div class="">
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div><br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> In addition it doesn't assume RAX never
is live in. </div>
</div>
</div>
</div>
</blockquote>
</div>
Why? Is this a bug fix? <br>
</div>
</blockquote>
<div>That depends on if it ever can be live in. There were
already logic for EAX which I generalized.</div>
</div>
</div>
</div>
</blockquote></div>
Is this change needed for anything else? I would prefer not to
change code without reason. Doing so only increases risk. <br></div></blockquote><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div class="">
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>It also disables the redzone (which never
is used on Windows) since the probing code
doesn't account for that. <br>
</div>
</div>
</div>
</div>
</blockquote>
</div>
Ok. I see the value in this.<br>
<br>
This sounds like at least three distinct changes. Please
separate them and we'll review each in turn. <br>
<br>
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. <br>
</div>
</blockquote>
</div>
</div>
</div>
</blockquote>
<br>
</div></div>
</blockquote></div><br></div></div>