Likely unlikely fix #120370

x17jiri · 2024-01-26T10:54:14Z

RFC 1131 ( #26179 ) added likely/unlikely intrinsics, but they have been broken for a while: #96276 , #96275 , #88767 . This PR tries to fix them.

Changes:

added a new cold_path() intrinsic
likely() and unlikely() changed to regular functions implemented using cold_path()

rustbot · 2024-01-26T10:54:22Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2024-01-26T10:54:24Z

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino, @ouz-a

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

RalfJung · 2024-01-26T10:56:19Z

compiler/rustc_middle/src/mir/syntax.rs

+    True,  // condition is probably true
+    False, // condition is probably false
+    Unpredictable,


Please make these doc comments, and also explain the last variant.

RalfJung · 2024-01-26T10:57:50Z

compiler/rustc_codegen_llvm/src/builder.rs

+                    // 'then' branch weight
+                    self.cx.const_u32(if expect == ExpectKind::True { 2000 } else { 1 }),
+                    // 'else' branch weight
+                    self.cx.const_u32(if expect == ExpectKind::True { 1 } else { 2000 }),


This is what Clang emits. I will add a comment

RalfJung · 2024-01-26T10:58:28Z

compiler/rustc_codegen_gcc/src/intrinsic/mod.rs

-                }
-                sym::unlikely => {
-                    self.expect(args[0].immediate(), false)
-                }


There are similar match arms here that can be cleaned up now.

EDIT: Ah, you already did. :)

bjorn3 · 2024-01-26T11:13:39Z

compiler/rustc_codegen_ssa/src/mir/statement.rs

                let op_val = self.codegen_operand(bx, op);
                bx.assume(op_val.immediate());
            }
+            mir::StatementKind::Intrinsic(box NonDivergingIntrinsic::Expect(..)) => {}


Could you please also add this at

rust/compiler/rustc_codegen_cranelift/src/base.rs

Line 817 in 69db514

NonDivergingIntrinsic::Assume(_) => {}

?

Sure. Normally, only the LLVM backend is build, right? Because I didn't get any compile time errors in GCC or Cranelift

./x.py check should check all backends. ./x.py build only builds backends other than LLVM if you tell the build system to with the rust.codegen-backends option in config.toml.

Thanks, I updated this and one more match in Cranelift

rustbot · 2024-01-26T12:59:48Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

cjgillot · 2024-01-26T22:47:02Z

compiler/rustc_codegen_ssa/src/mir/block.rs

+                    }
+                } else {
+                    ExpectKind::None
+                };


Nit: could this be a Option<mir::ExpectKind>?

Good point. Updated

cjgillot · 2024-01-26T22:47:45Z

