Add support to track total counts for DebugCounter

[zhizhouy]

Summary

This patch introduces a new member Count to DebugCounter so that it now can track the total number of execution for the Counter.

Adding this feature will help us to do automated transformation level bisecting better using DebugCounter.

One (only) use case in NewGVN.cpp is also modified since we changed related functions.

We are planning to add an actual printing for this info in a follow-up.

[george.burgess.iv]

Thanks for this!

Looks like these were added in D29998, so I'm tagging davide, who reviewed that, in case there's context/history I'm missing.

include/llvm/Support/DebugCounter.h

81

nit: This comment sort of feels redundant to me.

90

Should this be >= Count?

e.g. if I ask for skip=1 + stop-after=1, we'll end up having Count == 2 on the second call to shouldExecute (which should return true).

91–92

nit: this can be folded into return CounterInfo.StopAfter + CounterInfo.Skip > CounterInfo.Count;

150

nit: can this just be Counters[Result] = {};?

157

I don't know the history of this code, but it seems reasonable to me to make this a vector, add a string Desc to CounterInfo, and drop CounterDesc entirely. CounterVector just returns [1..UINT_MAX], in order, on insertion anyway.

(Alternatively, it seems that we could replace all three of these containers with a single StringMap<CounterInfo> and hand CounterInfo *s out instead of unsigneds, but that starts to stray pretty far from the goal of this patch, so... :) )

[davide]

Can you add a unittest for the new functionality?

include/llvm/Support/DebugCounter.h

153–155

int64_t ?

[zhizhouy]

Added a unit test for the change.

Tested with it and also three existing regression tests for debugcounter in test/Other.

[zhizhouy]

include/llvm/Support/DebugCounter.h

157

I have moved Desc into the CounterInfo struct. Using vector to replace DenseMap may cause some ambiguity from UniqueVector on index. So I am keeping the DenseMap for now. We may come up with another patch to do the second change you mentioned.

[george.burgess.iv]

include/llvm/Support/DebugCounter.h

157

This WFM, but I don't quite understand what you mean by

Using vector to replace DenseMap may cause some ambiguity from UniqueVector on index

For my curiosity, can you elaborate a bit, please? :)

unittests/Support/DebugCounterTest.cpp

2

Is this 80 column aligned?

17

We probably want to put this under the NDEBUG guard, as well.

Otherwise, we'll have an unused static, which will probably cause compiler warnings.

[zhizhouy]

include/llvm/Support/DebugCounter.h

157

So in DebugCounter::print() and getCounterInfo(), ID for RegisteredCounters and Counters comes up together. So no matter we return ID or ID-1, we need to +/- 1 or one of them.

[george.burgess.iv]

This LGTM; thanks again for the patch. Will land it for you once davide's happy

[george.burgess.iv]

Running ninja check-llvm after applying this on top of r337631, I see many test failures in CodeGen/AMDGPU. Are you seeing these, as well?

[zhizhouy]

In D49560#1170719, @george.burgess.iv wrote:

Running ninja check-llvm after applying this on top of r337631, I see many test failures in CodeGen/AMDGPU. Are you seeing these, as well?

Yes, I see them as well. Had a glance in the tests but could not find related code. Will investigate into it.

[zhizhouy]

Fix CodeGen/AMDGPU test failures, by adding Counters only when skip/count is set.
I believe this will save many memory when skip/count is not set, thus fixing the problem.

As a result, I also reverted the changes to make CounterDesc part of CounterInfo.

Tested with ninja check-llvm and ninja check-llvm-unit.

[george.burgess.iv]

As a result, I also reverted the changes to make CounterDesc part of CounterInfo.

Sorry, I'm confused. Is the goal here to ultimately print all of the counts? I don't see how that can happen if we're only Counting for things which have skip or stopAfter specified on the opt line.

this will save many memory when skip/count is not set, thus fixing the problem

I'm assuming you meant that the memory savings is a nice side-effect, and not the cause of the problem (which appears to be that we're now always saying that these counters are set). If not, +~300 bytes of heap crashing a test is scary. :)

[zhizhouy]

Fixed the failure issue.

The tests failures are actually caused by isCounterSet(). This version keeps the behavior of the function by checking if command line option is set or not.

[george.burgess.iv]

Thanks again!

include/llvm/Support/DebugCounter.h

