[PATCH] D147954: [SimplifyCFG]Prevent premature simplification of indirect callsites with profile data

[Mingming Liu via Phabricator via llvm-commits]

mingmingl added a comment.

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

> In D147954#4276299 <https://reviews.llvm.org/D147954#4276299>, @wenlei wrote:
>
>> I guess you do this only for calls and limit it for pre-link because you want to minimize the restriction on optimization, while making post-link inlining operate on accurate call profile?

Yep this is the plan, but for post-link indirect-call-promotion (which could be subsequently inlined after ICP)

>> Code motion is generally destructive for profile quality. Instead of blocking all sinking and hoisting, have you considered trying to update profile for sinking/hoisting when there's no merging, but only block those that involves merging (i.e. head/tail merge) with incompatible profile? I imagine in some cases, allow sinking/hoisting but updating profile properly may help better inlining later.

For my understanding, is https://gcc.godbolt.org/z/cMWY9Tqzv an example where updating profile would be useful? I think sinking/hoisting would merge instructions but I might miss something.

- In this example, two direct callsites are merged but both `branch_weights` metadata (`!prof 0`, `!prof 1`) are dropped which is sub-optimal. It's likely better in majority of cases by merging two `!prof` into one and accumulate counters.

Nevertheless, (after thinking about direct callsites above), I modified the implementation (and option names) to look at indirect calls only primarily because 1) the desired behavior upon icall (indicating `!prof` is value profile as opposed to `branch_weights` for direct call) is probably clearer and 2) the pattern shows up (as C++ code shows)  with polymorphism, and meanwhile added a 'FIXME' to mention the current 'hoisting' is still suboptimal since it dropped `branch_weights` on two identical direct calls.

I concur it's better to decide on option name once (`preserve-call` vs `preserve-indirect-call`) so I'm fine with implementing the sum-of-branch-weight counters or a better alternative in this patch (and name the option back to more general `preserve-call...` Let me know your thoughts!

> This one is mainly for profile based devirtualization. Related to inlining, there is a pre-pass to do the unmerging:

For my information, in which pass does the unmerging happens?

> arg = select cond, 1, val
> call foo(arg)
>
>>
> -
>
> if (cond) {
>
>   call foo(1)
>
> } else
>
>   call foo(val)





================
Comment at: llvm/test/Transforms/SimplifyCFG/preserve-call-metadata-in-hoist.ll:8
+; Without preserving call instructions, `d->func1` is hoisted while it may not make sense
+; to do so. For example, the candidate calls are different based on derived type.
+; class Base {
----------------
wenlei wrote:
> davidxl wrote:
> > Perhaps add "merging meta data will make the profile data less precise and not desirable either".
>  would be good to mention it in code comment and change description as well. 
sure, added comments why preserving !prof metadata (and not hoisting/sinking) in cpp files (indent existing comments accordingly).


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

https://reviews.llvm.org/D147954