<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/18/2014 12:40 PM, John Kåre
Alsaker wrote:<br>
</div>
<blockquote
cite="mid:CADjnDZcUAJrysYtmBb2Kdvidsa78_8m45OQZ+-2Am=B1UhGGGw@mail.gmail.com"
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 class=""> <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>
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 class="moz-txt-link-freetext" href="http://reviews.llvm.org/D5018">http://reviews.llvm.org/D5018</a>.<br>
<blockquote
cite="mid:CADjnDZcUAJrysYtmBb2Kdvidsa78_8m45OQZ+-2Am=B1UhGGGw@mail.gmail.com"
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 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"><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>
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>
<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>
<br>
<blockquote
cite="mid:CADjnDZcUAJrysYtmBb2Kdvidsa78_8m45OQZ+-2Am=B1UhGGGw@mail.gmail.com"
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 class=""><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>
Is this change needed for anything else? I would prefer not to
change code without reason. Doing so only increases risk. <br>
<blockquote
cite="mid:CADjnDZcUAJrysYtmBb2Kdvidsa78_8m45OQZ+-2Am=B1UhGGGw@mail.gmail.com"
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 class="">
<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>
</body>
</html>