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

[AntonBikineev]

Summary

Generally, with PGO enabled the C++20 likelyhood attributes shall be
dropped assuming the profile has a good coverage. However, currently
this is not the case for the following code:

if (always_false()) [[likely]] {
  ...
}

The patch fixes this and drops the attribute, if the parent context was
executed in the profile. The patch still preserves the attribute, if the
parent context was not executed, e.g. to support the cases when the
profile has insufficient coverage and the programmer's assumption is
correct.

[hans]

Looks reasonable to me.

+the folks from D85091 fyi or if they want to take a look.

clang/test/Profile/cxx-never-executed-branch.cpp

2

ultra nit: s/the/that/ (for the first one)

5

This probably needs a -target flag, since otherwise the mangled function names might not match the profile (e.g. on Windows).

[AntonBikineev]

Thanks!

[aaron.ballman]

I'm on the fence about this... the attributes were added as optimization hints, but for somewhat interesting use cases. The obvious one of "I think this path is likely/unlikely and so I'll help the compiler figure it out" was one such use case, but honestly that's akin to inline and register in terms of how likely it is the user will make a better decision than the optimizer. But the more interesting use case was "The optimizer is going to consider this to be the unlikely path, but I need it to be optimized because I need my failure code to fail as fast as possible". Basically, some folks wanted to use this to override the otherwise-reasonable decisions from the optimizer. Based on that, I'm not certain we should let PGO win -- the user is telling you "optimize based on something I know more about than you", and having a profile that covers the code path doesn't necessarily change that. WDYT?

[rnk]

clang/lib/CodeGen/CGStmt.cpp

815

I lean towards implementing the intended behavior reflected in this comment here, which is that PGO data overrides programmer annotations in case of conflict. We should also check the PGO docs to see if this is documented. 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.

[AntonBikineev]

In D134456#3809581, @aaron.ballman wrote:

I'm on the fence about this... the attributes were added as optimization hints, but for somewhat interesting use cases. The obvious one of "I think this path is likely/unlikely and so I'll help the compiler figure it out" was one such use case, but honestly that's akin to inline and register in terms of how likely it is the user will make a better decision than the optimizer. But the more interesting use case was "The optimizer is going to consider this to be the unlikely path, but I need it to be optimized because I need my failure code to fail as fast as possible". Basically, some folks wanted to use this to override the otherwise-reasonable decisions from the optimizer. Based on that, I'm not certain we should let PGO win -- the user is telling you "optimize based on something I know more about than you", and having a profile that covers the code path doesn't necessarily change that. WDYT?

I don't think that the second case is how things works today. PGO is still preferred over the attributes. However, the current behavior is somewhat strange:

• if the probability is infinitesimal (e.g. 0.00001%), the compiler will ignore the attribute,
• if the probability is zero (the branch never hit in the profile), the compiler will consider the attribute.

clang/lib/CodeGen/CGStmt.cpp

815

As Hans pointed out in a Chrome bug: the behaviour is documented here: https://clang.llvm.org/docs/AttributeReference.html#likely-and-unlikely:
"These attributes have no effect on the generated code when using PGO (Profile-Guided Optimization) or at optimization level 0."

[serge-sans-paille]

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

[aaron.ballman]

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

[Mordante]

When I implemented P0479R5 I didn't look at earlier revisions of the paper. I'm a bit surprised to see revision 2 had a lot more information. (At that time I was less familiar with WG21 papers, so I didn't consider looking at older papers or notice the missing revision history.) I agree with @aaron.ballman's observation; revision 2 is unclear what the best solution regarding overriding PGO is.

I'm not really active in Clang at the moment so I feel @aaron.ballman has a better view of which approach to take for `[[likely]]` and PGO.

[AntonBikineev]

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

Sounds like we need another proposal for [[always_likely]] :)
I understand both pros and cons of PGO having preference. However, whatever solution we stick to, I'd prefer it to be consistent.

[aaron.ballman]

In D134456#3814628, @AntonBikineev wrote:

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

Sounds like we need another proposal for [[always_likely]] :)

No, it sounds like we need a proposal for [[likely_unless_the_optimizer_decides_otherwise]] -- the [[likely]] attribute was intended for always-likely optimization decisions.

I understand both pros and cons of PGO having preference. However, whatever solution we stick to, I'd prefer it to be consistent.

I agree that we should be consistent, but there's different ways we can be consistent. Consistent with what the feature paper intended? Consistent with our past decisions? Consistent with how other implementations behave?

What do other implementations that support PGO do? If PGO implementations consistently behave a certain way, that would be interesting to know.

[AntonBikineev]

No, it sounds like we need a proposal for [[likely_unless_the_optimizer_decides_otherwise]] -- the [[likely]] attribute was intended for always-likely optimization decisions.

