<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Sep 8, 2015 at 2:42 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><br>
> On 2015-Sep-08, at 10:53, Cong Hou <<a href="mailto:congh@google.com">congh@google.com</a>> wrote:<br>
><br>
> On Tue, Sep 8, 2015 at 9:39 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
><br>
> > On 2015-Sep-05, at 22:41, David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>> wrote:<br>
> ><br>
> ><br>
> > ================<br>
> > Comment at: test/Analysis/BlockFrequencyInfo/basic.ll:71<br>
> > @@ -70,3 +70,3 @@<br>
> > ; The 'case_c' branch is predicted more likely via branch weight metadata.<br>
> > -; CHECK-NEXT: case_c: float = 0.8,<br>
> > +; CHECK-NEXT: case_c: float = 0.8<br>
> > case_c:<br>
> > ----------------<br>
> > where does the coma come from?<br>
><br>
> The comma was there to confirm that we get *exactly* 0.8. Why has<br>
> that changed?<br>
><br>
> With fixed point representation, we won't get exactly 0.8 here but 0.8000000001. Should I restrict the output precision when printing this value, or use regex or exact 0.8000000001 in the test case?<br>
<br>
</span>I don't like that we don't get exactly 0.8 here, and frankly I don't<br>
quite understand it. It's certainly possible to get exactly 0.8:<br>
--<br>
$ bc<br>
scale=100<br>
<a href="tel:3435973836" value="+13435973836">3435973836</a>/4294967295<br>
.8000000000000000000000000000000000000000000000000000000000000000000\<br>
000000000000000000000000000000000<br>
--<br>
<br>
What's going wrong?<br></blockquote><div><br></div><div>Thank you very much for the review, Duncan!</div><div><br></div><div>David pointed out that there is a rounding issue when calculating the numerator. Instead of </div><div><br></div><div> uint64_t Prob64 = (n * static_cast<uint64_t>(D)) / d;</div><div><br></div><div>I should use </div><div><br></div><div><div> uint64_t Prob64 = (n * static_cast<uint64_t>(D) + d / 2) / d;</div></div><div><br></div><div>To get the numerator. With this issue fixed, we still could not get exact 0.8. I think this is because that the frequency is calculated from branch probability and the precision loss is accumulated and enlarged. (here 0.8 is not branch probability but block frequency represented by a scaled number). Another possibility is that some other probability could not be expressed as precisely as 0.8: 0.7 is such an example:</div><div><br></div><div><div>>>> 3006477106/4294967295.</div><div>0.6999999998835846</div><div>>>> 3006477107/4294967295.</div><div>0.7000000001164153</div></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
(If it really is unavoidable, I think restricting output precision makes<br>
the most sense, so we get nice predictable output. But I want to be sure<br>
it's actually necessary, first.)<br></blockquote><div><br></div><div>To get 0.8 I need to adjust the precision when outputting the scaled number that represents the block frequency. The default precision is 10, and after changing it to a smaller number we will get exact 0.8 here (otherwise 0.8000000001, which exact has 10 digits after decimal).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
I have a few more comments on the patch below.<br>
<br>
> Index: test/Analysis/BlockFrequencyInfo/bad_input.ll<br>
> ===================================================================<br>
> --- test/Analysis/BlockFrequencyInfo/bad_input.ll<br>
> +++ test/Analysis/BlockFrequencyInfo/bad_input.ll<br>
> @@ -10,7 +10,7 @@<br>
> br label %for.body<br>
><br>
> ; Check that we get 1,4 instead of 0,3.<br>
> -; CHECK-NEXT: for.body: float = 4.0,<br>
> +; CHECK-NEXT: for.body: float = 3.99<br>
> for.body:<br>
> %i = phi i32 [ 0, %entry ], [ %inc, %for.body ]<br>
> call void @g(i32 %i)<br>
> @@ -43,7 +43,7 @@<br>
><br>
> ; Check that the exit weight is half of entry, since half is lost in the<br>
> ; infinite loop above.<br>
> -; CHECK-NEXT: for.end: float = 0.5,<br>
> +; CHECK-NEXT: for.end: float = 0.49<br>
<br>
All these testcases being slightly off is suspicous to me. 32 bits<br>
ought to be enough to represent these closely.<br></blockquote><div><br></div><div>With the rounding issue fixed we won't get such problems anymore (but I still need to restrict the output precision of the scaled number).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> for.end:<br>
> ret void<br>
> }<br>
> Index: test/Transforms/SampleProfile/propagate.ll<br>
> ===================================================================<br>
> --- test/Transforms/SampleProfile/propagate.ll<br>
> +++ test/Transforms/SampleProfile/propagate.ll<br>
> @@ -97,8 +97,8 @@<br>
> %div4 = sdiv i64 %10, 4, !dbg !22<br>
> %cmp5 = icmp sgt i64 %9, %div4, !dbg !22<br>
> br i1 %cmp5, label %if.then6, label %if.else7, !dbg !22<br>
> -; CHECK: edge if.end -> if.then6 probability is 3 / 6342 = 0.0473037%<br>
> -; CHECK: edge if.end -> if.else7 probability is 6339 / 6342 = 99.9527% [HOT edge]<br>
> +; CHECK: edge if.end -> if.then6 probability is 0%<br>
<br>
This is a fairly lossy change. We've lost all the significant digits<br>
of 0.0473037%, and made it look like 1:0. I hope that's unnecessary.<br></blockquote><div><br></div><div>This is because previously I only output the rounded integer as the percentage. In the updated patch, I have changed it to output as %.2f. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> +; CHECK: edge if.end -> if.else7 probability is 100%<br>
><br>
> if.then6: ; preds = %if.end<br>
> %11 = load i32, i32* %y.addr, align 4, !dbg !24<br>
> Index: lib/Support/BranchProbability.cpp<br>
> ===================================================================<br>
> --- lib/Support/BranchProbability.cpp<br>
> +++ lib/Support/BranchProbability.cpp<br>
> @@ -15,16 +15,42 @@<br>
> #include "llvm/Support/Debug.h"<br>
> #include "llvm/Support/Format.h"<br>
> #include "llvm/Support/raw_ostream.h"<br>
> +#include <cassert><br>
><br>
> using namespace llvm;<br>
><br>
> raw_ostream &BranchProbability::print(raw_ostream &OS) const {<br>
> - return OS << N << " / " << D << " = "<br>
<br>
This was nice because it gave us something exact for debugging.<br>
<br>
I suggest you maintain that, by printing `N` in hexidecimal with padding<br>
(so that things line up nicely):<br>
--<br>
0x00000000<br>
0x023ad039<br>
0xffffffff<br>
--<br>
This is similar to how BlockMass is printed.<br></blockquote><div><br></div><div>This is a nice suggestion. Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> - << format("%g%%", ((double)N / D) * 100.0);<br>
> + return OS << format("%d%%",<br>
> + static_cast<int>(std::round(((double)N / D) * 100.0)));<br>
<br>
Why change the way these are rounded and printed?<br></blockquote><div><br></div><div>This is changed to output as %.2f now (otherwise we may get strange result like 9.1242e-10%).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> }<br>
><br>
> void BranchProbability::dump() const { print(dbgs()) << '\n'; }<br>
><br>
> +BranchProbability::BranchProbability(uint32_t n, uint32_t d) {<br>
> + assert(d > 0 && "Denominator cannot be 0!");<br>
> + assert(n <= d && "Probability cannot be bigger than 1!");<br>
> + if (d == D)<br>
> + N = n;<br>
<span class="">> + else {<br>
> + uint64_t Prob64 = n * static_cast<uint64_t>(D) / d;<br>
> + N = static_cast<uint32_t>(Prob64);<br>
<br>
</span>It doesn't look like this is rounding correctly. Can you add some<br>
unit tests?<br></blockquote><div><br></div><div>This is fixed as stated above. The patch already failed many branch probability unit tests. I will fix them soon with a patch update. Those unit tests should cover the correctness of this constructor.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> + }<br>
> +}<br>
> +<br>
> +BranchProbability &BranchProbability::operator+=(BranchProbability RHS) {<br>
> + assert(N <= D - RHS.N &&<br>
> + "The sum of branch probabilities should not exceed one!");<br>
> + N += RHS.N;<br>
> + return *this;<br>
> +}<br>
> +<br>
> +BranchProbability &BranchProbability::operator-=(BranchProbability RHS) {<br>
> + assert(N >= RHS.N &&<br>
> + "Can only subtract a smaller probability from a larger one!");<br>
> + N -= RHS.N;<br>
> + return *this;<br>
> +}<br>
> +<br>
> static uint64_t scale(uint64_t Num, uint32_t N, uint32_t D) {<br>
> assert(D && "divide by 0");<br>
><br>
> Index: include/llvm/Support/BranchProbability.h<br>
> ===================================================================<br>
> --- include/llvm/Support/BranchProbability.h<br>
> +++ include/llvm/Support/BranchProbability.h<br>
> @@ -15,36 +15,44 @@<br>
> #define LLVM_SUPPORT_BRANCHPROBABILITY_H<br>
><br>
> #include "llvm/Support/DataTypes.h"<br>
> -#include <cassert><br>
><br>
> namespace llvm {<br>
><br>
> class raw_ostream;<br>
><br>
> -// This class represents Branch Probability as a non-negative fraction.<br>
> +// This class represents Branch Probability as a non-negative fraction that is<br>
> +// no greater than 1. It uses a fixed-point-like implementation, in which the<br>
> +// denominator is always a constant value (here we use UINT32_MAX for maximum<br>
> +// precision).<br>
> class BranchProbability {<br>
> // Numerator<br>
> uint32_t N;<br>
><br>
> - // Denominator<br>
> - uint32_t D;<br>
> + // Denominator, which is a constant value.<br>
> + static const uint32_t D = UINT32_MAX;<br>
> +<br>
> + // Construct a BranchProbability with only numerator assuming the denominator<br>
> + // is UINT32_MAX. For internal use only.<br>
> + explicit BranchProbability(uint32_t n) : N(n) {}<br>
<br>
It seems nice to expose this functionality publicly without having to<br>
pass in `UINT32_MAX` to the denominator.<br>
--<br>
static BranchProbability getRaw(uint32_t N)<br>
--<br>
<br>
Or maybe you can come up with a better name than `getRaw()`.<br>
<br></blockquote><div><br></div><div>I have added this public static function. I could not think out a better name so let's use getRaw() now. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
><br>
> public:<br>
> - BranchProbability(uint32_t n, uint32_t d) : N(n), D(d) {<br>
> - assert(d > 0 && "Denominator cannot be 0!");<br>
> - assert(n <= d && "Probability cannot be bigger than 1!");<br>
> - }<br>
> + BranchProbability() : N(0) {}<br>
> + BranchProbability(uint32_t n, uint32_t d);<br>
<br>
Can you fix the style here in a separate NFC commit? The parameters<br>
should really be 'N' and 'D'.<br></blockquote><div><br></div><div>OK. As N and D are used as data members, I need to use longer names like Num and Denom.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> +<br>
> + bool isZero() const { return N == 0; }<br>
><br>
> - static BranchProbability getZero() { return BranchProbability(0, 1); }<br>
> - static BranchProbability getOne() { return BranchProbability(1, 1); }<br>
> + static BranchProbability getZero() { return BranchProbability(0); }<br>
> + static BranchProbability getOne() { return BranchProbability(D); }<br>
> +<br>
> + // Normalize given probabilties so that the sum of them becomes one.<br>
<span class="">> + template <class ProbabilityList><br>
> + static void normalizeProbabilities(ProbabilityList &Probs);<br>
><br>
</span><span class="">> uint32_t getNumerator() const { return N; }<br>
> - uint32_t getDenominator() const { return D; }<br>
> + static uint32_t getDenominator() { return D; }<br>
><br>
</span>> // Return (1 - Probability).<br>
> - BranchProbability getCompl() const {<br>
> - return BranchProbability(D - N, D);<br>
> - }<br>
> + BranchProbability getCompl() const { return BranchProbability(D - N); }<br>
><br>
> raw_ostream &print(raw_ostream &OS) const;<br>
><br>
> @@ -66,24 +74,67 @@<br>
> /// \return \c Num divided by \c this.<br>
> uint64_t scaleByInverse(uint64_t Num) const;<br>
><br>
> - bool operator==(BranchProbability RHS) const {<br>
> - return (uint64_t)N * RHS.D == (uint64_t)D * RHS.N;<br>
> + BranchProbability &operator+=(BranchProbability RHS);<br>
> + BranchProbability &operator-=(BranchProbability RHS);<br>
> + BranchProbability &operator*=(BranchProbability RHS) {<br>
> + N = static_cast<uint64_t>(N) * RHS.N / D;<br>
> + return *this;<br>
> }<br>
> - bool operator!=(BranchProbability RHS) const {<br>
> - return !(*this == RHS);<br>
> +<br>
> + BranchProbability operator+(BranchProbability RHS) const {<br>
> + BranchProbability Prob(*this);<br>
> + return Prob += RHS;<br>
> }<br>
> - bool operator<(BranchProbability RHS) const {<br>
> - return (uint64_t)N * RHS.D < (uint64_t)D * RHS.N;<br>
> +<br>
> + BranchProbability operator-(BranchProbability RHS) const {<br>
> + BranchProbability Prob(*this);<br>
> + return Prob -= RHS;<br>
> }<br>
> +<br>
> + BranchProbability operator*(BranchProbability RHS) const {<br>
> + BranchProbability Prob(*this);<br>
> + return Prob *= RHS;<br>
> + }<br>
> +<br>
> + bool operator==(BranchProbability RHS) const { return N == RHS.N; }<br>
> + bool operator!=(BranchProbability RHS) const { return !(*this == RHS); }<br>
> + bool operator<(BranchProbability RHS) const { return N < RHS.N; }<br>
> bool operator>(BranchProbability RHS) const { return RHS < *this; }<br>
> bool operator<=(BranchProbability RHS) const { return !(RHS < *this); }<br>
> bool operator>=(BranchProbability RHS) const { return !(*this < RHS); }<br>
> +<br>
> + friend BranchProbability operator*(BranchProbability LHS, uint32_t RHS);<br>
> + friend BranchProbability operator/(BranchProbability LHS, uint32_t RHS);<br>
<br>
Why do these need to be friends? Can't they reach in and access the<br>
numerator via `getNumerator()`?<br></blockquote><div><br></div><div>Friends are removed now and getRaw() is used to construct the new object. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> };<br>
><br>
> -inline raw_ostream &operator<<(raw_ostream &OS, const BranchProbability &Prob) {<br>
> +inline raw_ostream &operator<<(raw_ostream &OS, BranchProbability Prob) {<br>
<br>
Why change this? If you really prefer passing by-value, please do it in<br>
a separate NFC commit so it doesn't add noise to this patch.<br>
<br>
> return Prob.print(OS);<br>
> }<br>
><br>
> +inline BranchProbability operator*(BranchProbability LHS, uint32_t RHS) {<br>
> + LHS.N *= RHS;<br>
<br>
Why do we need these? They seem error-prone. It's also new API we<br>
didn't need before, so I'm a little skeptical...<br></blockquote><div><br></div><div>Those are needed when I changed weights to probabilities in MBB. But at this moment there is no use case at all so I can remove them now and add them later. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
What's the story for overflow? I imagine we'd want saturation<br>
behaviour, or, possibly more robust, an assertion.<br></blockquote><div><br></div><div>Yes, I think saturation is a proper behavior.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> + return LHS;<br>
> +}<br>
> +<br>
> +inline BranchProbability operator/(BranchProbability LHS, uint32_t RHS) {<br>
> + LHS.N /= RHS;<br>
<br>
Assertion about 0? Rounding? (Do we even need this?)<br></blockquote><div><br></div><div>Yes. Rounding is necessary here. As this is not used so I have removed this function and will add it back later.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> + return LHS;<br>
> +}<br>
> +<br>
> +inline BranchProbability operator*(uint32_t LHS, BranchProbability RHS) {<br>
> + return RHS * LHS;<br>
> +}<br>
> +<br>
> +template <class ProbabilityList><br>
> +void BranchProbability::normalizeProbabilities(ProbabilityList &Probs) {<br>
> + uint64_t Sum = 0;<br>
> + for (auto Prob : Probs)<br>
> + Sum += Prob.N;<br>
> + if (Sum > 0)<br>
<br>
Early return instead of branching?<br></blockquote><div><br></div><div>OK.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Actually, this looks like bad input if `Sum` is 0. Should this just<br>
assert?<br></blockquote><div><br></div><div>There are some cases in the follow-up patch (replacing weights with probabilities) in which the sum can be zero. I will look into it further but here let's add an assert.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> + for (auto &Prob : Probs)<br>
> + Prob.N = Prob.N * uint64_t(D) / Sum;<br>
<br>
I don't think these are guaranteed to equal `UINT32_MAX`, since there<br>
can be rounding errors. You need to dither somehow. Can you add some<br>
unit tests and fix the bugs?<br></blockquote><div><br></div><div>It seems it is not necessary to make the sum of resulted probabilities to exact one, which is difficult. I have update the comment of this function saying we are getting approximate one as the sum. </div><div><br></div><div>The rounding problem is fixed by changing it to Prob.N = (Prob.N * uint64_t(D) + Sum / 2) / Sum;</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> +}<br>
> +<br>
> }<br>
><br>
> #endif<br>
><br>
<br>
</blockquote></div><br></div></div>