[llvm] r337748 - [DebugCounters] Keep track of total counts
Ilya Biryukov via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 31 08:30:01 PDT 2018
After this change TSAN started complaining that 'shouldExecute' has data
races:
Write of size 8 at 0x7b78000022b8 by thread T104 (mutexes: write
M322987330275573248):
#0 shouldExecute llvm/include/llvm/Support/DebugCounter.h:81:7
#1 (anonymous
namespace)::EarlyCSE::processNode(llvm::DomTreeNodeBase<llvm::BasicBlock>*)
llvm/lib/Transforms/Scalar/EarlyCSE.cpp:903
#2 (anonymous namespace)::EarlyCSE::run()
llvm/lib/Transforms/Scalar/EarlyCSE.cpp:1197:18
#3 (anonymous
namespace)::EarlyCSELegacyCommonPass<false>::runOnFunction(llvm::Function&)
llvm/lib/Transforms/Scalar/EarlyCSE.cpp:1277:16
#4 llvm::FPPassManager::runOnFunction(llvm::Function&)
llvm/lib/IR/LegacyPassManager.cpp:1586:27
#5 llvm::legacy::FunctionPassManagerImpl::run(llvm::Function&)
llvm/lib/IR/LegacyPassManager.cpp:1532:44
(This happens on tensorflow with XLA enabled, in case you're interested in
where the error comes from).
It does not seem debug counters were ever thread-safe, but at least they
did not produce any TSAN errors when disabled.
I guess we can add mutexes and make the counters thread-safe.
Does that LG? Any other ideas on what's the best way to fix the debug
counters so we can silence TSAN (it's probably fine if they don't really
work in multi-threaded environments)?
On Mon, Jul 23, 2018 at 11:49 PM George Burgess IV via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: gbiv
> Date: Mon Jul 23 14:49:36 2018
> New Revision: 337748
>
> URL: http://llvm.org/viewvc/llvm-project?rev=337748&view=rev
> Log:
> [DebugCounters] Keep track of total counts
>
> This patch makes debug counters keep track of the total number of times
> we've called `shouldExecute` for each counter, so it's easier to build
> automated tooling on top of these.
>
> A patch to print these counts is coming soon.
>
> Patch by Zhizhou Yang!
>
> Differential Revision: https://reviews.llvm.org/D49560
>
> Added:
> llvm/trunk/unittests/Support/DebugCounterTest.cpp
> Modified:
> llvm/trunk/include/llvm/Support/DebugCounter.h
> llvm/trunk/lib/Support/DebugCounter.cpp
> llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
> llvm/trunk/unittests/Support/CMakeLists.txt
>
> Modified: llvm/trunk/include/llvm/Support/DebugCounter.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/DebugCounter.h?rev=337748&r1=337747&r2=337748&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/DebugCounter.h (original)
> +++ llvm/trunk/include/llvm/Support/DebugCounter.h Mon Jul 23 14:49:36 2018
> @@ -77,23 +77,19 @@ public:
> 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;
> @@ -104,21 +100,21 @@ public:
> // 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 instance().Counters[ID].IsSet;
> }
>
> - // 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().
> @@ -136,7 +132,7 @@ public:
>
> // Return the name and description of the counter with the given ID.
> std::pair<std::string, std::string> getCounterInfo(unsigned ID) const {
> - return std::make_pair(RegisteredCounters[ID], CounterDesc.lookup(ID));
> + return std::make_pair(RegisteredCounters[ID],
> Counters.lookup(ID).Desc);
> }
>
> // Iterate through the registered counters
> @@ -149,11 +145,19 @@ public:
> private:
> unsigned addCounter(const std::string &Name, const std::string &Desc) {
> unsigned Result = RegisteredCounters.insert(Name);
> - CounterDesc[Result] = Desc;
> + Counters[Result] = {};
> + Counters[Result].Desc = Desc;
> return Result;
> }
> - DenseMap<unsigned, std::pair<long, long>> Counters;
> - DenseMap<unsigned, std::string> CounterDesc;
> + // Struct to store counter info.
> + struct CounterInfo {
> + int64_t Count = 0;
> + int64_t Skip = 0;
> + int64_t StopAfter = -1;
> + bool IsSet = false;
> + std::string Desc;
> + };
> + DenseMap<unsigned, CounterInfo> Counters;
> CounterVector RegisteredCounters;
> };
>
>
> Modified: llvm/trunk/lib/Support/DebugCounter.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/DebugCounter.cpp?rev=337748&r1=337747&r2=337748&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Support/DebugCounter.cpp (original)
> +++ llvm/trunk/lib/Support/DebugCounter.cpp Mon Jul 23 14:49:36 2018
> @@ -66,7 +66,7 @@ void DebugCounter::push_back(const std::
> }
> // 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";
> @@ -76,26 +76,24 @@ void DebugCounter::push_back(const std::
> // 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;
> + Counters[CounterID].Skip = CounterVal;
> + Counters[CounterID].IsSet = true;
> } 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;
> + Counters[CounterID].StopAfter = CounterVal;
> + Counters[CounterID].IsSet = true;
> } else {
> errs() << "DebugCounter Error: " << CounterPair.first
> << " does not end with -skip or -count\n";
> @@ -106,7 +104,8 @@ void DebugCounter::print(raw_ostream &OS
> 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 {
>
> Modified: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp?rev=337748&r1=337747&r2=337748&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp Mon Jul 23 14:49:36 2018
> @@ -861,7 +861,7 @@ private:
>
> // 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
>
> Modified: llvm/trunk/unittests/Support/CMakeLists.txt
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CMakeLists.txt?rev=337748&r1=337747&r2=337748&view=diff
>
> ==============================================================================
> --- llvm/trunk/unittests/Support/CMakeLists.txt (original)
> +++ llvm/trunk/unittests/Support/CMakeLists.txt Mon Jul 23 14:49:36 2018
> @@ -20,6 +20,7 @@ add_llvm_unittest(SupportTests
> ConvertUTFTest.cpp
> DataExtractorTest.cpp
> DebugTest.cpp
> + DebugCounterTest.cpp
> DJBTest.cpp
> EndianStreamTest.cpp
> EndianTest.cpp
>
> Added: llvm/trunk/unittests/Support/DebugCounterTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/DebugCounterTest.cpp?rev=337748&view=auto
>
> ==============================================================================
> --- llvm/trunk/unittests/Support/DebugCounterTest.cpp (added)
> +++ llvm/trunk/unittests/Support/DebugCounterTest.cpp Mon Jul 23 14:49:36
> 2018
> @@ -0,0 +1,42 @@
> +//===- 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) {
> + EXPECT_FALSE(DebugCounter::isCounterSet(TestCounter));
> +
> + auto DC = &DebugCounter::instance();
> + DC->push_back("test-counter-skip=1");
> + DC->push_back("test-counter-count=3");
> +
> + EXPECT_TRUE(DebugCounter::isCounterSet(TestCounter));
> +
> + 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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
--
Regards,
Ilya Biryukov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180731/5d357849/attachment.html>
More information about the llvm-commits
mailing list