[PATCH] D147545: [CSSPGO] Stale profile matching(part 2)

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D147545#4255880 <https://reviews.llvm.org/D147545#4255880>, @davidxl wrote:

> In D147545#4255847 <https://reviews.llvm.org/D147545#4255847>, @hoy wrote:
>
>> In D147545#4252021 <https://reviews.llvm.org/D147545#4252021>, @davidxl wrote:
>>
>>> consider this scenario:
>>>
>>> {
>>>
>>>   S1
>>>   foo();
>>>   S2
>>>
>>> }
>>>
>>> {
>>>
>>>   S3
>>>   bar();
>>>   S2
>>>
>>> }
>>>
>>> The first block including foo() call is shifted in source code, but the bar() block does not.
>>>
>>> With this remapping algorithm, A won't find sample, bar() and S2 is fine; but C's location will be wrongly remapped.
>>>
>>> I wonder if there is a need to do two way anchoring to handle this.
>>
>> Can you please point out what `A` and `C` are? Do they refer to `S1` and `S3`, respectively?
>
> Right -- sorry about the confusion.
>
>> I think the intuition of the current matching algorithm focuses on getting the callsites right as the first step, as we found out that brining back missing inlining (due to profile mismatch) is quite helpful. Getting the block weights right is also important, and there will be upcoming improvements on a better numbering of callsite ids and probe ids (mostly for CSSPGO). That said, the current heuristics is never perfect as it does not try to reason about user semantics changes. Our observation is that for many cases users don't change hot function calls often, so we currently rely on the calls as anchors.
>
> Understand. I also believe this patch works for most of the cases. The thing that remains concerning is if there are cases that applying the source remapping lead to worse performance. For instances, statements in S3 may get wrong profile data after remapping.

That's a valid concern. I think yes, there will be blocks getting wrong profile data if no call anchors are matched or the user changes the semantics that makes our assumption completed invalid. For non-CS AutoFDO, I think something similar is already happening as the current line-offset based profile loading has no knowledge about how source semantics is changed. For CSSPGO, the wrong profile data may not be completely bad since at least the function would not be considered as cold as it was previously, because of the mismatch that caused the profile completely dropped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147545