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

[Mingming Liu via Phabricator via llvm-commits]

mingmingl added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148876