[PATCH] Add a "probe-stack" attribute

[Philip Reames]

Sorry for the delay.

Patch 0002 LGTM.  Please submit
Patch 0003 LGTM.  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?


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?
- "0x1000" should be replaced with "PageSize" to make it more 
understandable what's happening here.
- 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?
- To make the loop more readable, I might restructure it as: for(int 
Offset = NumBytes - PageSize + 1; Offset >= 0; Offset -= PageSize) { 
touch RSP+Offset }
- 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.
- With these changes, LGTM.   I believe this is independent of 0004 right?

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?

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