public:
   inline static bool shouldExecute(unsigned CounterName) {
 // Compile to nothing when debugging is off
 #ifdef NDEBUG
     return true;
 #else
     auto &Us = instance();
     auto Result = Us.Counters.find(CounterName);
     if (Result != Us.Counters.end()) {
-      auto &CounterPair = Result->second;
-      // We only execute while the skip (first) is zero and the count (second)
-      // is non-zero.
+      auto &CounterInfo = Result->second;
+      ++CounterInfo.Count;
+
+      // We only execute while the Skip is not smaller than Count,
+      // and the StopAfter + Skip is larger than Count.
       // Negative counters always execute.
-      if (CounterPair.first < 0)
+      if (CounterInfo.Skip < 0)
         return true;
-      if (CounterPair.first != 0) {
-        --CounterPair.first;
+      if (CounterInfo.Skip >= CounterInfo.Count)
         return false;
-      }
-      if (CounterPair.second < 0)
-        return true;
-      if (CounterPair.second != 0) {
-        --CounterPair.second;
+      if (CounterInfo.StopAfter < 0)
         return true;
-      }
-      return false;
+      return CounterInfo.StopAfter + CounterInfo.Skip >= CounterInfo.Count;
     }
     // Didn't find the counter, should we warn?
     return true;
 #endif // NDEBUG
   }
 
   // Return true if a given counter had values set (either programatically or on
   // the command line).  This will return true even if those values are
   // currently in a state where the counter will always execute.
   static bool isCounterSet(unsigned ID) {
     return instance().Counters.count(ID);
   }
 
-  // Return the skip and count for a counter. This only works for set counters.
-  static std::pair<int, int> getCounterValue(unsigned ID) {
+  // Return the Count for a counter. This only works for set counters.
+  static int64_t getCounterValue(unsigned ID) {
     auto &Us = instance();
     auto Result = Us.Counters.find(ID);
     assert(Result != Us.Counters.end() && "Asking about a non-set counter");
-    return Result->second;
+    return Result->second.Count;
   }
 
-  // Set a registered counter to a given value.
-  static void setCounterValue(unsigned ID, const std::pair<int, int> &Val) {
+  // Set a registered counter to a given Count value.
+  static void setCounterValue(unsigned ID, int64_t Count) {
     auto &Us = instance();
-    Us.Counters[ID] = Val;
+    Us.Counters[ID].Count = Count;
   }
 
   // Dump or print the current counter set into llvm::dbgs().
   LLVM_DUMP_METHOD void dump() const;
 
   void print(raw_ostream &OS) const;
 
   // Get the counter ID for a given named counter, or return 0 if none is found.
#endif // NDEBUG
   }
   CounterVector::const_iterator end() const { return RegisteredCounters.end(); }
 
 private:
   unsigned addCounter(const std::string &Name, const std::string &Desc) {
     unsigned Result = RegisteredCounters.insert(Name);
     CounterDesc[Result] = Desc;
     return Result;
   }
-  DenseMap<unsigned, std::pair<long, long>> Counters;
+  // Struct to store counter info.
+  struct CounterInfo {
+    int64_t Count = 0;
+    int64_t Skip = 0;
+    int64_t StopAfter = -1;
+  };
+  DenseMap<unsigned, CounterInfo> Counters;
   DenseMap<unsigned, std::string> CounterDesc;
   CounterVector RegisteredCounters;
 };
 
 #define DEBUG_COUNTER(VARNAME, COUNTERNAME, DESC)                              \
   static const unsigned VARNAME =                                              \
       DebugCounter::registerCounter(COUNTERNAME, DESC)
 
 } // namespace llvm
 #endif

lib/Support/DebugCounter.cpp

