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

Fri Sep 30 10:08:20 PDT 2022

dexonsmith added a comment.

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.

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134456