[DebugCounter] Add support for non-continous ranges.

[Ralender]

• Script for bisecting though a build

[llvmbot]

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-llvm-globalisel
@llvm/pr-subscribers-backend-amdgpu
@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-llvm-ir

Author: None (Ralender)

Changes

• Script for bisecting though a build

---

Patch is 25.60 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/89470.diff

10 Files Affected:

• (modified) llvm/docs/ProgrammersManual.rst (+25-8)
• (modified) llvm/include/llvm/IR/PassManager.h (+5)
• (modified) llvm/include/llvm/Support/DebugCounter.h (+41-13)
• (modified) llvm/lib/IR/PassManager.cpp (+9)
• (modified) llvm/lib/Support/DebugCounter.cpp (+115-47)
• (added) llvm/tools/delta-driver/CMakeLists.txt (+9)
• (added) llvm/tools/delta-driver/delta-driver.cpp (+151)
• (modified) llvm/unittests/Support/DebugCounterTest.cpp (+14-14)
• (added) llvm/utils/bisector/bisector.py (+110)
• (added) llvm/utils/bisector/bisector_demo.sh (+35)

diff --git a/llvm/docs/ProgrammersManual.rst b/llvm/docs/ProgrammersManual.rst
index 491e6b1dd2498b..cb0796226310e5 100644
--- a/llvm/docs/ProgrammersManual.rst
+++ b/llvm/docs/ProgrammersManual.rst
@@ -1362,14 +1362,12 @@ Whatever code you want that control, use ``DebugCounter::shouldExecute`` to cont
   if (DebugCounter::shouldExecute(DeleteAnInstruction))
     I->eraseFromParent();
 
-That's all you have to do.  Now, using opt, you can control when this code triggers using
-the '``--debug-counter``' option.  There are two counters provided, ``skip`` and ``count``.
-``skip`` is the number of times to skip execution of the codepath.  ``count`` is the number
-of times, once we are done skipping, to execute the codepath.
+That's all you have to do. Now, using opt, you can control when this code triggers using
+the '``--debug-counter``' Options.To specify when to execute the codepath.
 
 .. code-block:: none
 
-  $ opt --debug-counter=passname-delete-instruction-skip=1,passname-delete-instruction-count=2 -passname
+  $ opt --debug-counter=passname-delete-instruction=1-3 -passname
 
 This will skip the above code the first time we hit it, then execute it twice, then skip the rest of the executions.
 
@@ -1384,9 +1382,28 @@ So if executed on the following code:
 
 It would delete number ``%2`` and ``%3``.
 
-A utility is provided in `utils/bisect-skip-count` to binary search
-skip and count arguments. It can be used to automatically minimize the
-skip and count for a debug-counter variable.
+A utility is provided in `llvm/tools/delta-driver/delta-driver.cpp` to drive the automated delta debugging.
+How to use delta-driver:
+First, Figure out the number of calls to the debug counter you want to minimize.
+To do so, run the compilation command causing issue with `-print-debug-counter` adding a `-mllvm` if needed.
+Than find the line with the counter of interest. it should look like:
+.. code-block:: none
+
+  my-counter               : {5678,empty}
+
+The number of calls to `my-counter` is 5678
+
+Than Find the minimal uses of the debug counter with `delta-driver`.
+Build a reproducer script like:
+.. code-block:: bash
+
+  #! /bin/bash
+  opt -debug-counter=my-counter=$1
+  # ... Test result of the command. Failure of the script is considered interesting
+
+Than run `delta-driver my-script.sh 0-5678 2>&1 | tee dump_bisect`
+This command may take some time.
+but when it is done, it will print the result like: `Minimal Chunks = 0:1:5:11-12:33-34`
 
 .. _ViewGraph:
 