void DebugCounter::push_back(const std::string &Val) {
   // The strings should come in as counter=value
   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.
-  long CounterVal;
+  int64_t CounterVal;
   if (CounterPair.second.getAsInteger(0, CounterVal)) {
     errs() << "DebugCounter Error: " << CounterPair.second
            << " is not a number\n";
     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.endswith("-skip")) {
     auto CounterName = CounterPair.first.drop_back(5);
-    unsigned CounterID = RegisteredCounters.idFor(CounterName);
+    unsigned CounterID = getCounterId(CounterName);
     if (!CounterID) {
       errs() << "DebugCounter Error: " << CounterName
              << " is not a registered counter\n";
       return;
     }
-
-    auto Res = Counters.insert({CounterID, {0, -1}});
-    Res.first->second.first = CounterVal;
+    auto Res = Counters.insert({CounterID, {}});
+    Res.first->second.Skip = CounterVal;
   } else if (CounterPair.first.endswith("-count")) {
     auto CounterName = CounterPair.first.drop_back(6);
-    unsigned CounterID = RegisteredCounters.idFor(CounterName);
+    unsigned CounterID = getCounterId(CounterName);
     if (!CounterID) {
       errs() << "DebugCounter Error: " << CounterName
              << " is not a registered counter\n";
       return;
     }
-
-    auto Res = Counters.insert({CounterID, {0, -1}});
-    Res.first->second.second = CounterVal;
+    auto Res = Counters.insert({CounterID, {}});
+    Res.first->second.StopAfter = CounterVal;
   } else {
     errs() << "DebugCounter Error: " << CounterPair.first
            << " does not end with -skip or -count\n";
   }
 }
 
 void DebugCounter::print(raw_ostream &OS) const {
   OS << "Counters and values:\n";
   for (const auto &KV : Counters)
     OS << left_justify(RegisteredCounters[KV.first], 32) << ": {"
-       << KV.second.first << "," << KV.second.second << "}\n";
+       << KV.second.Count << "," << KV.second.Skip << ","
+       << KV.second.StopAfter << "}\n";
 }
 
 LLVM_DUMP_METHOD void DebugCounter::dump() const {
   print(dbgs());
 }

lib/Transforms/Scalar/NewGVN.cpp

return isa<MemoryUseOrDef>(MA)
                : InstrDFS.lookup(MA);
   }
 
   bool isCycleFree(const Instruction *) const;
   bool isBackedge(BasicBlock *From, BasicBlock *To) const;
 
   // Debug counter info.  When verifying, we have to reset the value numbering
   // debug counter to the same state it started in to get the same results.
-  std::pair<int, int> StartingVNCounter;
+  int64_t StartingVNCounter;
 };
 
 } // end anonymous namespace
 
 template <typename T>
 static bool equalsLoadStoreHelper(const T &LHS, const Expression &RHS) {
   if (!isa<LoadExpression>(RHS) && !isa<StoreExpression>(RHS))
     return false;

unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   Casting.cpp
   CheckedArithmeticTest.cpp
   Chrono.cpp
   CommandLineTest.cpp
   CompressionTest.cpp
   ConvertUTFTest.cpp
   DataExtractorTest.cpp
   DebugTest.cpp
+  DebugCounterTest.cpp
   DJBTest.cpp
   EndianStreamTest.cpp
   EndianTest.cpp
   ErrnoTest.cpp
   ErrorOrTest.cpp
   ErrorTest.cpp
   FileOutputBufferTest.cpp
   FormatVariadicTest.cpp

unittests/Support/DebugCounterTest.cpp

+//===- llvm/unittest/Support/DebugCounterTest.cpp -------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/DebugCounter.h"
+#include "gtest/gtest.h"
+
+#include <string>
+using namespace llvm;
+
+#ifndef NDEBUG
+DEBUG_COUNTER(TestCounter, "test-counter",
+              "Counter used for unit test");
+
+TEST(DebugCounterTest, CounterCheck) {
+  auto DC = &DebugCounter::instance();
+  DC->push_back("test-counter-skip=1");
+  DC->push_back("test-counter-count=3");
+
+  EXPECT_EQ(0, DebugCounter::getCounterValue(TestCounter));
+  EXPECT_FALSE(DebugCounter::shouldExecute(TestCounter));
+
+  EXPECT_EQ(1, DebugCounter::getCounterValue(TestCounter));
+  EXPECT_TRUE(DebugCounter::shouldExecute(TestCounter));
+
+  DebugCounter::setCounterValue(TestCounter, 3);
+  EXPECT_TRUE(DebugCounter::shouldExecute(TestCounter));
+  EXPECT_FALSE(DebugCounter::shouldExecute(TestCounter));
+
+  DebugCounter::setCounterValue(TestCounter, 100);
+  EXPECT_FALSE(DebugCounter::shouldExecute(TestCounter));
+}
+#endif