Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tweak attributes for const panic macro #132662

Merged
merged 1 commit into from
Nov 13, 2024
Merged

tweak attributes for const panic macro #132662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions library/core/src/intrinsics/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2965,14 +2965,35 @@ pub(crate) macro const_eval_select {
$(#[$compiletime_attr:meta])* $compiletime:block
else
$(#[$runtime_attr:meta])* $runtime:block
) => {
// Use the `noinline` arm, after adding explicit `inline` attributes
$crate::intrinsics::const_eval_select!(
@capture { $($arg : $ty = $val),* } $(-> $ret)? :
#[noinline]
if const
#[inline] // prevent codegen on this function
$(#[$compiletime_attr])*
$compiletime
else
#[inline] // avoid the overhead of an extra fn call
$(#[$runtime_attr])*
$runtime
)
},
// With a leading #[noinline], we don't add inline attributes
(
@capture { $($arg:ident : $ty:ty = $val:expr),* $(,)? } $( -> $ret:ty )? :
#[noinline]
if const
$(#[$compiletime_attr:meta])* $compiletime:block
else
$(#[$runtime_attr:meta])* $runtime:block
) => {{
#[inline] // avoid the overhead of an extra fn call
$(#[$runtime_attr])*
fn runtime($($arg: $ty),*) $( -> $ret )? {
$runtime
}

#[inline] // prevent codegen on this function
$(#[$compiletime_attr])*
const fn compiletime($($arg: $ty),*) $( -> $ret )? {
// Don't warn if one of the arguments is unused.
Expand Down
5 changes: 3 additions & 2 deletions library/core/src/num/f128.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1268,8 +1268,9 @@ impl f128 {
min <= max,
"min > max, or either was NaN",
"min > max, or either was NaN. min = {min:?}, max = {max:?}",
min: f128,
max: f128,
// FIXME(f16_f128): Passed by-ref to avoid codegen crashes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed since it looks like it should still get #[inline]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "runtime panic" function generated here will not get inline. That's why this is needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What am I missing - the non-#[noinline] version seems to still get it.

            else
                #[inline] // avoid the overhead of an extra fn call
                $(#[$runtime_attr])*
                $runtime

If this isn't accurate then I think we need to change things to avoid breaking Cranelift (@bjorn3)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const_assert! uses const_panic! which uses #[noinline]. That's the entire point of this PR -- inlining the panic path is what caused the performance regression that this is fixing.

min: &f128 = &min,
max: &f128 = &max,
);

if self < min {
Expand Down
5 changes: 3 additions & 2 deletions library/core/src/num/f16.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1243,8 +1243,9 @@ impl f16 {
min <= max,
"min > max, or either was NaN",
"min > max, or either was NaN. min = {min:?}, max = {max:?}",
min: f16,
max: f16,
// FIXME(f16_f128): Passed by-ref to avoid codegen crashes
min: &f16 = &min,
max: &f16 = &max,
);

if self < min {
Expand Down
9 changes: 5 additions & 4 deletions library/core/src/panic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,15 +206,16 @@ pub macro const_panic {
// add the `rustc_allow_const_fn_unstable`. This is okay to do
// because both variants will panic, just with different messages.
#[rustc_allow_const_fn_unstable(const_eval_select)]
#[inline(always)]
#[inline(always)] // inline the wrapper
#[track_caller]
#[cfg_attr(bootstrap, rustc_const_stable(feature = "const_panic", since = "CURRENT_RUSTC_VERSION"))]
const fn do_panic($($arg: $ty),*) -> ! {
$crate::intrinsics::const_eval_select!(
@capture { $($arg: $ty),* } -> !:
if const #[track_caller] {
@capture { $($arg: $ty = $arg),* } -> !:
#[noinline]
if const #[track_caller] #[inline] { // Inline this, to prevent codegen
$crate::panic!($const_msg)
} else #[track_caller] {
} else #[track_caller] { // Do not inline this, it makes perf worse
$crate::panic!($runtime_msg)
}
)
Expand Down
Loading