[llvm-commits] [PATCH] [msan] Remove unreachable bb's

[Chandler Carruth]

  Sorry for the delay. Commenting more thoroughly on the new helper function.


================
Comment at: lib/Transforms/Scalar/SimplifyCFGPass.cpp:118-120
@@ -117,5 +118,2 @@
 
-    if (!Reachable.insert(BB))
-      continue;
-
     // Do a quick scan of the basic block, turning any obviously unreachable
----------------
While I think this change is a good change, please make it in a separate commit.

Feel free to commit the changes to this file directly (i've reviewed them) and you can explain the performance reasons in that commit log.

================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:730-732
@@ +729,5 @@
+
+/// RemoveUnreachableBlocks - Remove all blocks that can not be reached from the
+/// function's entry.
+void llvm::RemoveUnreachableBlocks(Function &F) {
+  SmallPtrSet<BasicBlock*, 128> Reachable;
----------------
This makes more sense in Local.h and Local.cpp than BasicBlockUtils.cpp -- this isn't for transforming a basic block, but a function.

Also, it should start with lower case letter as 'removeUnreachableBlocks', and it's comment should be in the modern doxygen style the coding standards recommend.

================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:748
@@ +747,3 @@
+  assert(Reachable.size() < F.size());
+  for (Function::iterator BB = ++F.begin(); BB != F.end();)
+    if (!Reachable.count(BB)) {
----------------
++ is confusing (and may be risky) here -- what guarantee do you have that begin() returns a temporary which can be mutated?

I would use llvm::next(F.begin()), this is also the more idiomatic way to do this.

Also, this should be written using the idiomatic LLVM loop pattern:

  for (Function::iterator I = llvm::next(F.begin()), E = F.end(); I != E;)

The end iterator is stable even across basic blocks being removed.

================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:749
@@ +748,3 @@
+  for (Function::iterator BB = ++F.begin(); BB != F.end();)
+    if (!Reachable.count(BB)) {
+      if (!BB->use_empty())
----------------
LLVM's style is to prefer early exit and early continue, so I would write:

  if (Reachable.count(BB))
    continue;

================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:750-752
@@ +749,5 @@
+    if (!Reachable.count(BB)) {
+      if (!BB->use_empty())
+        BB->replaceAllUsesWith(UndefValue::get(BB->getType()));
+      DeleteDeadBlock(BB++);
+    } else
----------------
Very frustratingly, this is not yet correct. =[

DeleteDeadBlock does some clever stuff to remove a basic block from its successors in a way that leaves them significantly cleaner than another technique might. However, it relies on the PHI nodes in the successor still having PHI nodes referencing the block to be deleted. Calling RAUW on the block to be deleted defeats this by making the PHI nodes reference undef. =/

I think I led you astray in having you re-use this routine. The only thing you really want to re-use is the code to zap the instructions within the dead basic block by first RAUW-ing them with undef, and then erasing them from the block. I'm fine either splitting that into a function that both DeleteDeadBlock and your routine call, or just re-implementing it here. It's not much code (one loop, 3 statements).

The interesting thing is that you *don't* want to do the 'removePredecessor' bit when the successor is dead -- no cleverness will matter as we're about to nuke that block too. Also, you want to replace uses with undef after clearing the reachable predecessors.

Does this make sense? Sorry for the confusion.

================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:754
@@ +753,3 @@
+    } else
+      ++BB;
+}
----------------
I think it might be simpler to increment in the loop, and if we end up deleting the block, do '--I' first...


http://llvm-reviews.chandlerc.com/D208