[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

Fri Sep 30 10:26:32 PDT 2022

aaron.ballman added a comment.

In D134456#3827579 <https://reviews.llvm.org/D134456#3827579>, @dexonsmith wrote:

> In D134456#3827478 <https://reviews.llvm.org/D134456#3827478>, @aaron.ballman wrote:
>
>> In D134456#3827410 <https://reviews.llvm.org/D134456#3827410>, @dexonsmith wrote:
>>
>>> In D134456#3827377 <https://reviews.llvm.org/D134456#3827377>, @aaron.ballman wrote:
>>>
>>>> Another thought I had is: when PGO is enabled, can we diagnose when PGO countermands an explicit annotation? Something to at least alert the user "hey, look over here, this suggestion was wrong" so they have more of a chance of knowing something is up?
>>>
>>> Seems pretty noisy, much like when the inliner countermands an `inline` hint. Maybe an optimization remark would be appropriate, allowing advanced users to selectively enable them in a critical section when debugging a performance issue?
>>
>> Hmmm, on the one hand, any optimization hint is really an advanced user feature and so the opt remark might make sense. On the other hand, when I posted about `[[likely]]` and PGO on Twitter (not exactly what I'd call a scientific poll, take with an ocean of salt!), multiple people were surprised and expected PGO to honor the attribute, which suggests users may want this information. I don't have a good feel for what the user expectations are here, so I think an opt remark would be the most conservative way to start, and if we find that it's too hidden and non-expert users really need warnings, we can add a warning at that point. WDYT?
>
> Let's say it *was* scientific: even then, I disagree with your assertion that users *really* want this information, just because they were surprised about what happens. Many users will be surprised when told that the optimizer reorders their statements; or, more relevantly, that adding `inline` doesn't cause a function to be inlined; it doesn't follow that we should diagnose those situations.
>
> I think what users want is good performance, and a way to fix the performance when it's poor. If optimization remarks aren't serving the purpose of the latter (which I believe was their design goal), maybe they can be improved somehow.

+1; I think we agree on what users want. I have no idea if opt remarks aren't serving their purpose, it's possible they're perfectly fine.

> Optimization annotations are generally annoying for users since they give the illusion of control, and users feel frustrated when they aren't followed. Adding a diagnostic sounds nice, since then "at least they have some visibility"... but in practice the optimizer supersedes user expectations *everywhere*; you'll find it challenging to enable such diagnostics without getting back a wave of non-actionable diagnostics. IMO, `[[likely]]` isn't special here.
>
> Another point: having a diagnostic fire (failing a `-Werror` build) depending on the content of the profile passed to `-fprofile-use` seems pretty hostile to build workflows.

Okay, this last point is especially compelling to me. I think opt remarks are probably the correct way to go.

Thank you (everyone!) for the discussion on this. To make sure we're all on the same page for where we're at:

1. The changes in this review are reasonable and once review is finished, we're clear to land it.
2. We should file an issue to track the feature request for adding opt remarks for likelihood attribute disagreements.
3. We should file a bug to improve the PGO documentation (https://clang.llvm.org/docs/UsersManual.html#profile-guided-optimization) to explicitly document our behavior around explicitly-provided user annotations that disagree with PGO (this goes beyond `[[likely]]` and into things like `__builtin_expect`, `inline`, etc).

Does that sound like the right plan to others?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134456