<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><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. In addition it doesn't assume RAX never is live in. It also disables the redzone (which never is used on Windows) since the probing code doesn't account for that. </div>
<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><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><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 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>