<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/15/2014 01:55 PM, John Kåre
Alsaker wrote:<br>
</div>
<blockquote
cite="mid:CADjnDZeEWXAwuwHHa1RGjPK3RBzZgX7av4ggSm3V3w=tG+QS-g@mail.gmail.com"
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>
Do you have commit access? Or do you want me to submit on your
behalf?<br>
<blockquote
cite="mid:CADjnDZeEWXAwuwHHa1RGjPK3RBzZgX7av4ggSm3V3w=tG+QS-g@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"><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>
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?<br>
<blockquote
cite="mid:CADjnDZeEWXAwuwHHa1RGjPK3RBzZgX7av4ggSm3V3w=tG+QS-g@mail.gmail.com"
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>
Why? Is this a bug fix? <br>
<blockquote
cite="mid:CADjnDZeEWXAwuwHHa1RGjPK3RBzZgX7av4ggSm3V3w=tG+QS-g@mail.gmail.com"
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>
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>
<blockquote
cite="mid:CADjnDZeEWXAwuwHHa1RGjPK3RBzZgX7av4ggSm3V3w=tG+QS-g@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"><br>
<br>
Patch 0005<br>
- 0x5000 - Please write this as " 5 * PageSize". I won't
ask you to make page size configurable right now, but it
should be clear to someone who might want to extend it
what this is coming from. Also, *why* 5?<br>
</blockquote>
<div>5 is what GCC uses before turning the code into a loop.</div>
</div>
</div>
</div>
</blockquote>
Document that motivation please.<br>
<blockquote
cite="mid:CADjnDZeEWXAwuwHHa1RGjPK3RBzZgX7av4ggSm3V3w=tG+QS-g@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">-
"0x1000" should be replaced with "PageSize" to make it
more understandable what's happening here.<br>
</blockquote>
<div>Ok</div>
<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">-
I don't think your addressing is quite right. If RSP
before adjustment was exactly on a page boundary, wouldn't
you miss touching the first page entirely?<br>
</blockquote>
<div>No, we'd write to the first word of the page.</div>
</div>
</div>
</div>
</blockquote>
Assume the original RSP was page aligned (after pushing the RetPC),
NumBytes is exactly one page. Your code would compute an offset
which is exactly one page off from the new RSP. This would be in
the same page as the old RSP, not the new one. In theory, you could
then push another frame and miss touching the page.<br>
<br>
0x1000 - ret PC, touch location<br>
0x2000 - new RSP<br>
<br>
I may be wrong here, but if so, why?<br>
<br>
<blockquote
cite="mid:CADjnDZeEWXAwuwHHa1RGjPK3RBzZgX7av4ggSm3V3w=tG+QS-g@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">-
To make the loop more readable, I might restructure it as:
for(int Offset = NumBytes - PageSize + 1; Offset >= 0;
Offset -= PageSize) { touch RSP+Offset }<br>
</blockquote>
<div>I like the clarity on the amount of probes done.</div>
<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">-
After reading up a bit about what chkstk does internally
on windows (i.e. internal probe stack), the optimization
seems clearly correct. It would be nice to explain that
in a comment. Don't assume everyone who reads the code
knows how chkstk manages stack expansion.<br>
</blockquote>
<div>Sure </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">-
With these changes, LGTM. I believe this is independent
of 0004 right?<br>
</blockquote>
<div>It is.</div>
<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 0006 LGTM.<br>
<br>
Patch 0007 LGTM, but you should get someone who's familiar
with the ARM backend to review.<br>
<br>
Patch 0008 I don't understand. Why is this correct?</blockquote>
<div>chkstk_ms on Cygwin/MinGW is the same as chkstk on
MSVC, so we can reuse the code for the call to the MSVC
version.</div>
<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"> </blockquote>
<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"><span
class=""><font color="#888888"><br>
<br>
Philip</font></span>
<div class="">
<div class="h5"><br>
<br>
On 08/07/2014 02:23 AM, John Kåre Alsaker 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">This
is a continuation of <a moz-do-not-send="true"
href="http://reviews.llvm.org/D4717"
target="_blank">http://reviews.llvm.org/D4717</a><br>
<br>
It seems Phabricator is useless with multiple
patches.<br>
<br>
I've fixed the points raised in D4717 and separated
out whitespaces changes to make review easier.<br>
<br>
</blockquote>
<br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</body>
</html>