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

Fri Sep 30 07:20:16 PDT 2022

dexonsmith added a comment.

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.

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134456