Well, whatever the default behavior is chosen.

I agree that we should be consistent, but there's different ways we can be consistent. Consistent with what the feature paper intended? Consistent with our past decisions? Consistent with how other implementations behave?

What I meant is that if the status quo is preserved, the never taken [[likely]] branch would be considered as "unlikely'' after PGO (the purpose of this patch).

What do other implementations that support PGO do? If PGO implementations consistently behave a certain way, that would be interesting to know.

I just checked GCC and it seems like it considers likely as a forcing optimization: https://godbolt.org/z/69E3383cq

[aaron.ballman]

In D134456#3814820, @AntonBikineev wrote:

No, it sounds like we need a proposal for [[likely_unless_the_optimizer_decides_otherwise]] -- the [[likely]] attribute was intended for always-likely optimization decisions.

Well, whatever the default behavior is chosen.

Agreed -- and we can easily add such an attribute under the clang vendor namespace once we nail down the default behavior we want for [[likely]].

I agree that we should be consistent, but there's different ways we can be consistent. Consistent with what the feature paper intended? Consistent with our past decisions? Consistent with how other implementations behave?

What I meant is that if the status quo is preserved, the never taken [[likely]] branch would be considered as "unlikely'' after PGO (the purpose of this patch).

What do other implementations that support PGO do? If PGO implementations consistently behave a certain way, that would be interesting to know.

I just checked GCC and it seems like it considers likely as a forcing optimization: https://godbolt.org/z/69E3383cq

Thanks for checking! It doesn't seem to have any impact on ICC one way or the other (at least from my simple testing).

[wenlei]

There's also subtle difference between different variants of PGO. For instrumentation PGO where profile quality is higher, profile counts always take precedence. But for sampling PGO (AutoFDO, CSSPGO), it tries to not overwrite any existing branch weights, which means the weights derived from hints earlier are honored. But there's count smoothing algorithm that try to make make CFG counts consistent, which may occasionally overwrite some branch weights that seem really off based on surround count/flow.

The current default looks more like a hint rather than a directive..

[thakis]

Maybe we could have a -f flag to control whether to prefer annotations or profile data when they conflict?

[aaron.ballman]

In D134456#3818548, @thakis wrote:

Maybe we could have a -f flag to control whether to prefer annotations or profile data when they conflict?

I think that would be reasonable to at least consider; the default behavior can be "the attribute wins" and this gives users an escape hatch to say "no, I trust PGO more, let that win instead".

[rnk]

In D134456#3818952, @aaron.ballman wrote:

I think that would be reasonable to at least consider; the default behavior can be "the attribute wins" and this gives users an escape hatch to say "no, I trust PGO more, let that win instead".

Right, but that's a policy change. We'd need to update docs, update release notes, and all of that. We'll need this fix in any case for the mode where PGO wins, which will remain long term for the users that rely on it.

What commitments can we make to land this fix without making a policy change, which is quite a great deal of work (RFC, docs, notes, etc)?

[aaron.ballman]

In D134456#3818982, @rnk wrote:

In D134456#3818952, @aaron.ballman wrote:

I think that would be reasonable to at least consider; the default behavior can be "the attribute wins" and this gives users an escape hatch to say "no, I trust PGO more, let that win instead".

Right, but that's a policy change. We'd need to update docs, update release notes, and all of that. We'll need this fix in any case for the mode where PGO wins, which will remain long term for the users that rely on it.

What commitments can we make to land this fix without making a policy change, which is quite a great deal of work (RFC, docs, notes, etc)?

Personally, I consider it a bug fix and not a policy change (which doesn't mean we don't need to make all these updates, of course). We didn't carefully consider this aspect of the original design when doing the implementation work, now we'd like to correct that as responsibly as we can.

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.

[wenlei]

For instr PGO, changing behavior may have perf implication. cc @davidxl

[rnk]

In 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.

[aaron.ballman]

In D134456#3819185, @rnk wrote:

In 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).

[dexonsmith]

In D134456#3827040, @aaron.ballman wrote:

In D134456#3819185, @rnk wrote:

In 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.

[aaron.ballman]

In D134456#3827185, @dexonsmith wrote:

In D134456#3827040, @aaron.ballman wrote:

In D134456#3819185, @rnk wrote:

In 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.

[dexonsmith]

In D134456#3827263, @aaron.ballman wrote:

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

[aaron.ballman]

In D134456#3827281, @dexonsmith wrote:

In D134456#3827263, @aaron.ballman wrote:

In 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.)

[dexonsmith]

In 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.

[dexonsmith]

In D134456#3827332, @dexonsmith wrote:

In 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.

(In practice a use of nopgo probably needs to drop the profile for the whole function, but maybe that's fine...)

[aaron.ballman]

In D134456#3827332, @dexonsmith wrote:

In 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.)

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?

