[PATCH] D13718: [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.

The general transform looks reasonable, but I'd suggest possibly tackling this a slightly different way.  Rather than explicitly doing the detection of a trivial block and rewrite in a single step, replace the branch to the unified resume with a resume and then let the existing code handle it.  Doing it this way could make the change less invasive and easier to reason about.  Note that this is a suggestion to think about, not a requirement.  If you feel the current approach is easier/better, we can run with this after cleanup.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2926
@@ +2925,3 @@
+  } else {
+    // Check all incoming blocks to see if they are trivial.
+    auto *PhiLPInst = cast<PHINode>(RI->getValue());
----------------
This comment is slightly confusing.  I thought at first *all* incoming blocks had to be trivial.  Wording change to indicate checking for *any* trivial block would be good.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2932
@@ +2931,3 @@
+
+      // Not the landing pad that caused the control to branch here.
+      if (IncomingLP != IncomingBB->getFirstNonPHI())
----------------
You're not actually checking the input is a landing pad with this check.  It could be another PHI.  Use a dyn_cast, continue pattern. 

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2937
@@ +2936,3 @@
+      // Assume the block is trivial. Invalidate if we prove it's not later.
+      TrivialUnwindBlocks.push_back(IncomingBB);
+
----------------
Why?  Just push the add below the loop.  Either use a lambda or a IsOkay condition flag to dedect which case you're in.  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2951
@@ +2950,3 @@
+    // from the phi node.
+    for (auto IncomingBB : TrivialUnwindBlocks) {
+      PhiLPInst->removeIncomingValue(IncomingBB);
----------------
Please move this into the loop below.  


http://reviews.llvm.org/D13718