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

Fri Apr 21 15:28:11 PDT 2023

mingmingl added a comment.

In D148876#4288873 <https://reviews.llvm.org/D148876#4288873>, @hoy wrote:

> In D148876#4288824 <https://reviews.llvm.org/D148876#4288824>, @mingmingl wrote:
>
>> In D148876#4288774 <https://reviews.llvm.org/D148876#4288774>, @hoy wrote:
>>
>>> In D148876#4288764 <https://reviews.llvm.org/D148876#4288764>, @mingmingl wrote:
>>>
>>>> In D148876#4288686 <https://reviews.llvm.org/D148876#4288686>, @hoy wrote:
>>>>
>>>>> In D148876#4288504 <https://reviews.llvm.org/D148876#4288504>, @mingmingl wrote:
>>>>>
>>>>>> In D148876#4288286 <https://reviews.llvm.org/D148876#4288286>, @tejohnson wrote:
>>>>>>
>>>>>>> Would be good to explain motivation (maybe reference other patch?)
>>>>>>
>>>>>> Makes sense, will do.
>>>>>>
>>>>>>> Also, the new tests only test the behavior with the internal option. It would be good to have tests with various pipelines to check the logic on when it gets cleared in those different cases simply based on the new pipeline logic.
>>>>>>
>>>>>> I see your point. I'm going to write a test like https://github.com/llvm/llvm-project/blob/main/llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll to test pipeline logic.
>>>>>>
>>>>>> In D148876#4288429 <https://reviews.llvm.org/D148876#4288429>, @wenlei wrote:
>>>>>>
>>>>>>> cc @hoy to make sure this works for csspgo.
>>>>>>
>>>>>> thanks! The assumption here is that ICP runs once per pipeline and there are no other passes that uses value-profiles. If the assumption doesn't hold, I'm thinking about an explicit boolean in ICP pass constructor (which makes pipeline logic a little bit more complex)
>>>>>
>>>>> Value profiles for indirect call sites can still be used by lld call graph sorting after ICP. Please see https://github.com/llvm/llvm-project/blob/26202a57e5e78b5473ef657737d181f0a68ed56d/llvm/lib/Transforms/Instrumentation/CGProfile.cpp#L81
>>>>
>>>> oh thanks for this pointer! Clearing is not an option then.
>>>>
>>>> CGProfile.cpp uses <counter, target-val> pairs but not 'total-count' <https://github.com/llvm/llvm-project/blob/26202a57e5e78b5473ef657737d181f0a68ed56d/llvm/lib/Transforms/Instrumentation/CGProfile.cpp#L84>, I'm suggesting this option. Let me know your thoughts!
>>>>
>>>> In this patch
>>>>
>>>> 1. extend its format from `!1 = !{!"VP",  IndirectCallKind , Sum, counter1, val1, counter2, val2}` to `!1 = !{!"VP",  Kind , Sum, counter1, val1, counter2, val2, boolean/int 'indirect-call-promotion-all-done'} `
>>>> 2. `indirect-call-promotion-all-done` is initially 0, and set to 1 after the last ICP in a compiler pipeline
>>>>
>>>> In D147954 <https://reviews.llvm.org/D147954>
>>>>
>>>> 3. simplify-cfg bails out from merge indirect-calls when seeing `indirect-call-promotion-all-done` as 0, could merge target value profiles after seeing `indirect-call-promotion-all-done` being set to 1, so that `CGProfile.cpp` could get the <counter, target-val> pairs.
>>>>
>>>> Meanwhile, the current ICP should probably still preserve counts as opposed to clearing <https://github.com/llvm/llvm-project/blob/26202a57e5e78b5473ef657737d181f0a68ed56d/llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp#L317> when all targets are used.
>>>>
>>>> cc @tejohnson @davidxl
>>>
>>> I guess I don't get the motivation of the original change. Can you please remind me why you'd like to cleanup VP metadata after ICP?
>>
>> (D147954 <https://reviews.llvm.org/D147954> is about the problem to solve and has more discussions) In brief, https://gcc.godbolt.org/z/Pxoh8PrGE shows a case where simplify-cfg aggressively drops profile metadata while it shouldn't. This would happen when prelink simplify-cfg enables instruction sink and postlink ICP couldn't see the value profiles.
>> The motivation of this patch and D147954 <https://reviews.llvm.org/D147954> is to preserve value profiles attached on indirect calls for indirect-call-promotion to use.
>>
>> Turning off simplify-cfg sinking/hoisting completely in prelink pipeline would be more invasive -> for 'llvm/test/Transforms/SimplifyCFG/merge-direct-call-branch-weights-in-hoist.ll' in D147954 <https://reviews.llvm.org/D147954>, it's desired to hoist address calculation instructions (from `if.then` and `if.else` in https://gcc.godbolt.org/z/b4Y9Mqrq1) in prelink stage.
>
> Thanks for the explanation. IIUC, the fix for SimplyCFG dropping !VP is to either block the corresponding optimization or to merge the !VP data when merge callsites. What is the problem of always merging callsite !VP?

Take https://gcc.godbolt.org/z/o6G68rn3v as example, `func1` and `func2` are two different virtual methods so inherently has different target values.

Say each virtual callsite has a sum and a list of <counter, target-value> pairs, merging <counter, target-value> pairs is okay, either merging sum, or use max(sum1, sum2) changed the ratio, and could make a significant target value no longer profitable according to [[ https://github.com/llvm/llvm-project/blob/ddcc50721a04996f52038066c693866daefa7d19/llvm/lib/Analysis/IndirectCallPromotionAnalysis.cpp#L52 | `isPromotionProfitable`   ]]

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148876