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

Fri Apr 21 22:19:39 PDT 2023

davidxl added a comment.

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.

> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148876