[PATCH] D96806: [SampleFDO] Stop repeated indirect call promotion for the same target

Wed Feb 17 11:25:41 PST 2021

hoy added a comment.

In D96806#2567120 <https://reviews.llvm.org/D96806#2567120>, @wmi wrote:

> In D96806#2566946 <https://reviews.llvm.org/D96806#2566946>, @hoy wrote:
>
>> In D96806#2566458 <https://reviews.llvm.org/D96806#2566458>, @wmi wrote:
>>
>>> Currently to prevent recursive indirect call promotion, SampleLoaderPass use a DenseSet PromotedInsns to memorize which indirect call has been promoted inside of current function. However, PromotedInsns is cleared everytime emitAnnotation has done for a function, so it cannot prevent the same indirect call being promoted for the same target again in another iteration of emitAnnotation.
>>>
>>> On the other side, because of PromotedInsns, an indirect call is limited to be promoted only for one target in every iteration of emitAnnotation. That limits inline instance profile annotation related with indirect call targets. I think PromotedInsns can be deleted after this patch. I will address it in a followup patch after performance testing.
>>
>> Did you see the recursive indirect call promotion between functions that do not follow top-down order, like functions in an SCC?
>
> Right. If everything follow top-down order, the recursive indirect call issue will be mostly hidden. But currently even the top-down order flag is enabled by default, SCC and missing call graph edges from indirect call can lead to bottom-up inlining.
>
>> I remember this change https://reviews.llvm.org/D38763 prevents re-promotion done by subsequent PGO ICP. Could the existing heuristic in `inlineHotFunctions` be tweaked like PGO ICP?
>
> I think there is a major difference between SampleFDO ICP and regular ICP.
>
> Regular ICP uses the value profile metadata to decide which targets should be promoted. Once metadata is populated, the promotion candidates are mostly decided.
>
> SampleFDO ICP is different. The metadata is refreshed every time the indirect call is inlined into a new function and new inline instance profile is annotated to it. Even if the indirect call is promoted before, the metadata updated after the promotion will be dropped when the new profile is annotated. That is why I am trying to keep "0" counts for those targets which have already been promoted, and need to merge those "0" counts when we update the metadata with new profile -- if there is "0" count for a target, we will keep it as "0" during metadata merge. That will ensure the target won't be promoted again. I think we need this process no matter how we tweak existing heuristic like regular ICP.

I see. I remember that I had to use probe distribution factor to scale down the unpromoted indirect callsites.

>> BTW, multiple indirect call targets can be promoted by `inlineHotFunctionsWithPriority`.
>
> I was wrong that PromotedInsns prevent an indirect call from being promoted for multiple targets in every iteration of emitAnnotation. It could. I misread the code. PromotedInsns can still be useful as a cache so it doesn't need to access metadata every loop iteration in inlineHotFunctions, so we may want to keep it in the end. Sorry about it.

================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1194
+    auto Pair = ValueCountMap.try_emplace(Data.Value, Data.Count);
+    if (Pair.second)
+      continue;
----------------
Nit: group this if-statement and the one below?

================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1198
+    // If it is 0, the call target has been promoted so keep it as 0.
+    if (Pair.first->second != 0)
+      Pair.first->second = Data.Count;
----------------
Is a zero count automatically set when a target is promoted in a callee?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96806/new/

https://reviews.llvm.org/D96806