[InstCombine] Add a DEBUG_COUNTER to InstCombine to limit how many instructions are visited for debug

[craig.topper]

Summary

Sometimes it would be nice to stop InstCombine mid way through its combining to see the current IR. By using a debug counter we can place an upper limit on how many instructions to process.

The debug counter infrastructure also supports skipping some number of calls at the beginning as well, but that feels like it would generate very odd behavior with InstCombine.

I also wonder if we should change the DEBUG_COUNTER macro to have the semicolon outside like we do for STATISTIC. Since they are often going to appear at the top of a file near STATISTIC the inconsistency seems likely to cause people to add a semicolon after DEBUG_COUNTER anyway.

[davide]

Looks good. It would be really nice if eventually we could just share code between debug-counters and opt-bisect (for example, teaching opt-bisect about counters) but that won't happen today and this patch is good regardless.

[davide]

I also wonder if we should change the DEBUG_COUNTER macro to have the semicolon outside like we do for STATISTIC. Since they are often going to appear at the top of a file near STATISTIC the inconsistency seems likely to cause people to add a semicolon after DEBUG_COUNTER anyway.

I think it boils down to a matter of tastes, and it's probably better for consistency.

[• dberlin]

The debug counter infrastructure also supports skipping some number of calls at the beginning as well, but that feels like it would generate very odd behavior with InstCombine.

FWIW: I think i would say that when it can be done reasonably, it can help a lot on large files. But it is definitely difficult, as we implement some optimizations, to have it work in a fashion that makes a ton of sense to think about. IE it's easy if the optimization just has candidates that it processes once.

For NewGVN, we go to the trouble of marking things such that it acts like a candidate counter (
IE if we skip value numbering A the first time, we'll always skip it) , because it can help us a lot to narrow down the space of stuff getting value numbered, and in some cases, reduce testcases to value numbering a single def-use chain.

All that said, since the goal is faster reduction and debugging, sometimes not thinking about it works too. Most optimization should *not* crash or break just because you decide not to optimize the first n instructions or whatever.
In this case, InstCombine shouldn't crash or break just because we randomly decide not to optimize the first N things on the worklist.

[davide]

In D36553#837752, @dberlin wrote:

The debug counter infrastructure also supports skipping some number of calls at the beginning as well, but that feels like it would generate very odd behavior with InstCombine.

FWIW: I think i would say that when it can be done reasonably, it can help a lot on large files. But it is definitely difficult, as we implement some optimizations, to have it work in a fashion that makes a ton of sense to think about. IE it's easy if the optimization just has candidates that it processes once.

For NewGVN, we go to the trouble of marking things such that it acts like a candidate counter (
IE if we skip value numbering A the first time, we'll always skip it) , because it can help us a lot to narrow down the space of stuff getting value numbered, and in some cases, reduce testcases to value numbering a single def-use chain.

All that said, since the goal is faster reduction and debugging, sometimes not thinking about it works too. Most optimization should *not* crash or break just because you decide not to optimize the first n instructions or whatever.
In this case, InstCombine shouldn't crash or break just because we randomly decide not to optimize the first N things on the worklist.

Yes. I'd say all these are bugs.
Whether we care or not, it's a different story (although I think we should because instcombine could change in a way that exposes them).
Also, Unfortunately is not uncommon to find a (even worse problem), which is, trying to reduce a testcase with opt-bisect triggers crashes in the backend (as we stop earlier in the pipeline).
Zhendong's fuzzer exposes a lot of them (as it generates bitcode and runs somehow arbitrary pipelines to trigger crashes).

tl;dr Craig, I agree that I think you may want to make that change and then we should deal with the fallout :)

lib/Transforms/InstCombine/InstructionCombining.cpp

 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/DebugCounter.h"
 #include "llvm/Support/KnownBits.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/InstCombine/InstCombine.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/Local.h"
 #include <algorithm>
 #include <climits>
 using namespace llvm;
 using namespace llvm::PatternMatch;
 
 #define DEBUG_TYPE "instcombine"
 
 STATISTIC(NumCombined , "Number of insts combined");
 STATISTIC(NumConstProp, "Number of constant folds");
 STATISTIC(NumDeadInst , "Number of dead inst eliminated");
 STATISTIC(NumSunkInst , "Number of instructions sunk");
 STATISTIC(NumExpand,    "Number of expansions");
 STATISTIC(NumFactor   , "Number of factorizations");
 STATISTIC(NumReassoc  , "Number of reassociations");
+DEBUG_COUNTER(VisitCounter, "instcombine-visit",
+              "Controls which instructions are visited");
 
 static cl::opt<bool>
 EnableExpensiveCombines("expensive-combines",
                         cl::desc("Enable expensive instruction combines"));
 
 static cl::opt<unsigned>
 MaxArraySize("instcombine-maxarray-size", cl::init(1024),
              cl::desc("Maximum array size considered when doing a combine"));
while (!Worklist.isEmpty()) {
     if (isInstructionTriviallyDead(I, &TLI)) {
       DEBUG(dbgs() << "IC: DCE: " << *I << '\n');
       eraseInstFromFunction(*I);
       ++NumDeadInst;
       MadeIRChange = true;
       continue;
     }
 
+    if (!DebugCounter::shouldExecute(VisitCounter))
+      continue;
+
     // Instruction isn't dead, see if we can constant propagate it.
     if (!I->use_empty() &&
         (I->getNumOperands() == 0 || isa<Constant>(I->getOperand(0)))) {
       if (Constant *C = ConstantFoldInstruction(I, DL, &TLI)) {
         DEBUG(dbgs() << "IC: ConstFold to: " << *C << " from: " << *I << '\n');
 
         // Add operands to the worklist.
         replaceInstUsesWith(*I, C);