[EarlyCSE] Add debug counter for debugging mis-optimizations. NFC.

[davide]

Can you add a test?

[gberry]

Add test

[reames]

This appears to have landed without review. What's going on here?

Stylistically, I see a few problems with this patch. It appears to mix early continues and if/else without any clear pattern.

However, I'm mostly concerned this appears to have gone in without review (at least as evidenced on phabricator.)

[reames]

ping! response needed.

[davide]

I never managed to review this one, but I saw it was committed anyway. I agree the style is inconsistent, though the patch logic is fine.

llvm/trunk/lib/Transforms/Scalar/EarlyCSE.cpp

 #include "llvm/IR/Type.h"
 #include "llvm/IR/Use.h"
 #include "llvm/IR/Value.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/AtomicOrdering.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/DebugCounter.h"
 #include "llvm/Support/RecyclingAllocator.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/Scalar.h"
 #include <cassert>
 #include <deque>
 #include <memory>
 #include <utility>
 
 using namespace llvm;
 using namespace llvm::PatternMatch;
 
 #define DEBUG_TYPE "early-cse"
 
 STATISTIC(NumSimplify, "Number of instructions simplified or DCE'd");
 STATISTIC(NumCSE,      "Number of instructions CSE'd");
 STATISTIC(NumCSECVP,   "Number of compare instructions CVP'd");
 STATISTIC(NumCSELoad,  "Number of load instructions CSE'd");
 STATISTIC(NumCSECall,  "Number of call instructions CSE'd");
 STATISTIC(NumDSE,      "Number of trivial dead stores removed");
 
