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

[Eric Christopher]

Few comments, plus some bits inline:

a) Would be nice if the verifier would look and see if the function passed to recover frame allocation actually has a frame allocation intrinisic listed. Otherwise you'll get segfault/assert during assembly time.

b) The documentation appears a bit misleading, what seems to be happening in the code is that you're recording where you put a particular allocation which is different than allocating at a specific fixed address. The allocations are getting merged.

c) some reason to have the symbol in the same function rather than a load of a symbol off to the side? e.g.
.data
.Lallocation_foo
.long -144

Otherwise it seems to be a reasonable way to pass back and forth some specific data for outlining etc via symbol values.

-eric


================
Comment at: docs/LangRef.rst:7301
@@ +7300,3 @@
+'``llvm.frameallocate``' is allocated prior to stack realignment to produce a
+fixed offset from the frame pointer, so the memory is only aligned to the
+ABI-required stack alignment.  Each function may only call
----------------
This doesn't appear to be true? The allocas seem to be merged and after alignment.

================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:75
@@ -74,1 +74,3 @@
 
+    RECOVER_FRAME_ALLOC,
+
----------------
Needs docs.

================
Comment at: include/llvm/Target/Target.td:868
@@ +867,3 @@
+  let InOperandList = (ins ptr_rc:$symbol, i32imm:$id);
+  let hasSideEffects = 0;
+  let hasCtrlDep = 1;
----------------
Might want to comment why both of these are true.

================
Comment at: include/llvm/Target/TargetOpcodes.h:121
@@ -120,1 +120,3 @@
+
+  FRAME_ALLOC = 21,
 };
----------------
Comments.

================
Comment at: lib/CodeGen/MachineFunction.cpp:591
@@ +590,3 @@
+int MachineFrameInfo::CreateFrameAllocation(uint64_t Size) {
+  // Force the use of a frame pointer. The intention is that this intrinsic be
+  // used in conjunction with unwind mechanisms that leak the frame pointer.
----------------
If this is for a specific use you probably want to reference the intrinsic more directly in the name. Right now CreateFrameAllocation(size) sounds like something I'd call in general :)

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5582
@@ +5581,3 @@
+    // Do the allocation and map it as a normal value.
+    // FIXME: Maybe we should add this to the alloca map so that we don't have
+    // to register allocate it?
----------------
Elaborate?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5605
@@ +5604,3 @@
+    MachineFunction &MF = DAG.getMachineFunction();
+    MachineRegisterInfo &MRI = MF.getRegInfo();
+    const TargetInstrInfo *TII = DAG.getSubtarget().getInstrInfo();
----------------
Unused.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5606
@@ +5605,3 @@
+    MachineRegisterInfo &MRI = MF.getRegInfo();
+    const TargetInstrInfo *TII = DAG.getSubtarget().getInstrInfo();
+    MVT PtrVT = TLI.getPointerTy(0);
----------------
Unused.

================
Comment at: test/CodeGen/X86/frameallocate.ll:25
@@ +24,3 @@
+
+define void @alloc_func(i32* %s, i32* %d) {
+  %alloc = call i8* @llvm.frameallocate(i32 16)
----------------
How about a couple tests that have both allocations and/or alignment requirements?

http://reviews.llvm.org/D6493

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/