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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D134456#3827377 <https://reviews.llvm.org/D134456#3827377>, @aaron.ballman wrote:

> In D134456#3827332 <https://reviews.llvm.org/D134456#3827332>, @dexonsmith wrote:
>
>> In D134456#3827263 <https://reviews.llvm.org/D134456#3827263>, @aaron.ballman wrote:
>>
>>> 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
>>
>> To be clear, I was imagining:
>>
>>   if (always_false()) [[likely]] [[clang::nopgo]] {
>>     // ...
>>   }
>>
>> where ``nopgo`` might be independently useful for telling clang to ignore any collected PGO data when estimating branch weights in a particular control flow block.
>>
>> Some users might combine the two into a macro ("always ignore the profile when I say something is likely!"), but I don't think there'd be a cascade.
>
> Ah, I see, thank you for clarifying! Does that seem like a generally useful attribute to you? (It seems like it could be, but this isn't my area of expertise.)

Well, "generally useful" might be a stretch. Probably most users wouldn't want/need this. But if a user wants fine-grained control of the optimizer (probably not achievable, really), then turning off PGO seems like a knob they might want.

On the other hand, maybe `[[nopgo]]` is all that the safety-critical code should be using. IIRC, `[[likely]]` is really saying "hint: the other path is cold", and the optimizer pessimists the other path. Perhaps the safety-critical crowd just wants to turn off all pessimization; if so, they'll find that adding `[[likely]]` to the must-fail-quickly error path will do bad things to the non-error path.

> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134456