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

Fri Sep 30 08:13:40 PDT 2022

aaron.ballman added a comment.

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

> In D134456#3827263 <https://reviews.llvm.org/D134456#3827263>, @aaron.ballman wrote:
>
>> In D134456#3827185 <https://reviews.llvm.org/D134456#3827185>, @dexonsmith wrote:
>>
>>> 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.
>
> In that case, maybe those files should just turn off PGO entirely. There's already a command-line for that:
>
> - `-fprofile-use=...`: compiler should use the profile to improve performance (user hints lose when there's disagreement)
> - default: no access to a profile; user hints by default
>
> Maybe (if it doesn't exist yet) `-fno-profile-use` would be useful for easy cancellation.
>
> In safety-critical code where you don't entirely trust the profile/coverage/compiler to do the right thing, why risk having the feature enabled at all?

My point is that the annotation is a way for the user to be able to entirely trust the implementation so that they can enable things like PGO, and because it's a standard attribute and GCC honors it during PGO, the user gets some theoretical portability out of it. But, 1) attributes can be ignored, even standard ones, and so there really isn't a super strong portability argument in terms of being able to *rely* on anything, 2) optimization hints are traditionally things the optimizer ignores when it believes it knows better, 3) it matches the design intent of our PGO implementation. So I think I'm sold on the approach in this patch, and going with a separate set of attributes to give users more control -- that at least leaves users in a situation where they can benefit from PGO but still have more control over its behavior. (I don't think this patch should be blocked on those attributes being implemented, either.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134456