[PATCH] D24918: [ADCE] Add code to remove dead branches

[Daniel Berlin via llvm-commits]

dberlin added inline comments.

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:217
@@ +216,3 @@
+    // and mark the branch live it if there is a back edge.
+    SmallPtrSet<BasicBlock *, 16> Seen;
+    for (auto &BB : F) {
----------------
I don't think this does what you think.
The order here is not actually guaranteed to be in any particular order, AFAIK.

It happens most things produce blocks in lexical order, but this is not guaranteed (again, AFAIK)

Thus, it's entirely possible you see a successor before a predecessor, which you will incorrectly mark a a back edge.
Please just use a standard DFS approach that is guaranteed correct.




================
Comment at: lib/Transforms/Scalar/ADCE.cpp:228
@@ +227,3 @@
+          markLive(Term);
+          break;
+        }
----------------
This  isn't correct.  What if you have two backedges going out of a single block?


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:235
@@ +234,3 @@
+  // return of the function or a successor for which this is true.
+  // This protects IDFCalculator which cannot handle such blocks.
+  for (auto &BBInfoPair : BlockInfo) {
----------------
We should just fix IDF calculator in the reverse case?

In any case, this is just a reverse DFS (IE DFS over Inverse<BasicBlock>) from all blocks with no successors (LLVM does not hvae a single return block, you can't really say "the return of the function").

Please just do that.



================
Comment at: lib/Transforms/Scalar/ADCE.cpp:248
@@ -202,2 +247,3 @@
+        markLive(BBInfo.Terminator);
         break;
       }
----------------
This has the same bug as above :)


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:610
@@ +609,3 @@
+  }
+
+  // Update each live phi based on the new incoming edges.
----------------
Is the above really necessary?

The standard way to do this, AFAIK, is to mark the useful block set while doing marking, and then just walking up the PDT to find the nearest block marked useful for each dead branch.
Replace all uses of the bb with that block using RAUW.
This should update phi nodes, since I believe the blocks in phi nodes are still considered uses.
 


https://reviews.llvm.org/D24918