Page MenuHomePhabricator

[ThinLTO] Represent relative BF using a scaled representation .
ClosedPublic

Authored by eraman on Feb 20 2018, 11:35 AM.

Details

Summary

The current integer representation of relative block frequency prevents
representing relative block frequencies below 1. This change uses a 8 of
the 29 bits to represent the decimal part by using a fixed scale of -8.

Diff Detail

Repository
rL LLVM

Event Timeline

eraman created this revision.Feb 20 2018, 11:35 AM
davidxl added inline comments.Feb 20 2018, 1:18 PM
include/llvm/IR/ModuleSummaryIndex.h
63 ↗(On Diff #135116)

Document that it is RelativeBlockFrequency scaled by 2^8

Also should all the uses of RelBlockFreq scale it back?

65 ↗(On Diff #135116)

Should this be scaled up too?

86 ↗(On Diff #135116)

EntryFreq can be zero.

eraman marked 2 inline comments as done.Feb 20 2018, 5:31 PM
eraman added inline comments.
include/llvm/IR/ModuleSummaryIndex.h
63 ↗(On Diff #135116)

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

65 ↗(On Diff #135116)

No, this is just the max value of the digits in the 29 bit bit-field.

86 ↗(On Diff #135116)

Somehow missed this in refactoring.

eraman updated this revision to Diff 135178.Feb 20 2018, 5:32 PM
eraman marked 2 inline comments as done.

Address review comments.

davidxl added inline comments.Feb 20 2018, 9:30 PM
include/llvm/IR/ModuleSummaryIndex.h
65 ↗(On Diff #135116)

then the 'min' function here Sum = std::min(Sum, uint64_t(MaxRelBlockFreq)); should use a scaled MaxRelBlockFreq, right?

eraman added inline comments.Feb 21 2018, 12:34 PM
include/llvm/IR/ModuleSummaryIndex.h
65 ↗(On Diff #135116)

No. The intent is to use a max of 29 bits including the integer and decimal(fractional) bits. Since I use a scale of 8, 8 bits will be used to represent values below 1 and I can use only 21 bits to represent the integral part.

This revision is now accepted and ready to land.Feb 21 2018, 12:45 PM
This revision was automatically updated to reflect the committed changes.