[PATCH] A bit-tracking DCE Pass

[Philip Reames]

A few comments inline.  As with Sanjoy, happy to see this evolve in tree.


================
Comment at: lib/Transforms/Scalar/BDCE.cpp:94
@@ +93,3 @@
+  // Collect the set of "root" instructions that are known live.
+  for (inst_iterator I = inst_begin(F), E = inst_end(F); I != E; ++I) {
+    if (!isAlwaysLive(I.getInstructionIterator()))
----------------
You could you inst_range and a range for loop here

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:104
@@ +103,3 @@
+      AliveBits[I.getInstructionIterator()] = APInt(IT->getBitWidth(), 0);
+      Worklist.push_back(I.getInstructionIterator());
+    } else {
----------------
I don't get this line.  Isn't the worklist of instructions, not of iterators?

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:110
@@ +109,3 @@
+          if (IntegerType *IT = dyn_cast<IntegerType>(J->getType()))
+            AliveBits[J] = APInt::getAllOnesValue(IT->getBitWidth());
+          Worklist.push_back(J);
----------------
Shouldn't you be adding I to Visited here?

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:142
@@ +141,3 @@
+
+    // Compute the set of alive bits for each operand. These are anded into the
+    // existing set, if any, and if that changes the set of alive bits, the
----------------
This needs broken up into helper functions.  It's hard to follow as is.

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:158
@@ +157,3 @@
+            // output are alive, all others are dead.
+            switch (UserI->getOpcode()) {
+            default: break;
----------------
Having this switch pulled out into it's own function would help readability a lot.

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:325
@@ +324,3 @@
+
+    if (!UserI->getType()->isIntegerTy())
+      Visited.insert(UserI);
----------------
Having this up near the top of the loop with an early might be more readable.  

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:347
@@ +346,3 @@
+
+  for (SmallVectorImpl<Instruction *>::iterator I = Worklist.begin(),
+       E = Worklist.end(); I != E; ++I) {
----------------
range-for?

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:367
@@ +366,3 @@
+
+  for (SmallVectorImpl<Instruction *>::iterator I = Worklist.begin(),
+       E = Worklist.end(); I != E; ++I) {
----------------
Given we're not invalidating iterators here, can't these two loops be merged?

Also, why not do this step first and then delete the dead instruction in the previous loop?

Also, if the bits are dead, why not use undef?

http://reviews.llvm.org/D7531

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/