[PATCH] A bit-tracking DCE Pass

[Sanjoy Das]

Quick first pass comments inline.  I'm okay with this landing in tree with the comments addressed in some way or another, but I think I'll wait for someone else to give an explicit LGTM.


================
Comment at: lib/Transforms/Scalar/BDCE.cpp:96
@@ +95,3 @@
+      // For integer-valued instructions, mark all bits as live so that none of
+      // these (integer-valued function calls, for example) are removed.
+      if (IntegerType *IT = dyn_cast<IntegerType>(I->getType())) {
----------------
But if you're not the one removing the side-effecting instructions, they should not be removed anyway, right, even if their result is unused?  In fact, I think it will be desirable to rewrite the result of `foo()` to be `0` if possible, even if `foo()` is a side-effecting call that cannot be removed.

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:142
@@ +141,3 @@
+    // operand is added to the work-list.
+    for (Instruction::op_iterator OI = UserI->op_begin(), OE = UserI->op_end();
+         OI != OE; ++OI)
----------------
[Minor] This is just my opinion, and does not stem form the LLVM style guide; but it'd be  easier to tell where the for loop ended if you put braces around the body.

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:149
@@ +148,3 @@
+          if (UserI->getType()->isIntegerTy() && !AOut)
+            AB = APInt(BitWidth, 0);
+          else
----------------
I don't think this is correct for side-effecting instructions.

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:213
@@ +212,3 @@
+                  // bits, then we must keep the highest input bit.
+                  if ((AOut | APInt::getHighBitsSet(BitWidth,
+                                                    CI->getZExtValue()))
----------------
Shouldn't this be `AOut & ...`?

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:266
@@ +265,3 @@
+              // bits, then we must keep the highest input bit.
+              if ((AOut | APInt::getHighBitsSet(AOut.getBitWidth(),
+                                                AOut.getBitWidth() - BitWidth))
----------------
Again, I think this should be `AOut & ...`

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:303
@@ +302,3 @@
+  // NOTE: We reuse the Worklist vector here for memory efficiency.
+  for (inst_iterator I = inst_begin(F), E = inst_end(F); I != E; ++I)
+    if (!Visited.count(I.getInstructionIterator()) &&
----------------
Am I right in understanding that this will delete `%z` in the
following case?

    ;; all i8
    %z = add %x, %y
    %z0 = and 0x0f, %z
    %z1 = and 0xf0, %z0
    side_effect(%z0)

Since you'll never visit `%z` as `AliveBits[%z]` will be `0`?

In any case, a one-liner on how this is better than trivial DCE will be helpful.

http://reviews.llvm.org/D7531

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