[PATCH] D148876: [IndirectCallPromotion] Clear value profile metadata after the last run of indirect-call-promotion in a pipeline

Mon Apr 24 13:26:23 PDT 2023

hoy added a comment.

In D148876#4293319 <https://reviews.llvm.org/D148876#4293319>, @xur wrote:

> In D148876#4289423 <https://reviews.llvm.org/D148876#4289423>, @davidxl wrote:
>
>> In D148876#4289326 <https://reviews.llvm.org/D148876#4289326>, @hoy wrote:
>>
>>> Thanks for the illustration. For AutoFDO, I took a look at the debug info merger (https://github.com/llvm/llvm-project/blob/a17b71d17f853350dcd6c72ab141b196d7caec2a/llvm/lib/IR/DebugInfoMetadata.cpp#L114). Here is my understanding:
>>>
>>> 1. For two callsites that are originally from different lines of a function, the merged location will have a zero line number. This basically means the value profile for the merged callsite will be dropped by the sample loader.
>>
>> Thanks for checking. I suspect bugs like this may exist, but it is orthogonal to the problem at hand.
>
> I don't think this is a bug. This is the intended behavior -- if there is conflict debug info, just set to zero line number of remove the debug info.
> This does create some problems for Sample FDO.
>
> This is not a problem for pseudo-probe, right?

Unfortunately it somehow still affects pseudo probe which uses `!dbg` info for callsites to store their probe information.

>>> 2. For callsites originally from same location but ended up with different inline contexts in the caller function, the merger tries to reconcile their inline contexts by finding a common suffix of the contexts. If successful, the common suffix will be the debug location for the merged callsites. This may allow for the value profile to be shared by the two callsites (under a different inlining situation). E.g, for a callsite defined in function `D` at line 4, if somehow it gets inlined twice into function `A`:
>>>
>>>   A:1 @ B:2 @ C:3 @ D:4
>>>   
>>>   A:2 @ B:3 @ C:3 @ D:4
>>>
>>> Then my understanding is that the merged callsite will have ` C:3 @ D:4` as its debug string. If somehow in the next compilation `A` never inlines `B` but `B` inlines `C` twice, then the two callsites could end up sharing the same value profile.
>>
>> True,  but this  happens today as well -- it is very unlikely such two callsites from inline instances can be merged in the first place.
>>
>>> #2 may sound uncommon,
>>
>> yes.  Besides sharing profile may still be better than dropping profile.
>>
>>> but #1 should be common. If that's the case, then we basically lose track of value profiles for merged callsites.
>>
>>> Please let me know if I miss anything.
>>
>> See my reply above.
>>
>>> Despite AutoFDO, I think Mingming's solution that allows merging only after ICP should be good for Instrument PGO.
>
> This does not make AutoFDO worse, right?
> This seems also to benefit pseudo-probe based AutoFDO.

Right, it doesn't make AutoFDO worse. It should help pseudo probe in some cases where merging biases one callsite.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148876