diff --git a/llvm/include/llvm/IR/PassManager.h b/llvm/include/llvm/IR/PassManager.h
index d701481202f8db..5fb6a59380a792 100644
--- a/llvm/include/llvm/IR/PassManager.h
+++ b/llvm/include/llvm/IR/PassManager.h
@@ -64,6 +64,9 @@ extern llvm::cl::opt<bool> UseNewDbgInfoFormat;
 
 namespace llvm {
 
+// Used for debug counter of adding a pass to the pipeline
+bool shouldAddNewPMPass();
+
 // Forward declare the analysis manager template.
 template <typename IRUnitT, typename... ExtraArgTs> class AnalysisManager;
 
@@ -247,6 +250,8 @@ class PassManager : public PassInfoMixin<
 
   // FIXME: Revert to enable_if style when gcc >= 11.1
   template <typename PassT> LLVM_ATTRIBUTE_MINSIZE void addPass(PassT &&Pass) {
+    if (!shouldAddNewPMPass())
+      return;
     using PassModelT =
         detail::PassModel<IRUnitT, PassT, AnalysisManagerT, ExtraArgTs...>;
     if constexpr (!std::is_same_v<PassT, PassManager>) {
diff --git a/llvm/include/llvm/Support/DebugCounter.h b/llvm/include/llvm/Support/DebugCounter.h
index 9fa4620ade3c8f..adafde1e16f702 100644
--- a/llvm/include/llvm/Support/DebugCounter.h
+++ b/llvm/include/llvm/Support/DebugCounter.h
@@ -43,6 +43,7 @@
 #ifndef LLVM_SUPPORT_DEBUGCOUNTER_H
 #define LLVM_SUPPORT_DEBUGCOUNTER_H
 
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/UniqueVector.h"
@@ -53,6 +54,18 @@ namespace llvm {
 
 class raw_ostream;
 
+struct Chunk {
+  int64_t Begin;
+  int64_t End;
+  void print(llvm::raw_ostream& OS);
+  bool contains(int64_t Idx) { return Idx >= Begin && Idx <= End; }
+};
+
+void printChunks(raw_ostream &OS, ArrayRef<Chunk>);
+
+/// Return true on parsing error and print the error message on the llvm::errs()
+bool parseChunks(StringRef Str, SmallVector<Chunk>& Res);
+
 class DebugCounter {
 public:
   /// Returns a reference to the singleton instance.
@@ -77,18 +90,28 @@ class DebugCounter {
     auto Result = Us.Counters.find(CounterName);
     if (Result != Us.Counters.end()) {
       auto &CounterInfo = Result->second;
-      ++CounterInfo.Count;
+      int64_t CurrCount = CounterInfo.Count++;
+      uint64_t CurrIdx = CounterInfo.CurrChunkIdx;
 
-      // We only execute while the Skip is not smaller than Count,
-      // and the StopAfter + Skip is larger than Count.
-      // Negative counters always execute.
-      if (CounterInfo.Skip < 0)
+      if (CounterInfo.Chunks.empty())
         return true;
-      if (CounterInfo.Skip >= CounterInfo.Count)
+      if (CurrIdx >= CounterInfo.Chunks.size())
         return false;
-      if (CounterInfo.StopAfter < 0)
-        return true;
-      return CounterInfo.StopAfter + CounterInfo.Skip >= CounterInfo.Count;
+
+      bool Res = CounterInfo.Chunks[CurrIdx].contains(CurrCount);
+      if (Us.BreakOnLast && CurrIdx == (CounterInfo.Chunks.size() - 1) &&
+          CurrCount == CounterInfo.Chunks[CurrIdx].End) {
+        LLVM_BUILTIN_DEBUGTRAP;
+      }
+      if (CurrCount > CounterInfo.Chunks[CurrIdx].End) {
+        CounterInfo.CurrChunkIdx++;
+
+        /// Handle consecutive blocks.
+        if (CounterInfo.CurrChunkIdx < CounterInfo.Chunks.size() &&
+            CurrCount == CounterInfo.Chunks[CounterInfo.CurrChunkIdx].Begin)
+          return true;
+      }
+      return Res;
     }
     // Didn't find the counter, should we warn?
     return true;
@@ -152,11 +175,11 @@ class DebugCounter {
 #ifdef NDEBUG
     return false;
 #else
-    return instance().Enabled;
+    return instance().Enabled || instance().ShouldPrintCounter;
 #endif
   }
 
-private:
+protected:
   unsigned addCounter(const std::string &Name, const std::string &Desc) {
     unsigned Result = RegisteredCounters.insert(Name);
     Counters[Result] = {};
@@ -166,17 +189,22 @@ class DebugCounter {
   // Struct to store counter info.
   struct CounterInfo {
     int64_t Count = 0;
-    int64_t Skip = 0;
-    int64_t StopAfter = -1;
+    uint64_t CurrChunkIdx = 0;
     bool IsSet = false;
     std::string Desc;
+    SmallVector<Chunk> Chunks;
   };
+
   DenseMap<unsigned, CounterInfo> Counters;
   CounterVector RegisteredCounters;
 
   // Whether we should do DebugCounting at all. DebugCounters aren't
   // thread-safe, so this should always be false in multithreaded scenarios.
   bool Enabled = false;
+
+  bool ShouldPrintCounter = false;
+
+  bool BreakOnLast = false;
 };
 
 #define DEBUG_COUNTER(VARNAME, COUNTERNAME, DESC)                              \
diff --git a/llvm/lib/IR/PassManager.cpp b/llvm/lib/IR/PassManager.cpp
index cbddf3dfb056cf..5dc53b78f5d66a 100644
--- a/llvm/lib/IR/PassManager.cpp
+++ b/llvm/lib/IR/PassManager.cpp
@@ -8,11 +8,20 @@
 
 #include "llvm/IR/PassManager.h"
 #include "llvm/IR/PassManagerImpl.h"
+#include "llvm/Support/DebugCounter.h"
 #include <optional>
 
 using namespace llvm;
 
 namespace llvm {
+
+DEBUG_COUNTER(AddNewPMPassCounter, "new-pm-pass-counter",
+              "Control what passes get run");
+
+bool shouldAddNewPMPass() {
+  return DebugCounter::shouldExecute(AddNewPMPassCounter);
+}
+
 // Explicit template instantiations and specialization defininitions for core
 // template typedefs.
 template class AllAnalysesOn<Module>;
diff --git a/llvm/lib/Support/DebugCounter.cpp b/llvm/lib/Support/DebugCounter.cpp
index 502665d2a8348e..889da1c657a5c2 100644
--- a/llvm/lib/Support/DebugCounter.cpp
+++ b/llvm/lib/Support/DebugCounter.cpp
@@ -7,6 +7,83 @@
 
 using namespace llvm;
 
+namespace llvm {
+
+void Chunk::print(llvm::raw_ostream &OS) {
+  if (Begin == End)
+    OS << Begin;
+  else
+    OS << Begin << "-" << End;
+}
+
+void printChunks(raw_ostream &OS, ArrayRef<Chunk> Chunks) {
+  if (Chunks.empty()) {
+    OS << "empty";
+  } else {
+    bool IsFirst = true;
+    for (auto E : Chunks) {
+      if (!IsFirst)
+        OS << ":";
+      else
+        IsFirst = false;
+      E.print(OS);
+    }
+  }
+}
+
+bool parseChunks(StringRef Str, SmallVector<Chunk> &Chunks) {
+  StringRef Remaining = Str;
+
+  auto ConsumeInt = [&]() -> int64_t {
+    StringRef Number =
+        Remaining.take_until([](char c) { return c < '0' || c > '9'; });
+    int64_t Res;
+    if (Number.getAsInteger(10, Res)) {
+      errs() << "Failed to parse int at : " << Remaining << "\n";
+      return -1;
+    }
+    Remaining = Remaining.drop_front(Number.size());
+    return Res;
+  };
+
+  while (1) {
+    int64_t Num = ConsumeInt();
+    if (Num == -1) {
+      return true;
+    }
+    if (!Chunks.empty() && Num <= Chunks[Chunks.size() - 1].End) {
+      errs() << "Expected Chunks to be in increasing order " << Num
+             << " <= " << Chunks[Chunks.size() - 1].End << "\n";
+      return true;
+    }
+    if (Remaining.starts_with("-")) {
+      Remaining = Remaining.drop_front();
+      int64_t Num2 = ConsumeInt();
+      if (Num2 == -1)
+        return true;
+      if (Num >= Num2) {
+        errs() << "Expected " << Num << " < " << Num2 << " in " << Num << "-"
+               << Num2 << "\n";
+        return true;
+      }
+
+      Chunks.push_back({Num, Num2});
+    } else {
+      Chunks.push_back({Num, Num});
+    }
+    if (Remaining.starts_with(":")) {
+      Remaining = Remaining.drop_front();
+      continue;
+    }
+    if (Remaining.empty())
+      break;
+    errs() << "Failed to parse at : " << Remaining;
+    return true;
+  }
+  return false;
+}
+}
+
 namespace {
 // This class overrides the default list implementation of printing so we
 // can pretty print the list of debug counter options.  This type of
@@ -47,15 +124,26 @@ class DebugCounterList : public cl::list<std::string, DebugCounter> {
 // itself, are owned by a single global instance of the DebugCounterOwner
 // struct. This makes it easier to control the order in which constructors and
 // destructors are run.
-struct DebugCounterOwner {
-  DebugCounter DC;
+struct DebugCounterOwner : DebugCounter {
   DebugCounterList DebugCounterOption{
       "debug-counter", cl::Hidden,
       cl::desc("Comma separated list of debug counter skip and count"),
-      cl::CommaSeparated, cl::location(DC)};
-  cl::opt<bool> PrintDebugCounter{
-      "print-debug-counter", cl::Hidden, cl::init(false), cl::Optional,
+      cl::CommaSeparated, cl::location<DebugCounter>(*this)};
+  cl::opt<bool, true> PrintDebugCounter{
+      "print-debug-counter",
+      cl::Hidden,
+      cl::Optional,
+      cl::location(this->ShouldPrintCounter),
+      cl::init(false),
       cl::desc("Print out debug counter info after all counters accumulated")};
+  cl::opt<bool, true> BreakOnLastCount{
+      "debug-counter-break-on-last",
+      cl::Hidden,
+      cl::Optional,
+      cl::location(this->BreakOnLast),
+      cl::init(false),
+      cl::desc("Insert a break point on the last enabled count of a "
+               "chunks list")};
 
   DebugCounterOwner() {
     // Our destructor uses the debug stream. By referencing it here, we
@@ -65,8 +153,8 @@ struct DebugCounterOwner {
 
   // Print information when destroyed, iff command line option is specified.
   ~DebugCounterOwner() {
-    if (DC.isCountingEnabled() && PrintDebugCounter)
-      DC.print(dbgs());
+    if (ShouldPrintCounter)
+      print(dbgs());
   }
 };
 
@@ -76,7 +164,7 @@ void llvm::initDebugCounterOptions() { (void)DebugCounter::instance(); }
 
 DebugCounter &DebugCounter::instance() {
   static DebugCounterOwner O;
-  return O.DC;
+  return O;
 }
 
 // This is called by the command line parser when it sees a value for the
@@ -84,52 +172,31 @@ DebugCounter &DebugCounter::instance() {
 void DebugCounter::push_back(const std::string &Val) {
   if (Val.empty())
     return;
-  // The strings should come in as counter=value
+
+  // The strings should come in as counter=chunk_list
   auto CounterPair = StringRef(Val).split('=');
   if (CounterPair.second.empty()) {
     errs() << "DebugCounter Error: " << Val << " does not have an = in it\n";
     return;
   }
-  // Now we have counter=value.
-  // First, process value.
-  int64_t CounterVal;
-  if (CounterPair.second.getAsInteger(0, CounterVal)) {
-    errs() << "DebugCounter Error: " << CounterPair.second
-           << " is not a number\n";
+  StringRef CounterName = CounterPair.first;
+  SmallVector<Chunk> Chunks;
+
+  if (parseChunks(CounterPair.second, Chunks)) {
     return;
   }
-  // Now we need to see if this is the skip or the count, remove the suffix, and
-  // add it to the counter values.
-  if (CounterPair.first.ends_with("-skip")) {
-    auto CounterName = CounterPair.first.drop_back(5);
-    unsigned CounterID = getCounterId(std::string(CounterName));
-    if (!CounterID) {
-      errs() << "DebugCounter Error: " << CounterName
-             << " is not a registered counter\n";
-      return;
-    }
-    enableAllCounters();
-
-    CounterInfo &Counter = Counters[CounterID];
-    Counter.Skip = CounterVal;
-    Counter.IsSet = true;
-  } else if (CounterPair.first.ends_with("-count")) {
-    auto CounterName = CounterPair.first.drop_back(6);
-    unsigned CounterID = getCounterId(std::string(CounterName));
-    if (!CounterID) {
-      errs() << "DebugCounter Error: " << CounterName
-             << " is not a registered counter\n";
-      return;
-    }
-    enableAllCounters();
 
-    CounterInfo &Counter = Counters[CounterID];
-    Counter.StopAfter = CounterVal;
-    Counter.IsSet = true;
-  } else {
-    errs() << "DebugCounter Error: " << CounterPair.first
-           << " does not end with -skip or -count\n";
+  unsigned CounterID = getCounterId(std::string(CounterName));
+  if (!CounterID) {
+    errs() << "DebugCounter Error: " << CounterName
+           << " is not a registered counter\n";
+    return;
   }
+  enableAllCounters();
+
+  CounterInfo &Counter = Counters[CounterID];
+  Counter.IsSet = true;
+  Counter.Chunks = std::move(Chunks);
 }
 
 void DebugCounter::print(raw_ostream &OS) const {
@@ -142,8 +209,9 @@ void DebugCounter::print(raw_ostream &OS) const {
   for (auto &CounterName : CounterNames) {
     unsigned CounterID = getCounterId(std::string(CounterName));
     OS << left_justify(RegisteredCounters[CounterID], 32) << ": {"
-       << Us.Counters[CounterID].Count << "," << Us.Counters[CounterID].Skip
-       << "," << Us.Counters[CounterID].StopAfter << "}\n";
+       << Us.Counters[CounterID].Count << ",";
+    printChunks(OS, Us.Counters[CounterID].Chunks);
+    OS << "}\n";
   }
 }
 
diff --git a/llvm/tools/delta-driver/CMakeLists.txt b/llvm/tools/delta-driver/CMakeLists.txt
new file mode 100644
index 00000000000000..54ffbb12b6fb05
--- /dev/null
+++ b/llvm/tools/delta-driver/CMakeLists.txt
@@ -0,0 +1,9 @@
+set(LLVM_LINK_COMPONENTS
+  Support
+  )
+
+add_llvm_tool(delta-driver
+  delta-driver.cpp
+
+  DEPENDS
+  )
diff --git a/llvm/tools/delta-driver/delta-driver.cpp b/llvm/tools/delta-driver/delta-driver.cpp
new file mode 100644
index 00000000000000..6562493f967f0c
--- /dev/null
+++ b/llvm/tools/delta-driver/delta-driver.cpp
@@ -0,0 +1,151 @@
+//===-- delta-driver.cpp - Tool to drive Automated Delta Debugging --------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// See the llvm-project/llvm/docs/ProgrammersManual.rst to see how to use this
+// tool
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/DebugCounter.h"
+#include "llvm/Support/Program.h"
+
+using namespace llvm;
+
+cl::opt<std::string> ReproductionCmd(cl::Positional, cl::Required);
+
+cl::opt<std::string> StartChunks(cl::Positional, cl::Required);
+
+cl::opt<bool> Pessimist("pessimist", cl::init(false));
+
+SmallVector<Chunk> simplifyChunksList(const SmallVector<Chunk> &Chunks) {
+  SmallVector<Chunk> Res;
+  Res.push_back(Chunks.front());
+  for (unsigned Idx = 1; Idx < Chunks.size(); Idx++) {
+    if (Chunks[Idx].Begin == Res.back().End + 1)
+      Res.back().End = Chunks[Idx].End;
+    else
+      Res.push_back(Chunks[Idx]);
+  }
+  return Res;
+}
+
+bool stillReproducesIssue(const SmallVector<Chunk> &Chunks) {
+  SmallVector<Chunk> SimpleChunks = simplifyChunksList(Chunks);
+
+  std::string ChunkStr;
+  {
+    raw_string_ostream OS(ChunkStr);
+    printChunks(OS, SimpleChunks);
+  }
+
+  errs() << "Checking with: " << ChunkStr << "\n";
+
+  std::vector<StringRef> Argv;
+  Argv.push_back(ReproductionCmd);
+  Argv.push_back(ChunkStr);
+
+  std::string ErrMsg;
+  bool ExecutionFailed;
+  int Result = sys::ExecuteAndWait(Argv[0], Argv, std::nullopt, {}, 0, 0,
+                                   &ErrMsg, &ExecutionFailed);
+  if (ExecutionFailed) {
+    errs() << "failed to execute : " << Argv[0] << " : " << ErrMsg << "\n";
+    exit(1);
+  }
+
+  bool Res = Result != 0;
+  if (Res) {
+    errs() << "SUCCESS : Still Interesting\n";
+  } else {
+    errs() << "FAILURE : Not Interesting\n";
+  }
+  return Res;
+}
+
+static bool increaseGranularity(SmallVector<Chunk> &Chunks) {
+  errs() << "Increasing granularity\n";
+  SmallVector<Chunk> NewChunks;
+  bool SplitOne = false;
+
+  for (auto &C : Chunks) {
+    if (C.Begin == C.End)
+      NewChunks.push_back(C);
+    else {
+      int Half = (C.Begin + C.End) / 2;
+      NewChunks.push_back({C.Begin, Half});
+      NewChunks.push_back({Half + 1, C.End});
+      SplitOne = true;
+    }
+  }
+  if (SplitOne) {
+    Chunks = std::move(NewChunks);
+  }
+  return SplitOne;
+}
+
+int main(int argc, char **argv) {
+  cl::ParseCommandLineOptions(argc, argv);
+
+  SmallVector<Chunk> CurrChunks;
+  if (parseChunks(StartChunks, CurrChunks)) {
+    return 1;
+  }
+
+  auto Program = sys::findProgramByName(ReproductionCmd);
+  if (!Program) {
+    errs() << "failed to find command : " << ReproductionCmd << "\n";
+    return 1;
+  }
+  ReproductionCmd.setValue(Program.get());
+
+  errs() << "Input Checking:\n";
+  if (!stillReproducesIssue(CurrChunks)) {
+    errs() << "starting chunks are not interesting\n";
+    return 1;
+  }
+  if (CurrChunks.size() == 1)
+    increaseGranularity(CurrChunks);
+  if (Pessimist)
+    while (increaseGranularity(CurrChunks)) {
+    }
+  while (1) {
+    SmallDenseSet<unsigned> NotNeedChunks;
+    auto FilteredCopy = [&]() {
+      SmallVector<Chunk> CopiedChunks;
+      for (unsigned SubIdx = 0; SubIdx < CurrChunks.size(); SubIdx++)
+        if (!NotNeedChunks.count(SubIdx))
+          CopiedChunks.push_back(CurrChunks[SubIdx]);
+      return CopiedChunks;
+    };
+
+    for (int Idx = (CurrChunks.size() - 1); Idx >= 0; Idx--) {
+      if (NotNeedChunks.size() + 1 == CurrChunks.size())
+        break;
+
+      errs() << "Trying to remove : ";
+      CurrChunks[Idx].print(errs());
+      errs() << "\n";
+
+      NotNeedChunks.insert(Idx);
+      SmallVector<Chunk> NextChunks = FilteredCopy();
+      if (!stillReproducesIssue(NextChunks)) {
+        NotNeedChunks.erase(Idx);
+      }
+    }
+    CurrChunks = FilteredCopy();
+    bool HasSplit = increaseGranularity(CurrChunks);
+    if (!HasSplit)
+      break;
+  }
+
+  errs() << "Minimal Chunks = ";
+  printChunks(llvm::errs(), simplifyChunksList(CurrChunks));
+  errs() << "\n";
+}
diff --git a/llvm/unittests/Support/DebugCounterTest.cpp b/llvm/unittests/Support/DebugCounterTest.cpp
index e7345b13cc1729..820b1ce5ca0d37 100644
--- a/llvm/unittests/Support/DebugCounterTest.cpp
+++ b/llvm/unittests/Support/DebugCounterTest.cpp
@@ -13,28 +13,28 @@
 using namespace llvm;
 
 #ifndef NDEBUG
-TEST(DebugCounterTest, CounterCheck) {
+TEST(DebugCounterTest, Basic) {
   DEBUG_COUNTER(TestCounter, "test-counter", "Counter used for unit test");
 
   EXPECT_FALSE(DebugCounter::isCounterSet(TestCounter));
-
   auto DC = &DebugCounter::instance();
-  DC->push_back("test-counter-skip=1");
-  DC->push_back("test-counter-count=3");
+  DC->push_back(...
[truncated]

[Ralender]

I didn't fix the automated test using the old debug counter command line. but I will if there is interest.

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[nhaehnle]

Hi, sorry it's been taking so long! I think this is a very nice addition to the DebugCounter functionality indeed.

Would you mind splitting the bisector changes out in a separate PR? It's not immediately obvious to me what it is for, and it seems likely to be logically independent. (IIUC, I think you'd have one PR for the DebugCounter changes + delta-driver, and one PR for the PassManager changes + bisector?)

I have a bunch of comments inline, and high-level comments:

• I'm not convinced by the delta-driver name. The name should have some connection to the DebugCounter facility. The previous tool is called bisect-skip-count. Maybe something like "minimize-debug-counter" for the new tool?
• Take the GitHub Actions complaints seriously; this means formatting feedback and the test failures.

With those notes addressed, I'd be happy to see this change go in.

[Ralender]

Thanks for reviewing. I will address your comments soon. I just wanted to answer some of your questions. I didn't yet update formatting and test failure because I didn't know if there was going to be interest.

The reason I named it delta-driver is that it is not intrinsically linked to debug counters. what it does is drive delta debugging logic on a chunk list. this can be applied to more than debug counters.

the bisector is one of the other uses of the delta-driver to find which translation unit or set of translation unit are needed to reproduce an issue. this is what I use locally to minimize a miss-complie in a multi-file project to 1 translation unit. I guess everyone has one of these so I don't know if there is interest on upstreaming one. bisector_demo.sh is a demo of how to use the bisector and delta-driver, I also used it as a test when I was writing the bisector.

[nhaehnle]

Thank you for making those changes! All the strictly DebugCounter-related changes look good to me, though I just realized that DebugCounter.h has a file header that is obsoleted by your changes so please update that.

I still think the "delta-driver" name is too generic. Yes, it could hypothetically be used for more than just debug counter. On the other hand, bugpoint and llvm-reduce are also delta debugging tools but don't make as far-reaching a claim to being "delta drivers". So I still think searching for a better name is worth it. I proposed minimize-debug-counter previously -- how about reduce-counter-chunks or reduce-indices or something of that nature? It seems to me that there is inherently something being counted here since the tool is really reducing a set of numbers. What do you think?

Apart from that, there are still two changes that just don't live up to the standards we should expect as far as I can see, and frankly it'd be best to put them in a separate PR because they are logically separate:

• The PassManager changes; there isn't even a test for the new counter, let alone documentation
• The bisector tool is wholly undocumented. Thanks for explaining it a bit more in the PR, but I think it needs some more lasting documentation, if only in a header comment.

The bisector also suffers from problematic naming IMHO. What exactly does it bisect? You may as well have called it "delta-driver", to be honest. After all, its meant for delta debugging and drives compilers :)

[nhaehnle]

Thanks, the changes look good to me! The Linux build has a failure in BOLT. I don't see how that could possibly be related, so...

[Ralender]

Should we wait for approval/comments from other people ?

[nhaehnle]

IMHO you've given enough time to others to take a look. I'd err on the side of action here. (In rare cases, somebody may speak up in post-commit review and you do have a responsibility to address that if it comes up. But it's probably fine.)

[nico]

Any chance this could've caused http://45.33.8.238/linux/139041/step_11.txt ? There's nothing else in the regression range for the failure really.

[Ralender]

Any chance this could've caused http://45.33.8.238/linux/139041/step_11.txt ? There's nothing else in the regression range for the failure really.

Yes, I will look into it.

[TomWeaver18]

Hello and good afternoon from the UK,

It looks as though the following commit:

d46e373

has caused a recent build bot failure here:

https://lab.llvm.org/buildbot/#/builders/247/builds/19014

Are you able to take a look see?

It's specifically these two tests:

• FAIL: LLVM::peephole-counter-XToI.mir
• FAIL: LLVM::peephole-counter-perOp.mir

Many thanks,
Tom W

[Ralender]

6e1a042
should fix:
FAIL: LLVM::peephole-counter-XToI.mir
FAIL: LLVM::peephole-counter-perOp.mir