[PATCH] Add the llvm.frameallocate and llvm.recoverframeallocation intrinsics

[Andy Kaylor]

The implementation looks OK to me.  It also looks like this will meet our needs for outlining functions for exception handling. My comments are pretty much documentation and error handling nits.

================
Comment at: docs/LangRef.rst:7238
@@ +7237,3 @@
+
+The '``llvm.frameallocate``' intrinsic allocates stack memory at some fixed
+offset from the frame pointer, and the '``llvm.recoverframeallocation``'
----------------
You should say something about how (or at least where) this "fixed offset" is determined.

================
Comment at: docs/LangRef.rst:7246
@@ +7245,3 @@
+
+The ``size`` argument to '``llvm.frameallocate``' must be a constant integer
+indicating the amount of stack memory to allocate.
----------------
What happens if 'size' is zero?

================
Comment at: docs/LangRef.rst:7263
@@ +7262,3 @@
+These intrinsics allow a group of functions to share the stack memory
+allocation of an ancestor stack frame. The memory returned from
+'``llvm.frameallocate``' is allocated prior to stack realignment to produce a
----------------
The phrase "the stack memory allocation" is a bit ambiguous.  You only mean for it to refer to the part of stack memory allocated by this intrinsic, right?  So if the function contains other allocas is addition to this intrinsic the descendant functions can't access that alloca (without rolling it into this frame allocation), yes?

================
Comment at: docs/LangRef.rst:7267
@@ +7266,3 @@
+ABI-required stack alignment.  Each function may only call
+'``llvm.frameallocate``' one or zero times from the function entry block.  The
+frame allocation intrinsic inhibits inlining, as any frame allocations in the
----------------
Can the call be anywhere in the entry block?  Can there be allocas before or after this call?

================
Comment at: docs/LangRef.rst:7271
@@ -7224,1 +7270,3 @@
+by '``llvm.recoverframeallocation``' called with the uninlined function.
+
 .. _int_read_register:
----------------
I think it would be helpful to describe the exception handling case here as an example of why this is useful.  With everything kept abstract like this it is hard to understand why anyone would want to do this.

================
Comment at: lib/CodeGen/MachineFunction.cpp:594
@@ +593,3 @@
+  setFrameAddressIsTaken(true);
+  Size = RoundUpToAlignment(Size, StackAlignment);
+  FrameAllocationSize = Size;
----------------
Will this round zero up to a non-zero value?  If not, then the call to setFrameAddressIsTaken may be incorrect.  Also, it seems like a good idea to verify here that FrameAllocationSize is zero before it is set by this function.

================
Comment at: lib/IR/Verifier.cpp:2584
@@ +2583,3 @@
+            "argument must be function defined in this module", &CI);
+    break;
+  }
----------------
Is there a reasonable way to verify that fp is at least a pointer?  Looking at your test case made me think that at the very least null should be flagged as an error, and it seems like any constant is likely to be an error.

================
Comment at: test/Verifier/frameallocate.ll:5
@@ +4,3 @@
+declare i8* @llvm.recoverframeallocation(i8*, i8*)
+
+define internal void @f() {
----------------
Checking for expected behavior if llvm.frameallocate is called with a zero argument would be nice.  You should also have a test for a non-constant argument.

================
Comment at: test/Verifier/frameallocate.ll:33
@@ +32,3 @@
+define internal void @i() {
+  call i8* @llvm.recoverframeallocation(i8* @global, i8* null)
+  ret void
----------------
The second argument here looks like an error also.

http://reviews.llvm.org/D6493