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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

In D147545#4255941 <https://reviews.llvm.org/D147545#4255941>, @wlei wrote:

> In D147545#4255928 <https://reviews.llvm.org/D147545#4255928>, @wenlei wrote:
>
>>> I wonder if there is a need to do two way anchoring to handle this.
>>
>> @davidxl can you elaborate on two way anchoring? is that also consider location of non-call site as anchor?  It's trickier to use non-callsite as anchor since we would need a "signature" for block that is unique enough.
>>
>> Also since this is all in the territory of heuristic and tuning, maybe we can get the initial version in while keep improving it. We got to this version after fair amount of internal evaluation, and it showed good results already. We still plan to keep tuning it though it will take some time to experiment.
>
> I guess what @davidxl meant for two way matching is to spilt the non-anchor range. some matches are based on the start anchor, some are based on the end anchor.
>
>   foo()
>   inst1
>   inst2
>   inst3
>   inst4
>   bar()
>
> if foo is mismatched and bar is matched, the current version, all inst1-4 will be mismatched.
> and if matching in two way, say split evenly,  inst3, inst4 will be matched based on bar().

Ok, then for both heuristics, we can always have cases that yield correct matching with one heuristic, but wrong result with the other.

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

> In D147545#4255941 <https://reviews.llvm.org/D147545#4255941>, @wlei wrote:
>
>> In D147545#4255928 <https://reviews.llvm.org/D147545#4255928>, @wenlei wrote:
>>
>>>> I wonder if there is a need to do two way anchoring to handle this.
>>>
>>> @davidxl can you elaborate on two way anchoring? is that also consider location of non-call site as anchor?  It's trickier to use non-callsite as anchor since we would need a "signature" for block that is unique enough.
>>>
>>> Also since this is all in the territory of heuristic and tuning, maybe we can get the initial version in while keep improving it. We got to this version after fair amount of internal evaluation, and it showed good results already. We still plan to keep tuning it though it will take some time to experiment.
>>
>> I guess what @davidxl meant for two way matching is to spilt the non-anchor range. some matches are based on the start anchor, some are based on the end anchor.
>>
>>   foo()
>>   inst1
>>   inst2
>>   inst3
>>   inst4
>>   bar()
>>
>> if foo is mismatched and bar is matched, the current version, all inst1-4 will be mismatched.
>> and if matching in two way, say split evenly,  inst3, inst4 will be matched based on bar().
>
> This one way. Another way is to do backward scanning of the anchors and create another loc mapping.

Ok, that sounds reasonable. We also discussed backward matching in the past. I'm fine if we want to quickly test backward matching (or a mix of forward and backward matching and compare result), or just get the current version in and iterate on that later.

> The heuristic can be used to merge two location mappings (i.e., prefer the one that matches the current IR loc. If both differs, perhaps keep both - which may complicate the profile annotation).

Keep both will add complexity -- we will need to see meaningfully better result to justify it. Inclined to keep it simple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147545