+DEBUG_COUNTER(CSECounter, "early-cse",
+              "Controls which instructions are removed");
+
 //===----------------------------------------------------------------------===//
 // SimpleValue
 //===----------------------------------------------------------------------===//
 
 namespace {
 
 /// \brief Struct representing the available values in the scoped hash table.
 struct SimpleValue {
if (BI && BI->isConditional()) {
         assert(BI->getSuccessor(0) == BB || BI->getSuccessor(1) == BB);
         auto *TorF = (BI->getSuccessor(0) == BB)
                          ? ConstantInt::getTrue(BB->getContext())
                          : ConstantInt::getFalse(BB->getContext());
         AvailableValues.insert(CondInst, TorF);
         DEBUG(dbgs() << "EarlyCSE CVP: Add conditional value for '"
                      << CondInst->getName() << "' as " << *TorF << " in "
                      << BB->getName() << "\n");
+        if (!DebugCounter::shouldExecute(CSECounter)) {
+          DEBUG(dbgs() << "Skipping due to debug counter\n");
+        } else {
           // Replace all dominated uses with the known value.
           if (unsigned Count = replaceDominatedUsesWith(
                   CondInst, TorF, DT, BasicBlockEdge(Pred, BB))) {
             Changed = true;
             NumCSECVP += Count;
           }
         }
       }
     }
+  }
 
   /// LastStore - Keep track of the last non-volatile store that we saw... for
   /// as long as there in no instruction that reads memory.  If we see a store
   /// to the same location, we delete the dead store.  This zaps trivial dead
   /// stores which can occur in bitfield code among other things.
   Instruction *LastStore = nullptr;
 
   // See if any instructions in the block can be eliminated.  If so, do it.  If
   // not, add them to AvailableValues.
   for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E;) {
     Instruction *Inst = &*I++;
 
     // Dead instructions should just be removed.
     if (isInstructionTriviallyDead(Inst, &TLI)) {
       DEBUG(dbgs() << "EarlyCSE DCE: " << *Inst << '\n');
+      if (!DebugCounter::shouldExecute(CSECounter)) {
+        DEBUG(dbgs() << "Skipping due to debug counter\n");
+        continue;
+      }
       salvageDebugInfo(*Inst);
       removeMSSA(Inst);
       Inst->eraseFromParent();
       Changed = true;
       ++NumSimplify;
       continue;
     }
 
if (match(Inst, m_Intrinsic<Intrinsic::experimental_guard>())) {
       LastStore = nullptr;
       continue;
     }
 
     // If the instruction can be simplified (e.g. X+0 = X) then replace it with
     // its simpler value.
     if (Value *V = SimplifyInstruction(Inst, SQ)) {
       DEBUG(dbgs() << "EarlyCSE Simplify: " << *Inst << "  to: " << *V << '\n');
+      if (!DebugCounter::shouldExecute(CSECounter)) {
+        DEBUG(dbgs() << "Skipping due to debug counter\n");
+      } else {
         bool Killed = false;
         if (!Inst->use_empty()) {
           Inst->replaceAllUsesWith(V);
           Changed = true;
         }
         if (isInstructionTriviallyDead(Inst, &TLI)) {
           removeMSSA(Inst);
           Inst->eraseFromParent();
           Changed = true;
           Killed = true;
         }
         if (Changed)
           ++NumSimplify;
         if (Killed)
           continue;
       }
+    }
 
     // If this is a simple instruction that we can value number, process it.
     if (SimpleValue::canHandle(Inst)) {
       // See if the instruction has an available value.  If so, use it.
       if (Value *V = AvailableValues.lookup(Inst)) {
         DEBUG(dbgs() << "EarlyCSE CSE: " << *Inst << "  to: " << *V << '\n');
+        if (!DebugCounter::shouldExecute(CSECounter)) {
+          DEBUG(dbgs() << "Skipping due to debug counter\n");
+          continue;
+        }
         if (auto *I = dyn_cast<Instruction>(V))
           I->andIRFlags(Inst);
         Inst->replaceAllUsesWith(V);
         removeMSSA(Inst);
         Inst->eraseFromParent();
         Changed = true;
         ++NumCSE;
         continue;
if (MemInst.isValid() && MemInst.isLoad()) {
           InVal.IsAtomic >= MemInst.isAtomic() &&
           (isOperatingOnInvariantMemAt(Inst, InVal.Generation) ||
            isSameMemGeneration(InVal.Generation, CurrentGeneration,
                                InVal.DefInst, Inst))) {
         Value *Op = getOrCreateResult(InVal.DefInst, Inst->getType());
         if (Op != nullptr) {
           DEBUG(dbgs() << "EarlyCSE CSE LOAD: " << *Inst
                        << "  to: " << *InVal.DefInst << '\n');
+          if (!DebugCounter::shouldExecute(CSECounter)) {
+            DEBUG(dbgs() << "Skipping due to debug counter\n");
+            continue;
+          }
           if (!Inst->use_empty())
             Inst->replaceAllUsesWith(Op);
           removeMSSA(Inst);
           Inst->eraseFromParent();
           Changed = true;
           ++NumCSELoad;
           continue;
         }
if (CallValue::canHandle(Inst)) {
       // If we have an available version of this call, and if it is the right
       // generation, replace this instruction.
       std::pair<Instruction *, unsigned> InVal = AvailableCalls.lookup(Inst);
       if (InVal.first != nullptr &&
           isSameMemGeneration(InVal.second, CurrentGeneration, InVal.first,
                               Inst)) {
         DEBUG(dbgs() << "EarlyCSE CSE CALL: " << *Inst
                      << "  to: " << *InVal.first << '\n');
+        if (!DebugCounter::shouldExecute(CSECounter)) {
+          DEBUG(dbgs() << "Skipping due to debug counter\n");
+          continue;
+        }
         if (!Inst->use_empty())
           Inst->replaceAllUsesWith(InVal.first);
         removeMSSA(Inst);
         Inst->eraseFromParent();
         Changed = true;
         ++NumCSECall;
         continue;
       }
if (MemInst.isValid() && MemInst.isStore()) {
         // In that case, LastStore should keep its present value since we're
         // removing the current store.
         assert((!LastStore ||
                 ParseMemoryInst(LastStore, TTI).getPointerOperand() ==
                     MemInst.getPointerOperand() ||
                 MSSA) &&
                "can't have an intervening store if not using MemorySSA!");
         DEBUG(dbgs() << "EarlyCSE DSE (writeback): " << *Inst << '\n');
+        if (!DebugCounter::shouldExecute(CSECounter)) {
+          DEBUG(dbgs() << "Skipping due to debug counter\n");
+          continue;
+        }
         removeMSSA(Inst);
         Inst->eraseFromParent();
         Changed = true;
         ++NumDSE;
         // We can avoid incrementing the generation count since we were able
         // to eliminate this store.
         continue;
       }
if (Inst->mayWriteToMemory()) {
         if (LastStore) {
           ParseMemoryInst LastStoreMemInst(LastStore, TTI);
           assert(LastStoreMemInst.isUnordered() &&
                  !LastStoreMemInst.isVolatile() &&
                  "Violated invariant");
           if (LastStoreMemInst.isMatchingMemLoc(MemInst)) {
             DEBUG(dbgs() << "EarlyCSE DEAD STORE: " << *LastStore
                          << "  due to: " << *Inst << '\n');
+            if (!DebugCounter::shouldExecute(CSECounter)) {
+              DEBUG(dbgs() << "Skipping due to debug counter\n");
+            } else {
               removeMSSA(LastStore);
               LastStore->eraseFromParent();
               Changed = true;
               ++NumDSE;
               LastStore = nullptr;
             }
+          }
           // fallthrough - we can exploit information about this store
         }
 
         // Okay, we just invalidated anything we knew about loaded values.  Try
         // to salvage *something* by remembering that the stored value is a live
         // version of the pointer.  It is safe to forward from volatile stores
         // to non-volatile loads, so we don't have to check for volatility of
         // the store.

llvm/trunk/test/Other/debugcounter-earlycse.ll

+; REQUIRES: asserts
+; RUN: opt -S -debug-counter=early-cse-skip=1,early-cse-count=1 -early-cse  < %s 2>&1 | FileCheck %s
+;; Test that, with debug counters on, we only optimize the second CSE opportunity.
+define i32 @test(i32 %a, i32 %b) {
+; CHECK-LABEL: @test(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    %add1 = add i32 %a, %b
+; CHECK-NEXT:    %add2 = add i32 %a, %b
+; CHECK-NEXT:    %add4 = add i32 %a, %b
+; CHECK-NEXT:    %ret1 = add i32 %add1, %add2
+; CHECK-NEXT:    %ret2 = add i32 %add1, %add4
+; CHECK-NEXT:    %ret = add i32 %ret1, %ret2
+; CHECK-NEXT:    ret i32 %ret
+;
+bb:
+  %add1 = add i32 %a, %b
+  %add2 = add i32 %a, %b
+  %add3 = add i32 %a, %b
+  %add4 = add i32 %a, %b
+  %ret1 = add i32 %add1, %add2
+  %ret2 = add i32 %add3, %add4
+  %ret = add i32 %ret1, %ret2
+  ret i32 %ret
+}
+
+
+