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

Fri Sep 30 07:44:22 PDT 2022

aaron.ballman added a comment.

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

> In D134456#3827040 <https://reviews.llvm.org/D134456#3827040>, @aaron.ballman wrote:
>
>> In D134456#3819185 <https://reviews.llvm.org/D134456#3819185>, @rnk wrote:
>>
>>> In D134456#3819042 <https://reviews.llvm.org/D134456#3819042>, @aaron.ballman wrote:
>>>
>>>> Alternatively, we could document that we don't follow the intent of the standard feature and that's just the way things are, and add a command line option to honor that intent. However, given that GCC honors the attribute over PGO, I think we should change our default behavior.
>>>
>>> That makes sense to me. I think it's reasonable to request folks to add a flag in the context of a bug fix, but it would be too much to ask a drive by contributor to go through the multi-step process to change the default flag behavior.
>>
>> I guess I view it differently -- I think users should get the correct behavior by default rather than have to opt into it, and I'm not certain that "it's a drive-by contribution" is a good driver of user experience decisions. Either way, I think we need an explicit decision as to which approach we think is *right* and go from there in terms of how to achieve that goal. (It'd be a bit different if I thought this patch was incremental progress, but from my current views on the topic, I actually think the patch is a further regression away from where we want to be.)
>>
>> I'm adding in @dexonsmith as "branch weights" code owner and @dnovillo as "SampleProfile and related parts of ProfileData" code owner on the LLVM side to see if they have opinions on default behaviors (if there are other PGO experts to bring in, please sign them up).
>
> The design of PGO was to use user hints when there’s no coverage, but basically ignore them if theres enough data to say otherwise.

Whelp, if that's the design, then I guess we should stick with it. It's rather unfortunate to me that our default behavior is "ignore what the user told us because we're certain we know better" when we know that to be false and isn't how GCC behaves in this case, but it also is what it is. At least it's consistent with other <tongue in cheek>highly successful standardized optimization hints</tongue in cheek> like `register`, `inline`, and `[[carries_dependency]]`. :-P

> This safety scenario sounds like it could differ within a file. Is a flag really the right control? Maybe what you want is a `[[noprofile]]`, similar to `[[noopt]]`, which selectively disables the profile where the user wants fine-grained control to ignore the profile.

My understanding is that it's pretty rare for safety critical code to mix with non-safety critical code, so a flag sounds like the correct granularity to me in terms of the use cases I'm aware of. However, parallel attributes do give the most control to the user. My worry is that parallel attributes will be used as:

  #if __has_cpp_attribute(clang::likely_but_honor_this_one)
  #define LIKELY [[clang::likely_but_honor_this_one]]
  #elif __has_cpp_attribute(likely)
  #define LIKELY [[likely]]
  #else
  #define LIKELY
  #endif

either due to need (like safety critical situations) or due to user confusion (like we used to see happen with `register` or `inline` in the Olden Days), but I suppose a command line flag runs that same danger of misuse and is about as portable, so I could go with either approach.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134456