Skip to content

Commit d874dce

Browse files
committed
Auto merge of #150386 - saethlin:no-const-assert-likely, r=tgross35
Remove the explicit branch hint from const_panic This was asked for in PR review for equivalence with `assert!`, but we don't need likely/unlikely intrinsics here (and indeed they are a bit of an antipattern), because codegen knows about the pattern that `assert!` produces where only one target of a SwitchInt leads to a block terminated by a `#[cold]` call. All our panic entrypoints are `#[cold]`. `assert!` does not use the likely/unlikely intrinsics, maybe it did in the past. So the intrinsic call in `const_assert!` should be completely unnecessary. But currently it isn't, because `const_panic!` stamps out a runtime wrapper for its entrypoint which is not given `#[inline]`, citing compile times. It's not obvious to me why not using `#[inline]` would even be good in this case. I think the runtime wrapper should have `#[inline]` or `#[cold]`, so I'm going to do 3 perf experiments here. Just changing `const_assert!`: https://perf.rust-lang.org/compare.html?start=fabece9e9491d0a3c3655dba488881968b7c5f7a&end=f6b082cfcf880ad80b33523d5047c11654d907a8&stat=instructions:u Also adding `#[inline]` to `const_panic!`'s runtime wrapper: https://perf.rust-lang.org/compare.html?start=fabece9e9491d0a3c3655dba488881968b7c5f7a&end=f467cc7e49b74b47da0e2cde49fc1a3140e00ff6&stat=instructions:u Also adding `#[cold]` to `const_panic!`'s runtime wrapper: https://perf.rust-lang.org/compare.html?start=2e854a9344154564259de51385e9ec9506c0f3b7&end=fd8e098558ff08ac002b05ff78e9f91ffa6f3738&stat=instructions:u The previous comment indicated perf would be worse with `#[inline]` on the runtime branch, but the results in this PR show that it is more mixed, and net positive. And in fact, the only regression in serde seems to be because we created a very tiny additional amount of codegen, but pushed the size of the second CGU just high enough that we no longer merge all CGUs together. So now serde (in debug with incremental disabled) has 2 CGUs instead of 1. If we just so happened to be still below the threshold where the CGUs don't merge, adding `#[inline]` would have been nearly all improvements.
2 parents b082bd5 + 315646a commit d874dce

File tree

3 files changed

+22
-31
lines changed

3 files changed

+22
-31
lines changed

library/core/src/intrinsics/mod.rs

Lines changed: 3 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2402,8 +2402,7 @@ where
24022402
/// The `@capture` block declares which surrounding variables / expressions can be
24032403
/// used inside the `if const`.
24042404
/// Note that the two arms of this `if` really each become their own function, which is why the
2405-
/// macro supports setting attributes for those functions. The runtime function is always
2406-
/// marked as `#[inline]`.
2405+
/// macro supports setting attributes for those functions. Both functions are marked as `#[inline]`.
24072406
///
24082407
/// See [`const_eval_select()`] for the rules and requirements around that intrinsic.
24092408
pub(crate) macro const_eval_select {
@@ -2413,35 +2412,14 @@ pub(crate) macro const_eval_select {
24132412
$(#[$compiletime_attr:meta])* $compiletime:block
24142413
else
24152414
$(#[$runtime_attr:meta])* $runtime:block
2416-
) => {
2417-
// Use the `noinline` arm, after adding explicit `inline` attributes
2418-
$crate::intrinsics::const_eval_select!(
2419-
@capture$([$($binders)*])? { $($arg : $ty = $val),* } $(-> $ret)? :
2420-
#[noinline]
2421-
if const
2422-
#[inline] // prevent codegen on this function
2423-
$(#[$compiletime_attr])*
2424-
$compiletime
2425-
else
2426-
#[inline] // avoid the overhead of an extra fn call
2427-
$(#[$runtime_attr])*
2428-
$runtime
2429-
)
2430-
},
2431-
// With a leading #[noinline], we don't add inline attributes
2432-
(
2433-
@capture$([$($binders:tt)*])? { $($arg:ident : $ty:ty = $val:expr),* $(,)? } $( -> $ret:ty )? :
2434-
#[noinline]
2435-
if const
2436-
$(#[$compiletime_attr:meta])* $compiletime:block
2437-
else
2438-
$(#[$runtime_attr:meta])* $runtime:block
24392415
) => {{
2416+
#[inline]
24402417
$(#[$runtime_attr])*
24412418
fn runtime$(<$($binders)*>)?($($arg: $ty),*) $( -> $ret )? {
24422419
$runtime
24432420
}
24442421

2422+
#[inline]
24452423
$(#[$compiletime_attr])*
24462424
const fn compiletime$(<$($binders)*>)?($($arg: $ty),*) $( -> $ret )? {
24472425
// Don't warn if one of the arguments is unused.

library/core/src/panic.rs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -166,10 +166,9 @@ pub macro const_panic {
166166
const fn do_panic($($arg: $ty),*) -> ! {
167167
$crate::intrinsics::const_eval_select!(
168168
@capture { $($arg: $ty = $arg),* } -> !:
169-
#[noinline]
170-
if const #[track_caller] #[inline] { // Inline this, to prevent codegen
169+
if const #[track_caller] {
171170
$crate::panic!($const_msg)
172-
} else #[track_caller] { // Do not inline this, it makes perf worse
171+
} else #[track_caller] {
173172
$crate::panic!($runtime_msg)
174173
}
175174
)
@@ -195,7 +194,7 @@ pub macro const_panic {
195194
#[doc(hidden)]
196195
pub macro const_assert {
197196
($condition: expr, $const_msg:literal, $runtime_msg:literal, $($arg:tt)*) => {{
198-
if !$crate::intrinsics::likely($condition) {
197+
if !($condition) {
199198
$crate::panic::const_panic!($const_msg, $runtime_msg, $($arg)*)
200199
}
201200
}}
Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,31 @@
11
//@ compile-flags: -Copt-level=3
2+
#![feature(panic_internals, const_eval_select, rustc_allow_const_fn_unstable, core_intrinsics)]
23
#![crate_type = "lib"]
34

5+
// check that assert! and const_assert! emit branch weights
6+
47
#[no_mangle]
58
pub fn test_assert(x: bool) {
69
assert!(x);
710
}
811

9-
// check that assert! emits branch weights
10-
1112
// CHECK-LABEL: @test_assert(
1213
// CHECK: br i1 %x, label %bb2, label %bb1, !prof ![[NUM:[0-9]+]]
1314
// CHECK: bb1:
1415
// CHECK: panic
1516
// CHECK: bb2:
1617
// CHECK: ret void
18+
19+
#[no_mangle]
20+
pub fn test_const_assert(x: bool) {
21+
core::panic::const_assert!(x, "", "",);
22+
}
23+
24+
// CHECK-LABEL: @test_const_assert(
25+
// CHECK: br i1 %x, label %bb2, label %bb1, !prof ![[NUM:[0-9]+]]
26+
// CHECK: bb1:
27+
// CHECK: panic
28+
// CHECK: bb2:
29+
// CHECK: ret void
30+
1731
// CHECK: ![[NUM]] = !{!"branch_weights", {{(!"expected", )?}}i32 2000, i32 1}

0 commit comments

Comments
 (0)