[dexonsmith]

In D134456#3827377, @aaron.ballman wrote:

In D134456#3827332, @dexonsmith wrote:

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

[aaron.ballman]

In D134456#3827410, @dexonsmith wrote:

In D134456#3827377, @aaron.ballman wrote:

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.

Heh, you interpreted me properly. :-D I mostly meant "does this attribute seem like it'd be usable in other circumstances or is it likely to be really specific to just this situation?

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.

If "bad things" is "make the non-error path slow", my understanding is that's exactly their expectations.

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?

Hmmm, on the one hand, any optimization hint is really an advanced user feature and so the opt remark might make sense. On the other hand, when I posted about [[likely]] and PGO on Twitter (not exactly what I'd call a scientific poll, take with an ocean of salt!), multiple people were surprised and expected PGO to honor the attribute, which suggests users may want this information. I don't have a good feel for what the user expectations are here, so I think an opt remark would be the most conservative way to start, and if we find that it's too hidden and non-expert users really need warnings, we can add a warning at that point. WDYT?

[dexonsmith]

In D134456#3827478, @aaron.ballman wrote:

In D134456#3827410, @dexonsmith wrote:

In D134456#3827377, @aaron.ballman wrote:

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?

Hmmm, on the one hand, any optimization hint is really an advanced user feature and so the opt remark might make sense. On the other hand, when I posted about [[likely]] and PGO on Twitter (not exactly what I'd call a scientific poll, take with an ocean of salt!), multiple people were surprised and expected PGO to honor the attribute, which suggests users may want this information. I don't have a good feel for what the user expectations are here, so I think an opt remark would be the most conservative way to start, and if we find that it's too hidden and non-expert users really need warnings, we can add a warning at that point. WDYT?

Let's say it *was* scientific: even then, I disagree with your assertion that users *really* want this information, just because they were surprised about what happens. Many users will be surprised when told that the optimizer reorders their statements; or, more relevantly, that adding inline doesn't cause a function to be inlined; it doesn't follow that we should diagnose those situations.

I think what users want is good performance, and a way to fix the performance when it's poor. If optimization remarks aren't serving the purpose of the latter (which I believe was their design goal), maybe they can be improved somehow.

Optimization annotations are generally annoying for users since they give the illusion of control, and users feel frustrated when they aren't followed. Adding a diagnostic sounds nice, since then "at least they have some visibility"... but in practice the optimizer supersedes user expectations *everywhere*; you'll find it challenging to enable such diagnostics without getting back a wave of non-actionable diagnostics. IMO, [[likely]] isn't special here.

Another point: having a diagnostic fire (failing a -Werror build) depending on the content of the profile passed to -fprofile-use seems pretty hostile to build workflows.

[aaron.ballman]

In D134456#3827579, @dexonsmith wrote:

In D134456#3827478, @aaron.ballman wrote:

In D134456#3827410, @dexonsmith wrote:

In D134456#3827377, @aaron.ballman wrote:

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?

Hmmm, on the one hand, any optimization hint is really an advanced user feature and so the opt remark might make sense. On the other hand, when I posted about [[likely]] and PGO on Twitter (not exactly what I'd call a scientific poll, take with an ocean of salt!), multiple people were surprised and expected PGO to honor the attribute, which suggests users may want this information. I don't have a good feel for what the user expectations are here, so I think an opt remark would be the most conservative way to start, and if we find that it's too hidden and non-expert users really need warnings, we can add a warning at that point. WDYT?

Let's say it *was* scientific: even then, I disagree with your assertion that users *really* want this information, just because they were surprised about what happens. Many users will be surprised when told that the optimizer reorders their statements; or, more relevantly, that adding inline doesn't cause a function to be inlined; it doesn't follow that we should diagnose those situations.

I think what users want is good performance, and a way to fix the performance when it's poor. If optimization remarks aren't serving the purpose of the latter (which I believe was their design goal), maybe they can be improved somehow.

+1; I think we agree on what users want. I have no idea if opt remarks aren't serving their purpose, it's possible they're perfectly fine.

Optimization annotations are generally annoying for users since they give the illusion of control, and users feel frustrated when they aren't followed. Adding a diagnostic sounds nice, since then "at least they have some visibility"... but in practice the optimizer supersedes user expectations *everywhere*; you'll find it challenging to enable such diagnostics without getting back a wave of non-actionable diagnostics. IMO, [[likely]] isn't special here.

Another point: having a diagnostic fire (failing a -Werror build) depending on the content of the profile passed to -fprofile-use seems pretty hostile to build workflows.

Okay, this last point is especially compelling to me. I think opt remarks are probably the correct way to go.

Thank you (everyone!) for the discussion on this. To make sure we're all on the same page for where we're at:

1. The changes in this review are reasonable and once review is finished, we're clear to land it.
2. We should file an issue to track the feature request for adding opt remarks for likelihood attribute disagreements.
3. We should file a bug to improve the PGO documentation (https://clang.llvm.org/docs/UsersManual.html#profile-guided-optimization) to explicitly document our behavior around explicitly-provided user annotations that disagree with PGO (this goes beyond [[likely]] and into things like __builtin_expect, inline, etc).

Does that sound like the right plan to others?

[dexonsmith]

SGTM.

[AntonBikineev]

Anyone wants to take a look/stamp? :)

[aaron.ballman]

I'm not comfortable giving the final LG but the changes here look reasonable to me.

clang/lib/CodeGen/CGStmt.cpp

829–831

You can drop the braces here per the coding standard.

[hans]

lgtm

[aaron.ballman]

In D134456#3827661, @aaron.ballman wrote:

Thank you (everyone!) for the discussion on this. To make sure we're all on the same page for where we're at:

1. The changes in this review are reasonable and once review is finished, we're clear to land it.
2. We should file an issue to track the feature request for adding opt remarks for likelihood attribute disagreements.

I filed https://github.com/llvm/llvm-project/issues/58187 for this.

3. We should file a bug to improve the PGO documentation (https://clang.llvm.org/docs/UsersManual.html#profile-guided-optimization) to explicitly document our behavior around explicitly-provided user annotations that disagree with PGO (this goes beyond [[likely]] and into things like __builtin_expect, inline, etc).

I filed https://github.com/llvm/llvm-project/issues/58189 for this.

[AntonBikineev]

Thanks everybody!

[thakis]

Another point: having a diagnostic fire (failing a -Werror build) depending on the content of the profile passed to -fprofile-use seems pretty hostile to build workflows.

Okay, this last point is especially compelling to me. I think opt remarks are probably the correct way to go.

Zombie comment, but I think optimization remarks and warnings for when the [[likely]] annotation doesn't match profile data already exist, see here: https://clang.llvm.org/docs/MisExpect.html (it predates this review by a few months, but I just learned about it recently).

clang/lib/CodeGen/CGStmt.cpp

   // Prefer the PGO based weights over the likelihood attribute.
   // When the build isn't optimized the metadata isn't used, so don't generate
   // it.
+  // Also, differentiate between disabled PGO and a never executed branch with
+  // PGO. Assuming PGO is in use:
+  // - we want to ignore the [[likely]] attribute if the branch is never
+  // executed,
+  // - assuming the profile is poor, preserving the attribute may still be
+  // beneficial.
+  // As an approximation, preserve the attribute only if both the branch and the
+  // parent context were not executed.
   Stmt::Likelihood LH = Stmt::LH_None;
-  uint64_t Count = getProfileCount(S.getThen());
-  if (!Count && CGM.getCodeGenOpts().OptimizationLevel)
+  uint64_t ThenCount = getProfileCount(S.getThen());
+  if (!ThenCount && !getCurrentProfileCount() &&
+      CGM.getCodeGenOpts().OptimizationLevel) {
     LH = Stmt::getLikelihood(S.getThen(), S.getElse());
-  EmitBranchOnBoolExpr(S.getCond(), ThenBlock, ElseBlock, Count, LH);
+  }
+  EmitBranchOnBoolExpr(S.getCond(), ThenBlock, ElseBlock, ThenCount, LH);
 
   // Emit the 'then' code.
   EmitBlock(ThenBlock);

clang/test/Profile/Inputs/cxx-never-executed-branch.proftext

+_Z13is_in_profilev
+0x000000029f493458
+2
+20000
+# The branch is never executed.
+0
+
+_Z17is_not_in_profilev
+0x000000029f493458
+2
+# The function was never executed
+0
+0

clang/test/Profile/cxx-never-executed-branch.cpp

+// Test that clang doesn't emit llvm.expect when the counter is 0
+
+// RUN: llvm-profdata merge %S/Inputs/cxx-never-executed-branch.proftext -o %t.profdata
+// RUN: %clang_cc1 -std=c++20 %s -triple %itanium_abi_triple -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -disable-llvm-passes | FileCheck %s
+
+int rand();
+
+// CHECK: define {{.*}}@_Z13is_in_profilev
+// CHECK-NOT: call {{.*}}@llvm.expect
+
+int is_in_profile() {
+  int rando = rand();
+  int x = 0;
+  if (rando == 0) [[likely]]
+    x = 2;
+  else
+    x = 3;
+  return x;
+}
+
+// CHECK: define {{.*}}@_Z17is_not_in_profilev
+// CHECK: call {{.*}}@llvm.expect
+
+int is_not_in_profile() {
+  int rando = rand();
+  int x = 0;
+  if (rando == 0) [[likely]]
+    x = 2;
+  else
+    x = 3;
+  return x;
+}