[PATCH] A bit-tracking DCE Pass

[hfinkel at anl.gov]

================
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())) {
----------------
sanjoy wrote:
> 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.
I had been using the setting of all bits to be live as the way to communicate that they'd not be removed.

You're right, however, that even if the call can't be removed, having an unnecessary dependency on the output is also not optimal. I'll change this.

================
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)
----------------
sanjoy wrote:
> [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.
I'll refactor this, I think.

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:149
@@ +148,3 @@
+          if (UserI->getType()->isIntegerTy() && !AOut)
+            AB = APInt(BitWidth, 0);
+          else
----------------
sanjoy wrote:
> I don't think this is correct for side-effecting instructions.
Good point. Currently, all such instructions have all of their alive bits set, but if I change that, I'll need to change this too.

================
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()))
----------------
sanjoy wrote:
> Shouldn't this be `AOut & ...`?
Heh, yes.

================
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))
----------------
sanjoy wrote:
> Again, I think this should be `AOut & ...`
Yes.

================
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()) &&
----------------
sanjoy wrote:
> 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.
> Since you'll never visit %z as AliveBits[%z] will be 0?

No, because we set all of its alive bits in the first loop (as with all of the side-effecting instructions).

http://reviews.llvm.org/D7531

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