[PATCH] D43520: [ThinLTO] Represent relative BF using a scaled representation .

[Easwaran Raman via Phabricator via llvm-commits]

eraman marked 2 inline comments as done.
eraman added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:63
   uint32_t Hotness : 3;
   uint32_t RelBlockFreq : 29;
+  static constexpr int32_t ScaleShift = 8;
----------------
davidxl wrote:
> Document that it is RelativeBlockFrequency scaled by 2^8
> 
> Also should all the uses of RelBlockFreq scale it back?
Documented. 

Yes, users should scale it back. In a separate patch, I will add a method to get this value as a ScaledNumber or uint64_t so that the users don't have to explicitly scale them


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:65
+  static constexpr int32_t ScaleShift = 8;
   static constexpr uint64_t MaxRelBlockFreq = (1 << 29) - 1;
 
----------------
davidxl wrote:
> Should this be scaled up too?
No, this is just the max value of the digits in the 29 bit bit-field. 


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:86
+    Scaled64 Temp(BlockFreq, ScaleShift);
+    Temp /= Scaled64::get(EntryFreq);
+
----------------
davidxl wrote:
> EntryFreq can be zero.
Somehow missed this in refactoring.


Repository:
  rL LLVM

https://reviews.llvm.org/D43520