[PATCH] Replace llvm.frameallocate with llvm.frameescape

[Reid Kleckner]

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:306
@@ -320,3 +305,3 @@
   // Map the alloca instructions to the corresponding index in the
   // frame allocation structure.  If any alloca is used only in a single
   // handler and is not used in the parent frame after outlining, it will
----------------
andrew.w.kaylor wrote:
> Since we don't need to build a structure type, this loop can be combined with the loop that replaces the temporary allocas.
> 
> Also, this comment is now wrong.
Tried to clean this up a bit

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:340
@@ -358,9 +339,3 @@
 
-  // Having filled the StructTys vector and assigned an index to each element,
-  // we can now create the structure.
-  StructType *EHDataStructTy = StructType::create(
-      F.getContext(), StructTys, "struct." + F.getName().str() + ".ehdata");
-  IRBuilder<> Builder(F.getParent()->getContext());
-
   // Create a frame allocation.
   Module *M = F.getParent();
----------------
andrew.w.kaylor wrote:
> This comment is wrong.
Fixed

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:355
@@ -395,3 +354,3 @@
   // Finally, replace all of the temporary allocas for frame variables used in
   // the outlined handlers and the original frame allocas with GEP instructions
   // that get the equivalent pointer from the frame allocation struct.
----------------
andrew.w.kaylor wrote:
> This comment is wrong.
Fixed


================
Comment at: lib/CodeGen/WinEHPrepare.cpp:412
@@ +411,3 @@
+            Builder.CreateCall(RecoverFrameFn, RecoverArgs);
+        Value *RecoveredAlloca =
+            Builder.CreateBitCast(RecoveredAllocaI8, TempAlloca->getType());
----------------
andrew.w.kaylor wrote:
> This bitcast won't be necessary if TempAlloca's type is already i8*.  Given how later we are in the process, is this something that should be checked here.  Similarly, if the TempAlloca value is cast back to an i8* for some reason we should avoid that.
True.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:414
@@ +413,3 @@
+            Builder.CreateBitCast(RecoveredAllocaI8, TempAlloca->getType());
+        // ElementPtr = Builder.CreateConstInBoundsGEP2_32(EHData, 0, Idx);
+        TempAlloca->replaceAllUsesWith(RecoveredAlloca);
----------------
andrew.w.kaylor wrote:
> This shouldn't be here.
oops


================
Comment at: lib/IR/Verifier.cpp:2883
@@ -2878,1 +2882,3 @@
+    Assert1(isa<ConstantInt>(IdxArg),
+            "idx argument of llvm.framerecover must be a constant int", &CI);
     break;
----------------
andrew.w.kaylor wrote:
> Is there any way we can find the parent's frameescape call and verify that the index is in range?
This is kind of awkward because the verifier is a FunctionPass, but we can do it after we've processed the whole module. It took some doing, but it should be in the next patch.

================
Comment at: lib/MC/MCContext.cpp:136
@@ -136,1 +135,3 @@
+  return GetOrCreateSymbol(Twine(MAI->getPrivateGlobalPrefix()) + FuncName +
+                           "$frame_offset_" + Twine(Idx));
 }
----------------
andrew.w.kaylor wrote:
> I find the use of "frame_offset" potentially confusing in this name.  I prefer something like "frame_escape" that gives some clue as to what the index value means.
Sounds reasonable.

================
Comment at: test/CodeGen/WinEH/cppeh-frame-vars.ll:179
@@ -181,8 +178,3 @@
 ; CHECK: entry:
-; CHECK:   %eh.alloc = call i8* @llvm.framerecover(i8* bitcast (void ()* @"\01?test@@YAXXZ" to i8*), i8* %1)
-; CHECK:   %eh.data = bitcast i8* %eh.alloc to %"struct.\01?test@@YAXXZ.ehdata"*
-; CHECK:   %e = getelementptr inbounds %"struct.\01?test@@YAXXZ.ehdata", %"struct.\01?test@@YAXXZ.ehdata"* %eh.data, i32 0, i32 2
-; CHECK:   %NumExceptions = getelementptr inbounds %"struct.\01?test@@YAXXZ.ehdata", %"struct.\01?test@@YAXXZ.ehdata"* %eh.data, i32 0, i32 3
-; CHECK:   %ExceptionVal = getelementptr inbounds %"struct.\01?test@@YAXXZ.ehdata", %"struct.\01?test@@YAXXZ.ehdata"* %eh.data, i32 0, i32 4
-; CHECK:   %i = getelementptr inbounds %"struct.\01?test@@YAXXZ.ehdata", %"struct.\01?test@@YAXXZ.ehdata"* %eh.data, i32 0, i32 5
-; CHECK:   %Data = getelementptr inbounds %"struct.\01?test@@YAXXZ.ehdata", %"struct.\01?test@@YAXXZ.ehdata"* %eh.data, i32 0, i32 6
+; CHECK:   %e.i81 = call i8* @llvm.framerecover(i8* bitcast (void ()* @"\01?test@@YAXXZ" to i8*), i8* %1, i32 0)
+; CHECK:   %e = bitcast i8* %e.i81 to i32*
----------------
andrew.w.kaylor wrote:
> Should %e.i81 here be %e.18?  If not, why not?
There's an e.i8 value in the original program, and llvm variable renaming causes us to get "e.i8" + "1". Probably the easiest thing to do is to remove the original e.i8 value, or to cleanup dead instructions.

================
Comment at: test/CodeGen/WinEH/cppeh-inalloca.ll:141
@@ -142,1 +140,3 @@
+; CHECK:   %cleanup.dest.slot = bitcast i8* %cleanup.dest.slot.i8 to i32*
+; CHECK:   %e.i8 = bitcast i32* %e to i8*
 ; CHECK:   %a = getelementptr inbounds <{ %struct.A }>, <{ %struct.A }>* %.reload, i32 0, i32 0
----------------
andrew.w.kaylor wrote:
> This bitcast is redundant, since %e.i81 already has this pointer with an i8* type.  It also doesn't seem to be used.  Is this an artifact of %e.i8 having been passed to llvm.eh.begincatch in the original code?
Yeah. We could try erasing such dead instructions after outlining a begincatch call.

================
Comment at: test/CodeGen/WinEH/cppeh-min-unwind.ll:27
@@ +26,3 @@
+; CHECK:   %obj = alloca %class.SomeClass, align 4
+; CHECK:   %exn.slot = alloca i8*
+; CHECK:   %ehselector.slot = alloca i32
----------------
andrew.w.kaylor wrote:
> These values should be eliminated after the outlined handlers are pruned, so it's probably better not to have them checked now.
True.

http://reviews.llvm.org/D8051

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