[PATCH] D12603: Use fixed-point representation for BranchProbability

[Cong Hou via llvm-commits]

congh added a comment.

In http://reviews.llvm.org/D12603#240665, @davidxl wrote:

> For block frequency print, I think it might make sense to set a fixed precision in the print. For instance, when the value is less than 1, do the following to filter noise in testing:
>
> OS << format("%.2f", float_freq)
>
> otherwise
>
> OS << format("%.1f", flat_freq)


Do we really need this precision to represent subtle differences?


================
Comment at: include/llvm/Support/BranchProbability.h:49
@@ -40,1 +48,3 @@
+  template <class ProbabilityList>
+  static void normalizeProbabilities(ProbabilityList &Probs);
 
----------------
davidxl wrote:
> Do we really need this method? If update is done properly, this is not needed. Similar method can be defined for verification purpose.
This is not needed in this patch but will be needed when substituting weights with probabilities. This is a utility function that is used to normalize several probabilities assigned to all successors. I can remove it from this patch and add it later.

================
Comment at: include/llvm/Support/BranchProbability.h:51
@@ -40,2 +50,3 @@
 
   uint32_t getNumerator() const { return N; }
+  static uint32_t getDenominator() { return D; }
----------------
davidxl wrote:
> Should be a private method.
It will be used in branch probability unit test.

================
Comment at: include/llvm/Support/BranchProbability.h:52
@@ -41,3 +51,3 @@
   uint32_t getNumerator() const { return N; }
-  uint32_t getDenominator() const { return D; }
+  static uint32_t getDenominator() { return D; }
 
----------------
davidxl wrote:
> Make this private.
Ditto.

================
Comment at: lib/Support/BranchProbability.cpp:35
@@ +34,3 @@
+  else {
+    uint64_t Prob64 = n * static_cast<uint64_t>(D) / d;
+    N = static_cast<uint32_t>(Prob64);
----------------
davidxl wrote:
> To make result with the right rounding, do this
> 
> Prob64 = n * (D + d/2) / d
OK.

================
Comment at: test/Analysis/BlockFrequencyInfo/basic.ll:71
@@ -70,3 +70,3 @@
 ; The 'case_c' branch is predicted more likely via branch weight metadata.
-; CHECK-NEXT: case_c: float = 0.8,
+; CHECK-NEXT: case_c: float = 0.8
 case_c:
----------------
davidxl wrote:
> where does the coma come from?
With this patch, the result is not 0.8 but 0.8000000001. Removing the comma is a quick way to capture 0.8 but not those zeros behind it. Maybe I should use regex here?


http://reviews.llvm.org/D12603