compiler/rustc_codegen_ssa/src/mir/block.rs

            let lltrue = helper.llbb_with_cleanup(self, target);
            let llfalse = helper.llbb_with_cleanup(self, targets.otherwise());
            if switch_ty == bx.tcx().types.bool {
+                let expect = if let Some(x) = self.mir[bb].statements.last()


What if the Expect is not the last statement? This could happen because of MIR opts.

Could it also be moved to a different BB? Or is that something worth considering?

I was also thinking it could be useful to emit a warning if there is Expect which cannot be paired with a branch.

Updated to handle expect anywhere in a BB

cjgillot · 2024-01-26T22:53:12Z

compiler/rustc_middle/src/mir/syntax.rs

    Assume(Operand<'tcx>),

+    // Denotes a call to the intrinsic functions `likely`, `unlikely` and `unpredictable`.
+    Expect(Operand<'tcx>, ExpectKind),


For MIR opts:

SimplifyLocals: should this statement be removed when Operand is an unused local?

DeadStoreElimination: when the operand is a local that is never read again?

SimplifyConstCondition: when the operand if a constant?

Opt passes updated

cjgillot · 2024-01-26T22:53:51Z

compiler/rustc_middle/src/mir/syntax.rs

+    HashStable,
+    TypeFoldable,
+    TypeVisitable
+)]


Nit: please split the derives in 2 lines.

Do you mean like this?

#[derive( Clone, TyEncodable, TyDecodable, Debug, PartialEq, Hash, HashStable, TypeFoldable, TypeVisitable )]

I mean:

#[derive(Clone, TyEncodable, TyDecodable, Debug, PartialEq)] #[derive(Hash, HashStable, TypeFoldable, TypeVisitable)]

To avoid rustfmt using a dozen lines.

x17jiri · 2024-01-29T13:06:18Z

@rustbot ready

rustbot · 2024-02-07T14:27:19Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

12a1bb3

x17jiri · 2024-02-08T16:53:49Z

@cjgillot I found I introduced a bug in simplify_branches.rs 🤦 It should be fixed now.

Anyway, the number of affected files is getting quite big. If there is anything I can do to make the review easier, please let me know

Noratrieb

small style nit (in case you're touching this code again, if you aren't it's fine to not implement it)

Noratrieb · 2024-02-15T18:16:18Z

compiler/rustc_middle/src/mir/syntax.rs

+    /// condition is probably true
+    True,
+
+    /// condition is probably false
+    False,
+
+    /// condition is unpredictable by hardware
+    Unpredictable,


Suggested change

/// condition is probably true

True,

/// condition is probably false

False,

/// condition is unpredictable by hardware

Unpredictable,

/// Condition is probably true

True,

/// Condition is probably false

False,

/// Condition is unpredictable by hardware

Unpredictable,

bors · 2024-02-16T11:59:44Z

☔ The latest upstream changes (presumably #120500) made this pull request unmergeable. Please resolve the merge conflicts.

klensy · 2024-11-18T10:10:20Z

I think this fix will be nice to have in relnotes?

RalfJung · 2024-11-18T10:23:37Z

This is completely internal to the standard library implementation. Why would the relnotes mention it, and what would they say? We typically only document things that users need to be aware of. "Some functions are now hopefully a bit faster" doesn't go into relnotes.

Kobzol · 2024-11-18T10:26:35Z

If the likely/unlikely intrinsics were broken before, and now they are fixed, then that information could be quite important to some users. Both to let them know that they can now again rely on these intrinsics doing something, and also to let them know that on previous versions of Rust it probably did nothing.

Assuming that this PR is about the unstable likely/unlikely intrinsics that are available to nightly users, and thus it's not completely internal to the standard library.

RalfJung · 2024-11-18T10:28:56Z

Relnotes generally don't mention changes to nightly features either. They document what changed with a stable release.

klensy · 2024-11-18T10:28:59Z

Someone used them to tune some perf and now they see that this actually only started working as expected, so less surprises.

RalfJung · 2024-11-18T10:29:50Z

Given that this is on nightly now, and relnotes will be put out in ~7 weeks, it also wouldn't make a lot of sense to attach this to the Rust 1.84 relnotes.

Kobzol · 2024-11-18T10:30:53Z

Right, I meant the detailed release notes, which also mention internal changes and interesting pieces of information like this, not the main blog post.

RalfJung · 2024-11-18T10:33:36Z

How often do these detailed release notes talk about nightly features? In my understand, not very often. But maybe I misremember.

klensy · 2024-11-18T10:34:43Z

Personally, i think that release notes also should contain block for nightly changes, i.e. new added\removed unstable features to help users discover them, but this not related to current PR.

RalfJung · 2024-11-18T10:36:42Z

That would announce such changes on average 9 weeks too late. I don't think it makes a lot of sense.

klensy · 2024-11-18T10:44:18Z

That would announce such changes on average 9 weeks too late. I don't think it makes a lot of sense.

With release cycle of 6 weeks this will be only <= 6 weeks if placed in nearest relnotes.

How currently users can discover new features (before stabilization), excluding ones that manually track this repo? This gives lot less possible testing for that features. Or i miss something?

RalfJung · 2024-11-18T10:50:31Z

Such updates can and often are posted to the tracking issues, and we expect interested users to follow those.

With release cycle of 6 weeks this will be only <= 6 weeks if placed in nearest relnotes.

That would be really strange, since the codebase that is released there doesn't contain these changes.

If we want to do something like this IMO it should be a completely separate "nightly feature release notes" announcement, not part of the regular release notes. It doesn't even have to use the same 6 week cadence.

rustbot assigned cjgillot Jan 26, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 26, 2024

RalfJung reviewed Jan 26, 2024

View reviewed changes

bjorn3 reviewed Jan 26, 2024

View reviewed changes

cjgillot reviewed Jan 26, 2024

View reviewed changes

cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 29, 2024

x17jiri mentioned this pull request Feb 3, 2024

#[cold] on match arms #120193

Open

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 7, 2024

x17jiri force-pushed the likely_unlikely_fix branch from 12a1bb3 to 2d49c41 Compare February 7, 2024 14:35

rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 7, 2024

This comment has been minimized.

Sign in to view

Noratrieb reviewed Feb 15, 2024

View reviewed changes

x17jiri force-pushed the likely_unlikely_fix branch from e4cc72b to d31b045 Compare February 17, 2024 07:36

This comment has been minimized.

Sign in to view

saethlin mentioned this pull request Nov 18, 2024

Meta tracking issue for branch hints (RFC 1131) #26179

Open

x17jiri mentioned this pull request Dec 4, 2024

improve cold_path() #133852

Merged

x17jiri mentioned this pull request Dec 21, 2024

Export cold_path() in std::hint rust-lang/libs-team#510

Closed

This was referenced Feb 10, 2025

parallel: rustc_data_structures: sync: implement RwLock as enum { Sync(parking_lot::RwLock), NoSync(RefCell) } #136772

Closed

Full Compile-Time Processor Feature Detection BurntSushi/memchr#160

Open

tgross35 mentioned this pull request Jul 17, 2025

Tracking Issue for likely_unlikely and cold_path #136873

Open

5 tasks

DaniPopes mentioned this pull request Jul 26, 2025

Re-implement likely/unlikely with #[cold] rust-lang/hashbrown#631

Merged

Likely unlikely fix #120370

Likely unlikely fix #120370

Uh oh!

Conversation

x17jiri commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jan 26, 2024

Uh oh!

rustbot commented Jan 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Jan 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

x17jiri commented Jan 29, 2024

Uh oh!

rustbot commented Feb 7, 2024

Uh oh!

This comment has been minimized.

x17jiri commented Feb 8, 2024

Uh oh!

Noratrieb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Feb 16, 2024

Uh oh!

This comment has been minimized.

klensy commented Nov 18, 2024

Uh oh!

RalfJung commented Nov 18, 2024

Uh oh!

Kobzol commented Nov 18, 2024

Uh oh!

RalfJung commented Nov 18, 2024

Uh oh!

klensy commented Nov 18, 2024

Uh oh!

x17jiri commented Jan 26, 2024 •

edited

Loading

RalfJung Jan 26, 2024 •

edited

Loading