<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><blockquote type="cite"><div>On Jun 25, 2014, at 4:32 PM, Alex L <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><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></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>+//</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>+// 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><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?</div><div><br></div><div><blockquote type="cite"><div>+</div><div>+void CounterMappingContext::dump(const Counter &C,</div><div>+ llvm::raw_ostream &OS) const {</div><div>+ switch (C.getKind()) {</div><div>+ default:</div></blockquote><div><br></div>Shouldn’t this be “case Counter::Zero”?</div><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><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>Good point. I’m not strongly opposed to keeping this here for now.</div><div><div><div><br></div></div></div></body></html>