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

@@ -70,6 +70,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                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

@@ -328,10 +329,26 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
            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

@@ -429,6 +451,9 @@ pub enum NonDivergingIntrinsic<'tcx> {
    /// If the argument is `false`, this operation is equivalent to `TerminatorKind::Unreachable`.
    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.

Amanieu · 2024-08-01T18:59:15Z

library/core/src/intrinsics.rs

 pub const fn likely(b: bool) -> bool {
-    b
+    #[cfg(any(bootstrap, miri))]


There shouldn't be any need to special-case miri here, just bootstrap is enough.

Without the special-case, I'm getting error in tests/pass/shims/time-with-isolation.rs

It reports "The loop took around 13s" instead of 12. I'm not sure why that is, because likely/unlikely are not used anywhere in the test

The next line in that test is:

println!("(It's fine for this number to change when you `--bless` this test.)")

So it's ok to bless the new number? I didn't want to slow things down.

Yes, it's fine.

Amanieu · 2024-08-01T19:00:33Z

library/core/src/intrinsics.rs

+#[cfg(not(bootstrap))]
+#[rustc_nounwind]
+#[miri::intrinsic_fallback_is_spec]
+#[cold]


This isn't marked as inline, so can you add a test to check that the call to cold_path is properly eliminated from the output IR?

I added CHECK-NOT to the tests and verified it fails when the call is not eliminated

As you have noticed the implementation requires that Rust's own inline pass does not inline an empty cold function, but the LLVM's inline pass does.

At the moment this works. If the behavior changed, we would have to make cold_path() a real intrinsic. It would be a no-op that's ignored by all mir passes and backends, but is detected by the find_cold_blocks() function.

I didn't implement this change yet in order to keep the pull request small and simple.

You can just an an empty implementation of the intrinsic in compiler/rustc_codegen_ssa/src/mir/intrinsic.rs to avoid emitting a call to the cold_path function during codegen.

I added the empty implementation of cold_path() to compiler/rustc_codegen_ssa/src/mir/intrinsic.rs

I also removed the #[rustc_intrinsic] from likely and unlikely. They are ordinary functions now. I found that Rust's inliner has several checks to avoid inlining intrinsics. But these functions should really be inlined.

Amanieu · 2024-08-01T19:06:27Z

Overall I like the cleanness of the design based on the cold_path intrinsic. Perhaps that is a way we could stabilize it through a hint in core::hint.

rustbot · 2024-08-01T20:59:07Z

The Miri subtree was changed

cc @rust-lang/miri

compiler/rustc_codegen_ssa/src/mir/intrinsic.rs

bors · 2024-08-20T01:54:35Z

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

x17jiri · 2024-08-27T16:08:17Z

Rebased

Amanieu · 2024-08-28T23:10:37Z

@cjgillot Are you still reviewing this or does this need to be reviewed by the compiler team?

bors · 2024-09-06T03:21:34Z

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

bors · 2024-09-17T22:23:21Z

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

cjgillot · 2024-09-21T12:01:37Z

compiler/rustc_codegen_ssa/src/mir/block.rs

+            let true_cold = self.cold_blocks[target];
+            let false_cold = self.cold_blocks[otherwise];


target_cold and otherwise_cold?

cjgillot · 2024-09-21T12:03:53Z

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());
+            let llfalse = helper.llbb_with_cleanup(self, otherwise);


Could we take the opportunity to rename those to lltarget and llotherwise?

cjgillot · 2024-09-21T12:03:56Z

compiler/rustc_codegen_ssa/src/mir/block.rs

+            let otherwise = targets.otherwise();
+            let true_cold = self.cold_blocks[target];
+            let false_cold = self.cold_blocks[otherwise];
+            let expect = if true_cold != false_cold { Some(false_cold) } else { None };


I had to scratch my head for quite some time to understand why you wrote it this way. IIUC:

if true_cold == false_cold, both branches have the same weight, so we don't emit anything;

if false_cold and 1 => target we expect to go to lltrue, so we expect is Some(true);

...

This definitely needs some comment.

That is correct. Commend added

cjgillot · 2024-09-21T12:08:18Z

@x17jiri sorry for the delay. Do you mind squashing the commits?

@Amanieu thanks. I could work this a second look by somebody more experienced with codegen.

x17jiri · 2024-09-21T21:35:58Z

Rebased and squashed commits

x17jiri · 2024-09-22T06:19:12Z

I edited the PR description to reflect the redesign

bors · 2024-11-08T07:05:48Z

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

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

x17jiri force-pushed the likely_unlikely_fix branch from 8a5b474 to 436c4fd Compare August 1, 2024 16:04

Amanieu reviewed Aug 1, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

bjorn3 reviewed Aug 3, 2024

View reviewed changes

compiler/rustc_codegen_ssa/src/mir/intrinsic.rs Show resolved Hide resolved

Dylan-DPC 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 Aug 6, 2024

x17jiri force-pushed the likely_unlikely_fix branch from e1cb8ca to 8096ed5 Compare August 27, 2024 14:50

x17jiri force-pushed the likely_unlikely_fix branch from 8096ed5 to d6396ee Compare September 16, 2024 06:47

cjgillot reviewed Sep 21, 2024

View reviewed changes

x17jiri force-pushed the likely_unlikely_fix branch from 8c4f8fc to 54da6f7 Compare September 21, 2024 21:02

Likely unlikely fix

383de54

x17jiri force-pushed the likely_unlikely_fix branch from 54da6f7 to 383de54 Compare September 21, 2024 21:32

kennytm mentioned this pull request Oct 23, 2024

ACP: add bool::select rust-lang/libs-team#468

Closed

Amanieu mentioned this pull request Nov 6, 2024

Further sequester Group/Tag code rust-lang/hashbrown#568

Open

		let true_cold = self.cold_blocks[target];
		let false_cold = self.cold_blocks[otherwise];

Likely unlikely fix #120370

Are you sure you want to change the base?

Likely unlikely fix #120370

Conversation

x17jiri commented Jan 26, 2024 • edited Loading

rustbot commented Jan 26, 2024

rustbot commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x17jiri commented Jan 29, 2024

rustbot commented Feb 7, 2024

This comment has been minimized.

x17jiri commented Feb 8, 2024

Noratrieb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 16, 2024

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amanieu commented Aug 1, 2024

This comment has been minimized.

rustbot commented Aug 1, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented Aug 20, 2024

x17jiri commented Aug 27, 2024

Amanieu commented Aug 28, 2024

bors commented Sep 6, 2024

bors commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x17jiri Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

cjgillot commented Sep 21, 2024

x17jiri commented Sep 21, 2024

x17jiri commented Sep 22, 2024

bors commented Nov 8, 2024

x17jiri commented Jan 26, 2024 •

edited

Loading

RalfJung Jan 26, 2024 •

edited

Loading

x17jiri Sep 21, 2024 •

edited

Loading