<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 08/22/2014 12:43 AM, John Kåre
Alsaker wrote:<br>
</div>
<blockquote
cite="mid:CADjnDZdb1V-r8JCaPjXC82WbKXH0XfhTZCiZYCGremgTe0WeHg@mail.gmail.com"
type="cite">
<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 moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true"
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>
</div>
</blockquote>
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>
<blockquote
cite="mid:CADjnDZdb1V-r8JCaPjXC82WbKXH0XfhTZCiZYCGremgTe0WeHg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</div>
</blockquote>
OK, that makes sense. <br>
<blockquote
cite="mid:CADjnDZdb1V-r8JCaPjXC82WbKXH0XfhTZCiZYCGremgTe0WeHg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</blockquote>
<br>
</body>
</html>