[PATCH] D14308: [SimplifyCFG] Extend SimplifyResume to handle phi of trivial landing pad.

[Philip Reames via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

All of my comments this time around are minor.  I think we can probably get this in after one last round of cleanup.

p.s. Sorry for the prolonged period without response.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3218
@@ -3215,3 +3217,3 @@
 bool SimplifyCFGOpt::SimplifyResume(ResumeInst *RI, IRBuilder<> &Builder) {
   // If this is a trivial landing pad that just continues unwinding the caught
   // exception then zap the landing pad, turning its invokes into calls.
----------------
Stale comment.  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3223
@@ +3222,3 @@
+  else
+    return SimplifyUniqueResume(RI);
+}
----------------
Naming wise, unique isn't quite right.  You could have a phi early in the value chain with a modification of it being resumed.  I think it would make a lot more sense to lift the check for landing pad here to form an else if...

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3288
@@ +3287,3 @@
+    // of erasing TrivialBB, we only remove the branch to the common resume
+    // block so that we can later erase the resume block if it has no
+    // predecessors.
----------------
minor wording: "if it has no", to "since it has no"

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3294
@@ +3293,3 @@
+
+  // Delete the resume block if all its predecessors have been removed.
+  if (pred_empty(BB))
----------------
Hm, what about an empty phi in an unreachable block?  I think you're probably guarded from getting here, but adding an assert to make that clear would be good.  


http://reviews.llvm.org/D14308