[PATCH] D42212: [ThinLTO] Add call edges' relative block frequency to per-module summary.

[Easwaran Raman via llvm-commits]

On Fri, Jan 19, 2018 at 4:23 PM, Teresa Johnson <tejohnson at google.com>
wrote:

>
>
> On Fri, Jan 19, 2018 at 4:15 PM, Easwaran Raman <eraman at google.com> wrote:
>
>>
>>
>> On Fri, Jan 19, 2018 at 4:08 PM, Teresa Johnson via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>>
>>> tejohnson added inline comments.
>>>
>>>
>>> ================
>>> Comment at: include/llvm/IR/ModuleSummaryIndex.h:58
>>>    HotnessType Hotness = HotnessType::Unknown;
>>> +  uint64_t RelBlockFreq = 0;
>>>
>>> ----------------
>>> eraman wrote:
>>> > tejohnson wrote:
>>> > > Do we need 64 bits in practice? Especially since these are scaled
>>> down by the entry freq. Concerned about the size increase for large indices
>>> that have huge numbers of edges. Also, since we'll never have both the
>>> Hotness and the RelBlockFreq, it would be great if we could union them,
>>> although that would require a flag on the index and I suppose we might want
>>> to support mixing modules built with and without profile data? Perhaps some
>>> other way to combine them. I.e. use a single field and reserve the lowest 3
>>> bits for the hotness, shifting the relative frequency into the upper n-3
>>> bits...
>>> > From our offline discussions I thought the use of 64 bits doesn't
>>> matter as they get compressed. Is the concern about in-memory size? I can
>>> reduce the size to, say, 29 bits (uint32_t with 3 bits reserved for the
>>> enum). Another thing to note is that only the per-module summary has this
>>> information.  Is there a way to make use of this?
>>> Right - concerned about in-memory size. Why does only the per-module
>>> summary contain this? Isn't that just temporary - I thought the idea was to
>>> propagate this into the backends?
>>>
>> We want to propagate the synthetic function entry counts to the backends.
>> This can be done by building a global graph whose edges are annotated with
>> the relative BFI and propagating the counts on this graph.
>>
>
> Can you clarify - to get them to the backends the updated propagated
> counts need to be written into the index (which is serialized out for
> distributed backends, and passed in-memory for in-process backends). So I
> would imagine this field would be used for that purpose?
>
Yes, the propagated function entry counts needs to be written into the
index, but that is  *per-function*  and not *per-edge*. On the other hand,
relative BFI is per call edge.

>
>
>>
>>>
>>>
>>> ================
>>> Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:63
>>> +cl::opt<bool> WriteRelBFToSummary(
>>> +    "write-relbf-to-summary", cl::Hidden, cl::init(false),
>>> +    cl::desc("Write relative block frequency to function summary "));
>>> ----------------
>>> eraman wrote:
>>> > tejohnson wrote:
>>> > > Suggest moving this to the bitcode writer and always adding
>>> RelBlockFreq into the in-memory index here.
>>> > >
>>> > > Is there any reason to default off? Why not just default on from the
>>> get-go?
>>> > Until this information actually gets used (first propagate the counts
>>> and then use the counts in the inliner), this just poses extra compile time
>>> and increased summary size if not turned off. Alternatively, I can drop
>>> this patch and then send it as part of a larger patch that builds the
>>> callgraph and propagates the counts.
>>> How long do you expect until the other patches to be sent? If not too
>>> long I don't know that it matters too much since we are going to incur the
>>> extra space/memory/time in the near term anyway. Better to have it tested.
>>>
>>>
>>> Repository:
>>>   rL LLVM
>>>
>>> https://reviews.llvm.org/D42212
>>>
>>>
>>>
>>>
>>
>
>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
> 408-460-2413 <(408)%20460-2413>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180119/08e91fb2/attachment-0001.html>