[PATCH] D147954: [SimplifyCFG] Merge branch_weights for direct callsites and introduce options to preserve value profile for indirect callsites

[Mingming Liu via Phabricator via llvm-commits]

mingmingl planned changes to this revision.
mingmingl added a comment.

In D147954#4280439 <https://reviews.llvm.org/D147954#4280439>, @tejohnson wrote:

>> In D147954#4277744 <https://reviews.llvm.org/D147954#4277744>, @mingmingl wrote:
>>
>>> In D147954#4276844 <https://reviews.llvm.org/D147954#4276844>, @nikic wrote:
>>>
>>>> Could you please start by implementing metadata merging, and then return to this patch only if strictly necessary? I'd rather not have another SimplifyCFG option for such a niche use case, if it's possible to get most of the benefit via standard metadata merging.
>>>
>>> (quick reply to describe the plan) okay, let me implement something as described and see how it works for this use case
>>>
>>> 1. do metadata merging for direct callsites 2) conservatively not hoisting/sinking indirect callsites for indirect callsites if `!prof` presents -> indirect-call-promotion pass will clear `!prof` for indirect-callsite (since there are no other uses of  this type of value profiles) so the SimplifyCFG pass after indirect-call-promotion should be able to do a full cleanup if the indirect-call is still in the IR but not speculatively devirtualized.
>>
>> Actually merging two different virtual calls with the same counters could be counterproductive, and ICP pass preserves `!prof` when not all information is used so I kept the option.
>
> I thought per our offline chat that you were going to try merging the !prof VP metadata. When is it counterproductive?

sorry for the confusion. merging !prof of indirect call targets may make target values not significant (e.g., in https://gcc.godbolt.org/z/o6G68rn3v, 'func1' and func2' are virtual calls with different address offset so the target values are different; when call-count is updated as a sum, the values become not significant).

As discussed (with xur@ together, I'll change the implementation as 
(Possible in a another patch before this)

1. introduce an option in ICP pass -to clear 'direct-call-targets' after the last ICP-> it could be a partial clear when one `!prof` has more than indirect-call target value profiles
2. Let pass builder set the option above  (e.g. post link in a LTO pipeline, or the ICP in a non-LTO pipeline)

In this D147954 <https://reviews.llvm.org/D147954>

3. Get rid of the option in SimplifyCFG pass
4. Change SimplifyCFG to bail out when it sees an indirect call with target value profiles

And move 'branch_weight' merge to a separate patch as well (per comment)

In this way, value-profile metadata is preserved for ICP to make use of it, and simplifycfg could proceed to sink/hoist indirect calls if ICP didn't happen.

In D147954#4280751 <https://reviews.llvm.org/D147954#4280751>, @davidxl wrote:

> Should the direct call prof data merge be in a separate patch?

Yep, a separate patch is reasonable. Will do that.


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

https://reviews.llvm.org/D147954