<div dir="ltr">After this change TSAN started complaining that 'shouldExecute' has data races:<div><pre id="gmail-test-log" class="gmail-log" style="margin:0px;white-space:pre-wrap;padding:1em 0px;word-wrap:break-word;word-break:break-word;color:rgb(0,0,0);text-decoration-style:initial;text-decoration-color:initial"> 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
</pre><div>(This happens on tensorflow with XLA enabled, in case you're interested in where the error comes from).</div><div><br></div><div>It does not seem debug counters were ever thread-safe, but at least they did not produce any TSAN errors when disabled.</div><div>I guess we can add mutexes and make the counters thread-safe. </div><div>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)?</div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Jul 23, 2018 at 11:49 PM George Burgess IV via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: gbiv<br>
Date: Mon Jul 23 14:49:36 2018<br>
New Revision: 337748<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=337748&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=337748&view=rev</a><br>
Log:<br>
[DebugCounters] Keep track of total counts<br>
<br>
This patch makes debug counters keep track of the total number of times<br>
we've called `shouldExecute` for each counter, so it's easier to build<br>
automated tooling on top of these.<br>
<br>
A patch to print these counts is coming soon.<br>
<br>
Patch by Zhizhou Yang!<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D49560" rel="noreferrer" target="_blank">https://reviews.llvm.org/D49560</a><br>
<br>
Added:<br>
llvm/trunk/unittests/Support/DebugCounterTest.cpp<br>
Modified:<br>
llvm/trunk/include/llvm/Support/DebugCounter.h<br>
llvm/trunk/lib/Support/DebugCounter.cpp<br>
llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp<br>
llvm/trunk/unittests/Support/CMakeLists.txt<br>
<br>
Modified: llvm/trunk/include/llvm/Support/DebugCounter.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/DebugCounter.h?rev=337748&r1=337747&r2=337748&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/DebugCounter.h?rev=337748&r1=337747&r2=337748&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/DebugCounter.h (original)<br>
+++ llvm/trunk/include/llvm/Support/DebugCounter.h Mon Jul 23 14:49:36 2018<br>
@@ -77,23 +77,19 @@ public:<br>
auto &Us = instance();<br>
auto Result = Us.Counters.find(CounterName);<br>
if (Result != Us.Counters.end()) {<br>
- auto &CounterPair = Result->second;<br>
- // We only execute while the skip (first) is zero and the count (second)<br>
- // is non-zero.<br>
+ auto &CounterInfo = Result->second;<br>
+ ++CounterInfo.Count;<br>
+<br>
+ // We only execute while the Skip is not smaller than Count,<br>
+ // and the StopAfter + Skip is larger than Count.<br>
// Negative counters always execute.<br>
- if (CounterPair.first < 0)<br>
+ if (CounterInfo.Skip < 0)<br>
return true;<br>
- if (CounterPair.first != 0) {<br>
- --CounterPair.first;<br>
+ if (CounterInfo.Skip >= CounterInfo.Count)<br>
return false;<br>
- }<br>
- if (CounterPair.second < 0)<br>
- return true;<br>
- if (CounterPair.second != 0) {<br>
- --CounterPair.second;<br>
+ if (CounterInfo.StopAfter < 0)<br>
return true;<br>
- }<br>
- return false;<br>
+ return CounterInfo.StopAfter + CounterInfo.Skip >= CounterInfo.Count;<br>
}<br>
// Didn't find the counter, should we warn?<br>
return true;<br>
@@ -104,21 +100,21 @@ public:<br>
// the command line). This will return true even if those values are<br>
// currently in a state where the counter will always execute.<br>
static bool isCounterSet(unsigned ID) {<br>
- return instance().Counters.count(ID);<br>
+ return instance().Counters[ID].IsSet;<br>
}<br>
<br>
- // Return the skip and count for a counter. This only works for set counters.<br>
- static std::pair<int, int> getCounterValue(unsigned ID) {<br>
+ // Return the Count for a counter. This only works for set counters.<br>
+ static int64_t getCounterValue(unsigned ID) {<br>
auto &Us = instance();<br>
auto Result = Us.Counters.find(ID);<br>
assert(Result != Us.Counters.end() && "Asking about a non-set counter");<br>
- return Result->second;<br>
+ return Result->second.Count;<br>
}<br>
<br>
- // Set a registered counter to a given value.<br>
- static void setCounterValue(unsigned ID, const std::pair<int, int> &Val) {<br>
+ // Set a registered counter to a given Count value.<br>
+ static void setCounterValue(unsigned ID, int64_t Count) {<br>
auto &Us = instance();<br>
- Us.Counters[ID] = Val;<br>
+ Us.Counters[ID].Count = Count;<br>
}<br>
<br>
// Dump or print the current counter set into llvm::dbgs().<br>
@@ -136,7 +132,7 @@ public:<br>
<br>
// Return the name and description of the counter with the given ID.<br>
std::pair<std::string, std::string> getCounterInfo(unsigned ID) const {<br>
- return std::make_pair(RegisteredCounters[ID], CounterDesc.lookup(ID));<br>
+ return std::make_pair(RegisteredCounters[ID], Counters.lookup(ID).Desc);<br>
}<br>
<br>
// Iterate through the registered counters<br>
@@ -149,11 +145,19 @@ public:<br>
private:<br>
unsigned addCounter(const std::string &Name, const std::string &Desc) {<br>
unsigned Result = RegisteredCounters.insert(Name);<br>
- CounterDesc[Result] = Desc;<br>
+ Counters[Result] = {};<br>
+ Counters[Result].Desc = Desc;<br>
return Result;<br>
}<br>
- DenseMap<unsigned, std::pair<long, long>> Counters;<br>
- DenseMap<unsigned, std::string> CounterDesc;<br>
+ // Struct to store counter info.<br>
+ struct CounterInfo {<br>
+ int64_t Count = 0;<br>
+ int64_t Skip = 0;<br>
+ int64_t StopAfter = -1;<br>
+ bool IsSet = false;<br>
+ std::string Desc;<br>
+ };<br>
+ DenseMap<unsigned, CounterInfo> Counters;<br>
CounterVector RegisteredCounters;<br>
};<br>
<br>
<br>
Modified: llvm/trunk/lib/Support/DebugCounter.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/DebugCounter.cpp?rev=337748&r1=337747&r2=337748&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/DebugCounter.cpp?rev=337748&r1=337747&r2=337748&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Support/DebugCounter.cpp (original)<br>
+++ llvm/trunk/lib/Support/DebugCounter.cpp Mon Jul 23 14:49:36 2018<br>
@@ -66,7 +66,7 @@ void DebugCounter::push_back(const std::<br>
}<br>
// Now we have counter=value.<br>
// First, process value.<br>
- long CounterVal;<br>
+ int64_t CounterVal;<br>
if (CounterPair.second.getAsInteger(0, CounterVal)) {<br>
errs() << "DebugCounter Error: " << CounterPair.second<br>
<< " is not a number\n";<br>
@@ -76,26 +76,24 @@ void DebugCounter::push_back(const std::<br>
// add it to the counter values.<br>
if (CounterPair.first.endswith("-skip")) {<br>
auto CounterName = CounterPair.first.drop_back(5);<br>
- unsigned CounterID = RegisteredCounters.idFor(CounterName);<br>
+ unsigned CounterID = getCounterId(CounterName);<br>
if (!CounterID) {<br>
errs() << "DebugCounter Error: " << CounterName<br>
<< " is not a registered counter\n";<br>
return;<br>
}<br>
-<br>
- auto Res = Counters.insert({CounterID, {0, -1}});<br>
- Res.first->second.first = CounterVal;<br>
+ Counters[CounterID].Skip = CounterVal;<br>
+ Counters[CounterID].IsSet = true;<br>
} else if (CounterPair.first.endswith("-count")) {<br>
auto CounterName = CounterPair.first.drop_back(6);<br>
- unsigned CounterID = RegisteredCounters.idFor(CounterName);<br>
+ unsigned CounterID = getCounterId(CounterName);<br>
if (!CounterID) {<br>
errs() << "DebugCounter Error: " << CounterName<br>
<< " is not a registered counter\n";<br>
return;<br>
}<br>
-<br>
- auto Res = Counters.insert({CounterID, {0, -1}});<br>
- Res.first->second.second = CounterVal;<br>
+ Counters[CounterID].StopAfter = CounterVal;<br>
+ Counters[CounterID].IsSet = true;<br>
} else {<br>
errs() << "DebugCounter Error: " << CounterPair.first<br>
<< " does not end with -skip or -count\n";<br>
@@ -106,7 +104,8 @@ void DebugCounter::print(raw_ostream &OS<br>
OS << "Counters and values:\n";<br>
for (const auto &KV : Counters)<br>
OS << left_justify(RegisteredCounters[KV.first], 32) << ": {"<br>
- << KV.second.first << "," << KV.second.second << "}\n";<br>
+ << KV.second.Count << "," << KV.second.Skip << ","<br>
+ << KV.second.StopAfter << "}\n";<br>
}<br>
<br>
LLVM_DUMP_METHOD void DebugCounter::dump() const {<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp?rev=337748&r1=337747&r2=337748&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp?rev=337748&r1=337747&r2=337748&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp Mon Jul 23 14:49:36 2018<br>
@@ -861,7 +861,7 @@ private:<br>
<br>
// Debug counter info. When verifying, we have to reset the value numbering<br>
// debug counter to the same state it started in to get the same results.<br>
- std::pair<int, int> StartingVNCounter;<br>
+ int64_t StartingVNCounter;<br>
};<br>
<br>
} // end anonymous namespace<br>
<br>
Modified: llvm/trunk/unittests/Support/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CMakeLists.txt?rev=337748&r1=337747&r2=337748&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CMakeLists.txt?rev=337748&r1=337747&r2=337748&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Support/CMakeLists.txt (original)<br>
+++ llvm/trunk/unittests/Support/CMakeLists.txt Mon Jul 23 14:49:36 2018<br>
@@ -20,6 +20,7 @@ add_llvm_unittest(SupportTests<br>
ConvertUTFTest.cpp<br>
DataExtractorTest.cpp<br>
DebugTest.cpp<br>
+ DebugCounterTest.cpp<br>
DJBTest.cpp<br>
EndianStreamTest.cpp<br>
EndianTest.cpp<br>
<br>
Added: llvm/trunk/unittests/Support/DebugCounterTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/DebugCounterTest.cpp?rev=337748&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/DebugCounterTest.cpp?rev=337748&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Support/DebugCounterTest.cpp (added)<br>
+++ llvm/trunk/unittests/Support/DebugCounterTest.cpp Mon Jul 23 14:49:36 2018<br>
@@ -0,0 +1,42 @@<br>
+//===- llvm/unittest/Support/DebugCounterTest.cpp -------------------------===//<br>
+//<br>
+// The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+#include "llvm/Support/DebugCounter.h"<br>
+#include "gtest/gtest.h"<br>
+<br>
+#include <string><br>
+using namespace llvm;<br>
+<br>
+#ifndef NDEBUG<br>
+DEBUG_COUNTER(TestCounter, "test-counter",<br>
+ "Counter used for unit test");<br>
+<br>
+TEST(DebugCounterTest, CounterCheck) {<br>
+ EXPECT_FALSE(DebugCounter::isCounterSet(TestCounter));<br>
+<br>
+ auto DC = &DebugCounter::instance();<br>
+ DC->push_back("test-counter-skip=1");<br>
+ DC->push_back("test-counter-count=3");<br>
+<br>
+ EXPECT_TRUE(DebugCounter::isCounterSet(TestCounter));<br>
+<br>
+ EXPECT_EQ(0, DebugCounter::getCounterValue(TestCounter));<br>
+ EXPECT_FALSE(DebugCounter::shouldExecute(TestCounter));<br>
+<br>
+ EXPECT_EQ(1, DebugCounter::getCounterValue(TestCounter));<br>
+ EXPECT_TRUE(DebugCounter::shouldExecute(TestCounter));<br>
+<br>
+ DebugCounter::setCounterValue(TestCounter, 3);<br>
+ EXPECT_TRUE(DebugCounter::shouldExecute(TestCounter));<br>
+ EXPECT_FALSE(DebugCounter::shouldExecute(TestCounter));<br>
+<br>
+ DebugCounter::setCounterValue(TestCounter, 100);<br>
+ EXPECT_FALSE(DebugCounter::shouldExecute(TestCounter));<br>
+}<br>
+#endif<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>