[PATCH] A bit-tracking DCE Pass

[hfinkel at anl.gov]

================
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()))
----------------
reames wrote:
> You could you inst_range and a range for loop here
Good point, will do.

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:104
@@ +103,3 @@
+      AliveBits[I.getInstructionIterator()] = APInt(IT->getBitWidth(), 0);
+      Worklist.push_back(I.getInstructionIterator());
+    } else {
----------------
reames wrote:
> I don't get this line.  Isn't the worklist of instructions, not of iterators?
This is left over from ADCE, which I've now fixed there. The iterators are convertable to Instruction*s. It is, however, unnecessary (doubly so after converting to a range-based for loop).

================
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);
----------------
reames wrote:
> Shouldn't you be adding I to Visited here?
No need, we need to check isAlwaysLive on the instructions again anyway to determine whether or not to delete them. I've added a comment.

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

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

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

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:367
@@ +366,3 @@
+
+  for (SmallVectorImpl<Instruction *>::iterator I = Worklist.begin(),
+       E = Worklist.end(); I != E; ++I) {
----------------
reames wrote:
> 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?  
We could do that (although, maybe we should also remove the all-dead-bits instructions).

Regarding undef, I considered that, but I'm somewhat afraid of that given our discussions recently regarding poison and undef.

http://reviews.llvm.org/D7531

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