Proposal for Adoption of C++20 [[likely]] and [[unlikely]] Attributes in Chromium Codebase

[Punith Nayak]

I would like to propose the adoption of the C++20 standardized [[likely]] and [[unlikely]] attributes (as outlined in https://en.cppreference.com/w/cpp/language/attributes/likely) within the Chromium codebase -> https://issues.chromium.org/issues/40256217

Currently, Chromium uses custom macros like LIKELY and UNLIKELY to provide similar hints to the compiler. However, with the availability of standardized attributes in C++20, it's advantageous for us to migrate

I encourage the group to consider this proposal and initiate discussions on the best approach for implementing these changes. 

Regards,

Punith B Nayak

---

Open-source contributor @Wikimedia, @Chromium

[Peter Boström]

+1 lgtm please do

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/a7dbafd5-e5fd-48e2-82cd-6bbd938d77e4n%40chromium.org.

[Peter Boström]

Not sure what you're asking for w/r/t implementing these changes. If you're picking up the bug I would probably replace them piecemeal across the codebase then remove the macro, there's not too many imo.

pbos@pbos:~/chromium/src (roll_protobuf)$ git grep "\<LIKELY(" | wc -l
315
pbos@pbos:~/chromium/src (roll_protobuf)$ git grep "\<UNLIKELY(" | wc -l
1022

[danakj]

+1, agree we should migrate to the standard.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sH9j5XMkwDCNOVowJ0_gU2b4UreGjwdy%3DrHLkZC64t9DQ%40mail.gmail.com.

[Roland McGrath]

The way these attributes are used is somewhat new and different from the old `__builtin_expect` that underlies past macro approaches. It doesn't go on an expression used in a conditional. Instead, it precedes a statement syntactically and says any way of reaching that path is likely/unlikely. So it's probably a good idea to establish some explicit style guidance on how to use it syntactically.  In another code base, I have been using this style:

```

if (foo) [[unlikely]] {

  ...

}

if (bar) {

  ...

} else [[unlikely]] {
  ...

}

```

I don't know if that's what people most like.  It's semantically equivalent to:

```

if (foo) {

  [[unlikely]];

  ...

}

```

I prefer the former and find it more natural and readable. But as with all style issues the most important aspect is just to have a consistent standard that everybody uses across the codebase.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaR2XQn%2BVNvkX1bw-m4sFudPnnQT6MKO_tCLun52JeJ70A%40mail.gmail.com.

[Peter Boström]

I'd like the former too. I assume that outside conditionals you could also have:

switch(x) {
  case y:
    return bar;
  case z:
    return baz;
}
[[unlikely]];
return boohoo;

Would that still be valid? I.e. does it annotate "this statement is unlikely to be hit"?

[Roland McGrath]

Yes, it means "this statement is (un)likely to be reached", which implies that the code paths following that statement (and not reachable otherwise) are also.  So it can go before any statement, whether that's `something;` or `{ something; }` or just `;` or `{}`.  And in C(++) the "body" of conditionals and loops is actually any single statement, and `{ ... }` is just a kind of statement (unlike some other languages where the braces are syntax of the conditional/loop statement itself).

As a style choice, I'd advocate saying it should be used only before either a `{` or a `;` (empty statement) rather than before any other statement.  This implies that using it for an if or loop construct requires using braces for the body even if the style otherwise allows single-statement bodies without braces.  i.e. `if (foo) [[unlikely]] bar();` should instead be:
```

if (foo) [[unlikely]] {

  bar();

}

```

(In the codebase I work most in, we've decided just to make braces mandatory for all conditional/loop bodies anyway, but I've lost track of Chromium's style rule about that.)

[Adam Rice]

I have heard rumours that overuse of __builtin_expect() can lead to suboptimal compiler output. So probably the same applies to [[likely]] and [[unlikely]] and we should not get carried away with them.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAB%3D4xhrr1gEnjXss2gAM%3DXvrUhg9xUWoSN49v%3DViCYTaHZHSOw%40mail.gmail.com.

[Mark Mentovai]

Do we really want __builtin_expect or [[likely]]/[[unlikely]] at all? Barring special circumstances, isn’t this something that our build should be taking care of for us, via (for example) PGO?

C++20 contains an admonition (§9.12.6, dcl.attr.likelihood):

[Note: Excessive usage of either of these attributes is liable to result in performance degradation. — end note]

There’s a CppCon 2022 talk on the topic that goes into quite a bit of detail. Much of that detail is GCC-specific, but that’s because Clang didn’t show any difference in output in the bulk of the tests that they did (22:15).

Before adopting this or engaging in a conversion project, we should think at a higher level about whether these types of hints are broadly appropriate for our codebase.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAC_ixdzEBtFZAG61%2BtTJG4%2B4p0V3pxUyq_ZS_n3VRmiNTJX9Hw%40mail.gmail.com.

[Leszek Swirski]

If discouraging manual annotations is a direction we want to go, then I'd be interested in seeing whether there's some way we could codify PGO data so that it's available (to some extent) in non-PGO builds. While PGO helps us squeeze out the performance we'd get with manual annotations (and more!) our released binaries, it's a nightmare for A/B testing of performance in the lab; this means that, in practice, we have to look at non-PGO builds when running commit-level regression tests or pinpoint analyses. There, having at least some very certain known likely/unlikely cases annotated can make a big difference to the realism of the comparison.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHk1GDx98zYrv1U3Q80XzCceKLEj7ws3%2BKaxiJwwcf_xnJ_uCA%40mail.gmail.com.

[Peter Kasting]

I think this is a very fair question, but I think it might be orthogonal to whether we want to convert existing users of our macros to use the c++20-compliant attribute spellings.

_If_ they actually behave the same as __builtin_expect, then I think we do want that -- we'll eliminate non standard spellings and placements and potentially aid non pgo builds, and we'll probably be able to do so sooner than we can resolve your question, which I suspect requires research none of us are likely to put in.

And there are at least a few cases (notreached annotations, tracing macros that are only enabled when a debug flag is set) where use of these feels like it ought to be safe and correct. Those seem distinct from the "trying to help the branch predictor in a hot loop" questionable case.

PK

[Peter Boström]

For my case I'd pretty much only be interested in making the failing branches of CHECK unlikely and NOTREACHEDs being reached. They'll both eventually be understood as noreturn which imo should imply that they're optimized equally though I have no idea if that's remotely true.

[danakj]

On Sat, Mar 9, 2024 at 5:57 PM Adam Rice <ri...@chromium.org> wrote:

I have heard rumours that overuse of __builtin_expect() can lead to suboptimal compiler output. So probably the same applies to [[likely]] and [[unlikely]] and we should not get carried away with them.

__builtin_assume() can cause bad performance changes in llvm by interfering with optimization passes, as can llvm.expect/__builtin_expect. So yes we should not start using these in lots of places. They should be in places that are nearly-dead code, or because we can observe a win from them, which can happen. But it will be uncommon.

likely and unlikely are useful for branches that we will never take without terminating (there's a NOTREACHED_NORETURN inside) or, inside terminating things like the CHECK macro. PGO does a better job, in that it's based on what we actually see happen in benchmarks, so the [[likely]] tool is not meant for "i believe we'll usually end up in this path" in a general sense. It really is for "we should ~never go here".

Since all of our A/B testing and performance work is done _without_ PGO (for good reasons Leszek talks about as well), these attributes are not totally useless, they are worth using to skip over what would be ~dead code, but they really should be rare too.

It's not like these are particularly new ideas for us, we've had LIKELY/UNLIKELY macros in base for a long time. We already use the builtins via macros, and switching to the standard thing SGTM.

+1 to using them before {} on if/else.

[Mark Mentovai]

If this were a trivial find-and-replace-type rewrite, sure. But __builtin_expect and the existing LIKELY and UNLIKELY macros work differently than the [[likely]] and [[unlikely]] attributes. I don’t know if we can justify a more involved rewrite without also answering whether this is something that we want to keep doing.

[danakj]

On Mon, Mar 11, 2024 at 2:32 PM Mark Mentovai <ma...@chromium.org> wrote:

If this were a trivial find-and-replace-type rewrite, sure. But __builtin_expect and the existing LIKELY and UNLIKELY macros work differently than the [[likely]] and [[unlikely]] attributes. I don’t know if we can justify a more involved rewrite without also answering whether this is something that we want to keep doing.

I thought __builtin_expect is the same, but I see the LLVM output of clang differs, and is worse with [[unlikely]]: https://godbolt.org/z/4feEzxfjK

[danakj]

On Mon, Mar 11, 2024 at 2:44 PM danakj <dan...@chromium.org> wrote:

On Mon, Mar 11, 2024 at 2:32 PM Mark Mentovai <ma...@chromium.org> wrote:

If this were a trivial find-and-replace-type rewrite, sure. But __builtin_expect and the existing LIKELY and UNLIKELY macros work differently than the [[likely]] and [[unlikely]] attributes. I don’t know if we can justify a more involved rewrite without also answering whether this is something that we want to keep doing.

I thought __builtin_expect is the same, but I see the LLVM output of clang differs, and is worse with [[unlikely]]: https://godbolt.org/z/4feEzxfjK

Wait no, sorry, I broke my example: https://godbolt.org/z/5PdnoxPMq

I can't see what is different with __builtin_expect vs [[unlikely]], but I am also failing to get llvm.expect in the IR at all.

[Joe Mason]

Given that the semantics of [[likely]] and [[unlikely]] are different than LIKELY and UNLIKELY, I suggest doing an auto-rename of LIKELY and UNLIKELY to something that doesn't conflict with the standard names (eg. LIKELY_RESULT and UNLIKELY_RESULT since it's used around expression results), and encouraging the standard attributes for any new usage but not migrating old usages (except in code that's being updated anyway).

Since the bar for using these is high, the existing uses are probably in code that's performance-critical and subtle, so I don't think we can justify the risk of updating it just for consistency.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAB%3D4xhpZq%3DgEi%3D%2Bzb39jANZDTsh69g4K12dD5Scz_NBfYeKieQ%40mail.gmail.com.

[danakj]

On Mon, Mar 11, 2024 at 3:18 PM Joe Mason <joenot...@google.com> wrote:

Given that the semantics of [[likely]] and [[unlikely]] are different than LIKELY and UNLIKELY, I suggest doing an auto-rename of LIKELY and UNLIKELY to something that doesn't conflict with the standard names (eg. LIKELY_RESULT and UNLIKELY_RESULT since it's used around expression results), and encouraging the standard attributes for any new usage but not migrating old usages (except in code that's being updated anyway).

Since the bar for using these is high, the existing uses are probably in code that's performance-critical and subtle, so I don't think we can justify the risk of updating it just for consistency.

I think I am missing something. I don't see where we get that [[likely]] and [[unlikely]] are different (for the same compiler, they are different between clang and gcc). They should result in the same thing that __builtin_expect() does, which is normally nothing at all. It seems like what is new is that they can appear in more places (such as on switch cases), where before you'd need to put a __builtin_expect() above the switch instead to tell the compiler such things.

I tried looking (quickly) through the talk for some hint but I see them saying the same thing, or did I miss it?

[Peter Kasting]

On Mon, Mar 11, 2024 at 12:34 PM danakj <dan...@chromium.org> wrote:

On Mon, Mar 11, 2024 at 3:18 PM Joe Mason <joenot...@google.com> wrote:

Given that the semantics of [[likely]] and [[unlikely]] are different than LIKELY and UNLIKELY, I suggest doing an auto-rename of LIKELY and UNLIKELY to something that doesn't conflict with the standard names (eg. LIKELY_RESULT and UNLIKELY_RESULT since it's used around expression results), and encouraging the standard attributes for any new usage but not migrating old usages (except in code that's being updated anyway).

Since the bar for using these is high, the existing uses are probably in code that's performance-critical and subtle, so I don't think we can justify the risk of updating it just for consistency.

I think I am missing something. I don't see where we get that [[likely]] and [[unlikely]] are different (for the same compiler, they are different between clang and gcc). They should result in the same thing that __builtin_expect() does, which is normally nothing at all. It seems like what is new is that they can appear in more places (such as on switch cases), where before you'd need to put a __builtin_expect() above the switch instead to tell the compiler such things.

They can appear in different places -- some places where __builtin_expect() is valid, [[likely]] would not be, and vice versa. It's not a trivial rename, but it's a pretty brainless mechanical one. I've done it before for half a dozen other similar macro cases that switched __attribute__(()) to [[]]. This shouldn't be a blocker to allowing a rename.

PK

[Joe Mason]

On Mon, Mar 11, 2024 at 3:34 PM danakj <dan...@chromium.org> wrote:

I think I am missing something. I don't see where we get that [[likely]] and [[unlikely]] are different (for the same compiler, they are different between clang and gcc). They should result in the same thing that __builtin_expect() does, which is normally nothing at all. It seems like what is new is that they can appear in more places (such as on switch cases), where before you'd need to put a __builtin_expect() above the switch instead to tell the compiler such things.

I thought from what Roland said: 

 

On Fri, Mar 8, 2024 at 3:19 PM Roland McGrath <mcgr...@chromium.org> wrote:

The way these attributes are used is somewhat new and different from the old `__builtin_expect` that underlies past macro approaches. It doesn't go on an expression used in a conditional. Instead, it precedes a statement syntactically and says any way of reaching that path is likely/unlikely.

That LIKELY / UNLIKELY was applied to an expression and annotated whether the result was likely to be `true`, while [[likely]] and [[unlikely]] was applied to a statement and annotated whether it was likely to be executed. That seems like a different enough usage that having both named almost the same thing would be confusing, which is why I suggested renaming LIKELY / UNLIKELY.

The other point is that, as you said, it SHOULD result in the same thing, but is it worth doing the work to verify that?

[danakj]

I guess I see these as basically the same. The macros worked on an expression that is used as a branch point. The attributes mark the branch itself, they both say which way a branch is going to go ~all the time.

The other point is that, as you said, it SHOULD result in the same thing, but is it worth doing the work to verify that?

Without any evidence to the contrary I don't know why we'd think they do something different and need to dig deeper. The attributes are a standardization of the builtin. We can run pinpoint against any rewrite if we wanted to as well, just to be sure, and that seems like enough rigor for any other change?

[Peter Kasting]

On Mon, Mar 11, 2024 at 11:47 AM danakj <dan...@chromium.org> wrote:

On Mon, Mar 11, 2024 at 2:44 PM danakj <dan...@chromium.org> wrote:

On Mon, Mar 11, 2024 at 2:32 PM Mark Mentovai <ma...@chromium.org> wrote:

If this were a trivial find-and-replace-type rewrite, sure. But __builtin_expect and the existing LIKELY and UNLIKELY macros work differently than the [[likely]] and [[unlikely]] attributes. I don’t know if we can justify a more involved rewrite without also answering whether this is something that we want to keep doing.

I thought __builtin_expect is the same, but I see the LLVM output of clang differs, and is worse with [[unlikely]]: https://godbolt.org/z/4feEzxfjK

Wait no, sorry, I broke my example: https://godbolt.org/z/5PdnoxPMq

I can't see what is different with __builtin_expect vs [[unlikely]], but I am also failing to get llvm.expect in the IR at all.

I think the IR viewer on godbolt must not be showing everything, because by looking only at the generated assembly, I can see that tweaking between "[[likely]]", "[[unlikely]]", and no annotation does indeed produce different effects.

I verified that [[likely]] and [[unlikely]] produce the same effects on the generated assembly as __builtin_expect(..., true) and ...false). To me this is sufficiently conclusive: we're not going to change anything about our existing codegen by switching to these attributes, we're just going to spell them in a standards-compliant way and at a compliant place in the statement.

+1 to making this change from me.

PK

[Nico Weber]

I'll note that thanks to https://reviews.llvm.org/D134456 , PGO data wins over the annotation when both are present. (That used to not be the case!) So at least the annotations shouldn't hurt.

Mild +1 to Mark's point that we should drop most of these annotations since in practice they're often wrong (there's a diagnostic for when it doesn't match PGO data, but the diagnostic itself wasn't quite right when we tried it: crbug.com/40264987) and even when right often don't help a lot.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFA-TJibgbVDw%2BBZnxbGWRjOWVQ6UFqnwgXx7xEy5hZusQ%40mail.gmail.com.

[danakj]

On Tue, Mar 12, 2024 at 12:06 PM Nico Weber <tha...@chromium.org> wrote:

I'll note that thanks to https://reviews.llvm.org/D134456 , PGO data wins over the annotation when both are present. (That used to not be the case!) So at least the annotations shouldn't hurt.

This is very nice to see.

[Peter Kasting]

I'm going to call the question.

I think we should allow these and do a mechanical change that rewrites existing uses (to a form that still compiles, which will sometimes involve moving where in the statement they appear) and eliminates the macros. This is monotonically better than our current state. It will not change our current codegen (which seems to be one of the main concerns above), and it is standardized and readable.

For cost/benefit reasons, I do not propose auditing existing uses to remove things that "don't seem necessary", unless someone is motivated to do that themselves.

Please reply in the next week with your thoughts.

PK

[Roland McGrath]

I'd love to see that rewriting done as a reusable clang-tidy fixit that's generic/configurable and upstreamed so as not to apply only to chromium.

[Peter Kasting]

On Thu, Apr 11, 2024 at 10:42 AM Roland McGrath <mcgr...@chromium.org> wrote:

I'd love to see that rewriting done as a reusable clang-tidy fixit that's generic/configurable and upstreamed so as not to apply only to chromium.

In general, I'd love for clang-tidy to do more, and it's a longstanding goal of mine to eventually have some folks who know enough about all facets of that to write and upstream our own clang-tidy transforms. Right now we lack that expertise, and I don't think we should block this decision on gaining it.

PK

[Brian Osman]

On Monday, March 11, 2024 at 12:17:38 PM UTC-4 danakj wrote:

likely and unlikely are useful for branches that we will never take without terminating (there's a NOTREACHED_NORETURN inside) or, inside terminating things like the CHECK macro. PGO does a better job, in that it's based on what we actually see happen in benchmarks, so the [[likely]] tool is not meant for "i believe we'll usually end up in this path" in a general sense. It really is for "we should ~never go here".

Since all of our A/B testing and performance work is done _without_ PGO (for good reasons Leszek talks about as well), these attributes are not totally useless, they are worth using to skip over what would be ~dead code, but they really should be rare too

 

Aside, going back to this point: Is this aspirational, or something we should actually be considering in the code today? I happen to be editing a file right now that includes >50 annotated branches, none of which actually lead to NOTREACHED/CHECK -- they're just trying to second-guess the branch predictor.

[danakj]

Maybe there's some measurements that show they were the right thing to do? But I would be suspicious of such usage without documentation explaining how it came to be seen as loadbearing.

[David Benjamin]

I think this is orthogonal to the spelling question though. As long as it is sometimes appropriate to use likely/unlikely, even if rare, we'll want to have a spelling for it. The standard spelling seems preferable to the macro version, so let's migrate that.

Separately, it may well be the case that some, or even most, of our existing uses are not appropriate. But I think we can change the syntax en masse and worry about that separately if someone feels inspired.

I.e., I'm +1 to Peter's proposal.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaSOV3pad2WMb_qGw0FKUouDRSrfCEiGP0YqqYh01NvW_w%40mail.gmail.com.

[danakj]

On Thu, Apr 11, 2024 at 2:12 PM David Benjamin <davi...@chromium.org> wrote:

I think this is orthogonal to the spelling question though. As long as it is sometimes appropriate to use likely/unlikely, even if rare, we'll want to have a spelling for it. The standard spelling seems preferable to the macro version, so let's migrate that.

Separately, it may well be the case that some, or even most, of our existing uses are not appropriate. But I think we can change the syntax en masse and worry about that separately if someone feels inspired.

I.e., I'm +1 to Peter's proposal.

Yes, +1

[Peter Kasting]

On Thursday, April 11, 2024 at 10:38:26 AM UTC-7 pkas...@google.com wrote:

I'm going to call the question.

I think we should allow these and do a mechanical change that rewrites existing uses (to a form that still compiles, which will sometimes involve moving where in the statement they appear) and eliminates the macros. This is monotonically better than our current state. It will not change our current codegen (which seems to be one of the main concerns above), and it is standardized and readable.

For cost/benefit reasons, I do not propose auditing existing uses to remove things that "don't seem necessary", unless someone is motivated to do that themselves.

Please reply in the next week with your thoughts.

It's been 2.5 weeks since this, and I've seen a couple explicit +1s and no objections. I consider this approved. Someone is welcome to implement the above; if no one does I will hopefully eventually get to it.

PK