<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-27 16:07 GMT-07:00 Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><blockquote type="cite"><div class=""><div>On Jun 25, 2014, at 4:32 PM, Alex L <<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>> wrote:</div>
<br></div><div class=""><div><div dir="ltr">Hi everyone,<br>This is a first patch that implements the data structures, readers and writers used by the new code coverage system. I added the new code to the ProfileData library. I also added a very minimal C api for the ProfileData library.</div>
</div></div></blockquote><br></div><div>Review, part 2…. I’m skipping over the reader and writer code for now.</div><div><br></div><div><blockquote type="cite"><div>diff --git a/lib/ProfileData/CoverageMapping.cpp b/lib/ProfileData/CoverageMapping.cpp</div>
<div>new file mode 100644</div><div>index 0000000..33ea866</div><div>--- /dev/null</div><div>+++ b/lib/ProfileData/CoverageMapping.cpp</div><div>@@ -0,0 +1,175 @@</div><div>+//=-- CoverageMapping.cpp - Code coverage mapping support ---------*- C++ -*-=//</div>
<div class=""><div>+//</div><div>+// The LLVM Compiler Infrastructure</div><div>+//</div><div>+// This file is distributed under the University of Illinois Open Source</div><div>+// License. See LICENSE.TXT for details.</div>
<div>+//</div><div>+//===----------------------------------------------------------------------===//</div><div>+//</div></div><div>+// This file contains support for clang's and llvm's instrumentation based</div><div>
+// code coverage.</div><div>+//</div><div>+//===----------------------------------------------------------------------===//</div><div>+</div><div>+#include "llvm/ProfileData/CoverageMapping.h"</div><div>+</div>
<div>+using namespace llvm;</div><div>+</div><div>+// Encode the counter in the following way:</div><div>+// Low 2 bits - tag/counter kind.</div><div>+// Remaining bits - Counter/Expression ID.</div><div>+unsigned Counter::Encode() const {</div>
<div>+ assert(ID <= (std::numeric_limits<unsigned>::max() >> EncodingTagBits));</div><div>+</div><div>+ return unsigned(Kind) | (ID << EncodingTagBits);</div><div>+}</div><div>+</div><div>+bool Counter::Decode(unsigned Value) {</div>
<div>+ Kind = CounterKind(Value & EncodingTagMask);</div><div>+ ID = Value >> EncodingTagBits;</div><div>+ return false;</div><div>+}</div><div>+</div><div>+CounterExpressionBuilder::CounterExpressionBuilder(unsigned NumCounters) {</div>
<div>+ Terms.resize(NumCounters);</div><div>+}</div></blockquote><div><br></div>It looks like NumCounters here refers to the number of counts from the profile data and does not include all of the “abstract Counters”. In the CounterKind enum, you refer to the former as “CounterValueReference”. Perhaps the argument here should be named “NumCounterValues” to help make that distinction more clear?</div>
</div></blockquote><div><br></div><div>Yes, it does refer to the number of counts from profile data. This rename here is a good idea.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><br><blockquote type="cite"><div>+</div><div>+Counter CounterExpressionBuilder::get(Counter::CounterKind Kind,</div><div>+ const CounterExpression &E) {</div>
<div>+ for (unsigned I = 0, S = Expressions.size(); I < S; ++I) {</div><div>+ if (Expressions[I] == E)</div><div>+ return Counter::getExpression(Kind, I);</div><div>+ }</div><div>+ Expressions.push_back(E);</div>
<div>+ return Counter::getExpression(Kind, Expressions.size() - 1);</div><div>+}</div><div>+</div><div>+void CounterExpressionBuilder::ExtractTerms(Counter C, int Sign) {</div><div>+ switch (C.getKind()) {</div><div>+ case Counter::Zero:</div>
<div>+ break;</div><div>+ case Counter::CounterValueReference:</div><div>+ Terms[C.getCounterID()] += Sign;</div><div>+ break;</div><div>+ case Counter::AdditionExpression:</div><div>+ ExtractTerms(Expressions[C.getExpressionID()].LHS, Sign);</div>
<div>+ ExtractTerms(Expressions[C.getExpressionID()].RHS, Sign);</div><div>+ break;</div><div>+ case Counter::SubtractionExpression:</div><div>+ ExtractTerms(Expressions[C.getExpressionID()].LHS, Sign);</div><div>
+ ExtractTerms(Expressions[C.getExpressionID()].RHS, Sign == 1 ? -1 : 1);</div></blockquote><div><br></div>How about just “-Sign”?</div><div><br><blockquote type="cite"><div>+ break;</div><div>+ }</div><div>+}</div>
<div>+</div><div>+Counter CounterExpressionBuilder::Simplify(Counter ExpressionTree) {</div><div>+ // Gather constant terms</div><div>+ for (auto &I : Terms)</div><div>+ I = 0;</div><div>+ ExtractTerms(ExpressionTree);</div>
<div>+</div><div>+ Counter C;</div><div>+ // Create additions</div><div>+ // Note: the additions are created first</div><div>+ // to avoid creation of a tree like ((0 - X) + Y)</div><div>+ // instead of (Y - X).</div>
<div>+ for (unsigned I = 0, S = Terms.size(); I < S; ++I) {</div><div>+ if (Terms[I] <= 0)</div><div>+ continue;</div><div>+ for (int J = 0; J < Terms[I]; ++J) {</div><div>+ if (C.isZero())</div>
<div>+ C = Counter::getCounter(I);</div><div>+ else</div><div>+ C = get(Counter::AdditionExpression,</div><div>+ CounterExpression(CounterExpression::Add, C,</div><div>+ Counter::getCounter(I)));</div>
<div>+ }</div><div>+ }</div><div>+</div><div>+ // Create subtractions</div><div>+ for (unsigned I = 0, S = Terms.size(); I < S; ++I) {</div><div>+ if (Terms[I] >= 0)</div><div>+ continue;</div><div>+ for (int J = 0; J < (-Terms[I]); ++J)</div>
<div>+ C = get(Counter::SubtractionExpression,</div><div>+ CounterExpression(CounterExpression::Subtract, C,</div><div>+ Counter::getCounter(I)));</div><div>+ }</div><div>
+ return C;</div><div>+}</div><div>+</div><div>+Counter CounterExpressionBuilder::Add(Counter LHS, Counter RHS) {</div><div>+ return Simplify(get(Counter::AdditionExpression,</div><div>+ CounterExpression(CounterExpression::Add, LHS, RHS)));</div>
<div>+}</div><div>+</div><div>+Counter CounterExpressionBuilder::Subtract(Counter LHS, Counter RHS) {</div><div>+ return Simplify(</div><div>+ get(Counter::SubtractionExpression,</div><div>+ CounterExpression(CounterExpression::Subtract, LHS, RHS)));</div>
<div>+}</div></blockquote><div><br></div>You’re using the same set of Expressions in the CounterExpressionBuilder both before and after the simplification. That will leave some unused entries. I see that you have code in the writer to only emit the ones that are used in the end result, but I’m curious if you’ve measured how often you get unused entries.</div>
<div><br></div><div>Have you done anything to evaluate the effectiveness of your simplification code? Did you try anything different before settling on the current approach? How much does the Simplify function reduce the number of counter expressions?<br>
</div><div><blockquote type="cite"><div></div></blockquote></div></div></blockquote><div><br></div><div>Yes, there are unused expressions stored in the builder, but the writer writes out only the ones that are used.<br></div>
<div>This simplification code brought a lot of improvement when compared to the old method. It managed to get rid of expression trees like ((X - Y) + Y), etc. It also reduced the number of emitted mapping regions in some specific test cases, because now those regions had the same counters (they where found to be the same because they contained the same simplified expression).<br>
</div><div>The total reduction of expressions for a function with this simplification method and the writer only writing the used expressions is very significant - one test case went from 60 something to around 3 or 4 serialized expressions.<br>
The unused entries make up the majority of expressions in the builder, so the additional filtering code in the writer is necessary.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><blockquote type="cite"><div>+</div><div>+void CounterMappingContext::dump(const Counter &C,</div><div class=""><div>+ llvm::raw_ostream &OS) const {</div>
</div><div>+ switch (C.getKind()) {</div><div>+ default:</div></blockquote><div><br></div>Shouldn’t this be “case Counter::Zero”?<br></div></div></blockquote><div><br></div><div>Thanks, It should, the default case was from a very early version when I had multiple regions types with zero counters.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div> </div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><br><blockquote type="cite"><div>+ OS << '0';</div><div>+ return;</div><div>+ case Counter::CounterValueReference:</div><div>+ OS << '#' << C.getCounterID();</div>
<div>+ break;</div><div>+ case Counter::SubtractionExpression:</div><div>+ case Counter::AdditionExpression: {</div><div>+ if (C.getExpressionID() >= Expressions.size())</div><div>+ return;</div><div>+ const auto &E = Expressions[C.getExpressionID()];</div>
<div>+ OS << '(';</div><div>+ dump(E.LHS);</div><div>+ OS << (C.getKind() == Counter::SubtractionExpression ? " - " : " + ");</div><div>+ dump(E.RHS);</div><div>+ OS << ')';</div>
<div>+ break;</div><div>+ }</div><div>+ }</div><div>+ if (CounterValues.empty())</div><div>+ return;</div><div>+ std::error_code Error;</div><div>+ auto Value = evaluate(C, Error);</div><div>+ if (Error)</div>
<div>+ return;</div><div>+ OS << '[' << Value << ']';</div><div>+}</div><div>+</div><div>+int64_t CounterMappingContext::evaluate(const Counter &C,</div><div>+ std::error_code *EC) const {</div>
<div>+ switch (C.getKind()) {</div><div>+ case Counter::Zero:</div><div>+ return 0;</div><div>+ case Counter::CounterValueReference:</div><div>+ if (C.getCounterID() >= CounterValues.size()) {</div><div>+ if (EC)</div>
<div>+ *EC = std::make_error_code(std::errc::argument_out_of_domain);</div><div>+ break;</div><div>+ }</div><div>+ return CounterValues[C.getCounterID()];</div><div>+ case Counter::SubtractionExpression:</div>
<div>+ case Counter::AdditionExpression: {</div><div>+ if (C.getExpressionID() >= Expressions.size()) {</div><div>+ if (EC)</div><div>+ *EC = std::make_error_code(std::errc::argument_out_of_domain);</div>
<div>+ break;</div><div>+ }</div><div>+ const auto &E = Expressions[C.getExpressionID()];</div><div>+ auto LHS = evaluate(E.LHS, EC);</div><div>+ if (EC && *EC)</div><div>+ return 0;</div>
<div>+ auto RHS = evaluate(E.RHS, EC);</div><div>+ if (EC && *EC)</div><div>+ return 0;</div><div>+ return C.getKind() == Counter::SubtractionExpression ? LHS - RHS</div><div>+ : LHS + RHS;</div>
<div>+ }</div><div>+ }</div><div>+ return 0;</div><div>+}</div></blockquote><div><br></div>Responding to your email about the header file review:</div><div><div class=""><blockquote type="cite"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
Ditto for the CounterMappingContext. Let’s come back to that later.<br></blockquote><div><br></div><div>Okay. However, it's used in the implementation of the C api in this library.</div></blockquote><div><br></div></div>
Good point. I’m not strongly opposed to keeping this here for now.</div><div><div><div><br></div></div></div></div></blockquote></div><br></div></div>