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

Fri Sep 23 08:59:36 PDT 2022

aaron.ballman added a comment.

In D134456#3810769 <https://reviews.llvm.org/D134456#3810769>, @serge-sans-paille wrote:

> Shouldn't this be a situation where `-Wmisexpect` shold / could trigger?

If we're going to let PGO override what the user explicitly wrote in their code, we definitely should be diagnosing it. But I'm still not convinced that PGO winning actually matches the intent of the feature.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0479r2.html was the last version of the paper which still had its motivation section (<aside>it's very frustrating when WG21 authors strip relevant information from the paper being adopted!</aside>). It talks about PGO explicitly:

  Objection #2: Why should branch hints be standardized when many compilers implement PGO?

  Branch hints are definitely not meant to replace PGO; if PGO can be used it should be. PGO may be able to give the optimizer even more information then a branch hint. However, PGO and branch hints do not need to be mutually exclusive; branch hints could act as another indicator of expected frequent usage of particular branches.

  Also, PGO is not always easy to use. PGO necessitates creating a realistic test scenario to profile which, depending on the application, may be difficult and opens up the possibility that some important real world usage may be missed. The additional build tooling to keep the profile up to date, particularly across multiple OS's and compilers may also be challenging.

  Additionally, certain kinds of applications, such as those which may be distributed to clients who then build and run their own plugins in the distributed application may be very difficult or impossible to generate a realistic benchmark for, as there is no way of knowing how the client will use the application. Similar problems could also occur if an application is highly configurable; creating test scenarios and builds for all different possible configurations may not be realistic.

  Finally, PGO sometimes does the exact opposite of what is wanted for certain low-latency use cases. PGO tries to make the common case faster, but it may actually be more important to make the uncommon cases faster so that when they do occur they run as quickly as possible.

The first few paragraphs read to me like "sure, PGO should win" but the last paragraph reads like "no, PGO should lose". I remember when we were discussing this proposal in WG21 and the last paragraph was what motivated a number of committee members, especially in safety-critical spaces. Looking back through Core discussion, this point was brought up there as well as being important (https://lists.isocpp.org/core/2019/09/7193.php for those with access to the committee reflectors).

> If we've already made promises about how this is supposed to work, I'd prefer to trust our past decisions rather than revisiting them.

I'm sympathetic to this but only if we actually made a decision here instead of documenting whatever behavior we got by default. https://reviews.llvm.org/D59467 is the original review which added this feature and from my reading of that we recognized the conflict between PGO and an explicit annotation, but it doesn't look like this concern was really considered.

Do users have some other escape hatch to tell PGO "ignore what you think you know about this branch" so that a user who *wants* PGO to lose has some ability to do that other than "don't use PGO"?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134456