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

[David Li via llvm-commits]

davidxl added a comment.

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)


================
Comment at: include/llvm/Support/BranchProbability.h:49
@@ -40,1 +48,3 @@
+  template <class ProbabilityList>
+  static void normalizeProbabilities(ProbabilityList &Probs);
 
----------------
Do we really need this method? If update is done properly, this is not needed. Similar method can be defined for verification purpose.

================
Comment at: include/llvm/Support/BranchProbability.h:51
@@ -40,2 +50,3 @@
 
   uint32_t getNumerator() const { return N; }
+  static uint32_t getDenominator() { return D; }
----------------
Should be a private method.

================
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; }
 
----------------
Make this private.

================
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);
----------------
To make result with the right rounding, do this

Prob64 = n * (D + d/2) / d

================
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:
----------------
where does the coma come from?


http://reviews.llvm.org/D12603