[llvm] r337748 - [DebugCounters] Keep track of total counts
Ilya Biryukov via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 31 09:58:06 PDT 2018
Using atomics silences the TSAN warnings: https://reviews.llvm.org/D50080
The change is a bit ugly, though.
On Tue, Jul 31, 2018 at 5:30 PM Ilya Biryukov <ibiryukov at google.com> wrote:
> 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
>
--
Regards,
Ilya Biryukov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180731/96a2c257/attachment.html>
More information about the llvm-commits
mailing list