[PATCH] A bit-tracking DCE Pass

[Chandler Carruth]

Lots of nit-picky comments. Generally, this looks really nice. Have you profiled it? Have you tried to synthesize extreme test cases with very large integers to make sure this doesn't become *too* expensive?

I'm generally fine just landing this and sorting out these issues after the fact. Please commit, and sorry for the delay reviewing it.


================
Comment at: lib/Transforms/Scalar/BDCE.cpp:45
@@ +44,3 @@
+namespace {
+  struct BDCE : public FunctionPass {
+    static char ID; // Pass identification, replacement for typeid
----------------
clang-format won't indent this, and that's my preference as well.

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:73
@@ +72,3 @@
+
+static bool isAlwaysLive(Instruction *I) {
+  return isa<TerminatorInst>(I) || isa<DbgInfoIntrinsic>(I) ||
----------------
Accept 'Instruction &'?

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:95
@@ +94,3 @@
+  for (inst_iterator I = inst_begin(F), E = inst_end(F); I != E; ++I) {
+    if (!isAlwaysLive(I.getInstructionIterator()))
+      continue;
----------------
'*I'?

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:45
@@ +44,3 @@
+namespace {
+  struct BDCE : public FunctionPass {
+    static char ID; // Pass identification, replacement for typeid
----------------
clang-format doesn't indent things in an anonymous namespace, and I prefer that.

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:94
@@ +93,3 @@
+    KnownOne =  APInt(BitWidth, 0);
+    computeKnownBits(const_cast<Value*>(V1), KnownZero, KnownOne, DL, 0, AC,
+                     UserI, DT);
----------------
Why accept const inputs just to cast it way?

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:97-99
@@ +96,5 @@
+
+    if (V2) {
+      KnownZero2 = APInt(BitWidth, 0);
+      KnownOne2 =  APInt(BitWidth, 0);
+      computeKnownBits(const_cast<Value*>(V2), KnownZero2, KnownOne2, DL, 0, AC,
----------------
This behavior doesn't really make sense. Maybe add a comment to the lambda to clarify what V1 and V2 are and why they would work this way?

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:258-259
@@ +257,4 @@
+  AC = &getAnalysis<AssumptionCacheTracker>().getAssumptionCache(F);
+  DataLayoutPass *DLP = getAnalysisIfAvailable<DataLayoutPass>();
+  DL = DLP ? &DLP->getDataLayout() : nullptr;
+  DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
----------------
Just dig it out of the module, simpler than using the analysis layer.

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:278-279
@@ +277,4 @@
+    if (IntegerType *IT = dyn_cast<IntegerType>(I.getType())) {
+      AliveBits[&I] = APInt(IT->getBitWidth(), 0);
+      Worklist.push_back(&I);
+    } else {
----------------
continue after this?

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:341
@@ +340,3 @@
+          if (ABNew != ABPrev || ABI == AliveBits.end()) {
+            AliveBits[I] = ABNew;
+            Worklist.push_back(I);
----------------
std::move?

Probably a few other places you might want to move here so that large APInt values don't trigger too many copies of a heap allocated set of bits.

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:361-363
@@ +360,5 @@
+    if (I.getType()->isIntegerTy()) {
+      auto ABI = AliveBits.find(&I);
+      if (ABI != AliveBits.end()) {
+        if (!ABI->second) {
+          DEBUG(dbgs() << "BDCE: Trivializing: " << I << " (all bits dead)\n");
----------------
Invert the conditions, use continue, and un-indent this?

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:365-367
@@ +364,5 @@
+          DEBUG(dbgs() << "BDCE: Trivializing: " << I << " (all bits dead)\n");
+          Value *Zero = ConstantInt::get(I.getType(), 0);
+          ++NumSimplified;
+          I.replaceAllUsesWith(Zero);
+          Changed = true;
----------------
Why not replace with undef?

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:380-382
@@ +379,5 @@
+
+    DEBUG(dbgs() << "BDCE: Removing: " << I << " (unused)\n");
+    Worklist.push_back(&I);
+    I.dropAllReferences();
+    Changed = true;
----------------
Why not just erase the instructions? You can use the two-level iterators and pre-increment the iterator to avoid invalidation...

================
Comment at: test/Transforms/BDCE/dce-pure.ll:6
@@ +5,3 @@
+define void @test1() {
+        call i32 @strlen( i8* null )                ; <i32>:1 [#uses=0]
+        ret void
----------------
The indentation (and weird comments) make my OCD-tendencies twitch.

http://reviews.llvm.org/D7531

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