[PATCH] D23225: [ADCE] Modify data structures to support removing control flow

[Mehdi AMINI via llvm-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.

Patch looks good overall, but it seems to me that you missed D.Majnemer's comments.
I added a few nits.
LGTM with these fixed.


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:39
@@ -38,1 +38,3 @@
 
+// This is a tempoary option until we change the interface
+// to this pass based on optimization level.
----------------
"tempoary"

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:90
@@ -49,1 +89,3 @@
 
+  /// Set of blocks with not known to have live terminators.
+  SmallPtrSet<BasicBlock *, 16> BlocksWithDeadTerminators;
----------------
"blocks with not know to" does not seem correct (but English is not my native language)

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:98
@@ -50,1 +97,3 @@
+  /// Set up auxiliary data structures for Instructions and BasicBlocks and
+  /// initialize the Worklist to the set of must-be-live Instruscions.
   void initialize();
----------------
"Instruscions"

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:136
@@ +135,3 @@
+static bool isUnconditionalBranch(TerminatorInst *Term) {
+  auto BR = dyn_cast<BranchInst>(Term);
+  return BR && BR->isUnconditional();
----------------
ping.

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:145
@@ +144,3 @@
+  // We will have an entry in the map for each block so we grow the
+  // structure to twice that size to keep the load factor low in the hash table.
+  BlockInfo.reserve(NumBlocks);
----------------
Stalled comment (probably don't need a comment here, reserve is fairly explicit)

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:324
@@ +323,3 @@
+    DeadTerminators.push_back(BB->getTerminator());
+  for (auto I : DeadTerminators)
+    markLive(I);
----------------
Ping


https://reviews.llvm.org/D23225