[PATCH] D15139: [IR] Reformulate LLVM's EH funclet IR

[Andy Kaylor via llvm-commits]

andrew.w.kaylor added inline comments.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3276
@@ +3275,3 @@
+      DestParentFunclet = CSI->getOuterScope();
+    else if (auto *FPI = dyn_cast<FuncletPadInst>(DestEHPad))
+      DestParentFunclet = FPI->getOuterScope();
----------------
Can a cleanupret ever unwind to a catchpad?  If not, this should be CleanupPadInst.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3284
@@ +3283,3 @@
+    // DestParentFunclet is an ancestor of CPInst.
+    if (CPInst->getOuterScope() != DestParentFunclet)
+      return false;
----------------
Can you explain to me the scenario where we can't remove the empty cleanup pad?

I would have expected that for any legal IR there would be something we could do to remove the cleanup pad.

================
Comment at: test/CodeGen/WinEH/wineh-cloning.ll:276
@@ +275,3 @@
+; CHECK:     inner:
+; CHECK:       [[I_R:\%.+]] = cleanuppad within none []
+; CHECK:       call void @h(i32 %x1.wineh.reload)
----------------
Here, on the other hand, I don't think we need to generalize, since this block won't be cloned.

================
Comment at: test/CodeGen/WinEH/wineh-cloning.ll:281
@@ -306,3 +280,3 @@
 
-define void @test7() personality i32 (...)* @__CxxFrameHandler3 {
+define void @test7() personality i32 (...)* @__C_specific_handler {
 entry:
----------------
Is there any reason to keep this test case?  There doesn't seem to be anything interesting happening here.

================
Comment at: test/CodeGen/WinEH/wineh-cloning.ll:326
@@ -363,3 +325,3 @@
 
-define void @test8() personality i32 (...)* @__CxxFrameHandler3 {
+define void @test8() personality i32 (...)* @__C_specific_handler {
 entry:
----------------
This test case also seems to be redundant now.

================
Comment at: test/CodeGen/WinEH/wineh-cloning.ll:383
@@ -434,3 +382,3 @@
 
-define void @test9() personality i32 (...)* @__CxxFrameHandler3 {
+define void @test9() personality i32 (...)* @__C_specific_handler {
 entry:
----------------
It bothers me that this is still apparently legal.  I would think that an exception thrown from within a top level EH pad should only be able to unwind to caller.  In any event, the test is now verifying that we did not resolve the problem that the old implementation of this test was looking for.

What would the state numbering code do with this if we left it in the state that is checked here?

================
Comment at: test/CodeGen/WinEH/wineh-cloning.ll:479
@@ -552,3 +478,3 @@
 }
 ; merge.end will get cloned for outer and inner, but is implausible
 ; from inner, so the invoke @f() in inner's copy of merge should be
----------------
This comment is incorrect now, since there is no invoke in %merge anymore.  I think this test case is also obsolete.


http://reviews.llvm.org/D15139