[PATCH] Add a "probe-stack" attribute

[John Kåre Alsaker]

On Fri, Aug 15, 2014 at 6:47 PM, Philip Reames <listmail at philipreames.com>
wrote:

> Sorry for the delay.
>
> Patch 0002 LGTM.  Please submit
> Patch 0003 LGTM.  Please submit.
>
Yes, please submit.


>
> Patch 0001 LGTM, but doesn't do anything by itself.  Please hold it until
> either 0004 or 0006 are ready to land.
>
> Patch 0004
> - 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?
>
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.


>
> Patch 0005
> - 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?
>
5 is what GCC uses before turning the code into a loop.


> - "0x1000" should be replaced with "PageSize" to make it more
> understandable what's happening here.
>
Ok


> - 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?
>
No, we'd write to the first word of the page.


> - To make the loop more readable, I might restructure it as: for(int
> Offset = NumBytes - PageSize + 1; Offset >= 0; Offset -= PageSize) { touch
> RSP+Offset }
>
I like the clarity on the amount of probes done.


> - 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.
>
Sure

> - With these changes, LGTM.   I believe this is independent of 0004 right?
>
It is.


>
> Patch 0006 LGTM.
>
> Patch 0007 LGTM, but you should get someone who's familiar with the ARM
> backend to review.
>
> Patch 0008 I don't understand.  Why is this correct?

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.


>


>
> Philip
>
>
> On 08/07/2014 02:23 AM, John Kåre Alsaker wrote:
>
>> This is a continuation of http://reviews.llvm.org/D4717
>>
>> It seems Phabricator is useless with multiple patches.
>>
>> I've fixed the points raised in D4717 and separated out whitespaces
>> changes to make review easier.
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140815/f38f70bf/attachment.html>