Skip to content

Better float casting tests in coretests#152597

Open
eggyal wants to merge 2 commits intorust-lang:mainfrom
eggyal:float-casting-coretests
Open

Better float casting tests in coretests#152597
eggyal wants to merge 2 commits intorust-lang:mainfrom
eggyal:float-casting-coretests

Conversation

@eggyal
Copy link
Contributor

@eggyal eggyal commented Feb 14, 2026

A direct port of the float casting tests from MIRI.

Closes #152592

r? tgross35

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 14, 2026
@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2026

tgross35 is currently at their maximum review capacity.
They may take a while to respond.

@rust-log-analyzer

This comment has been minimized.

@okaneco
Copy link
Contributor

okaneco commented Feb 14, 2026

My first exposure to the cranelift codebase was in the float constant PR, so I'm by no means an expert.

There was a change in that PR that you might want to cherry pick so it solves the separate f128->int casts issue c4582c8

For this CI failure, in the lib_call on line 220, should that to_ty be replaced with ret_ty in the third argument of the Vec? to_ty would still be an I8 if passed directly, but line 208 sets ret_ty to an I32.

} else if from_ty == types::F128 && to_ty.is_int() {
let ret_ty = if to_ty.bits() < 32 { types::I32 } else { to_ty };
let name = format!(
"__fix{sign}tf{size}i",
sign = if from_signed { "" } else { "un" },
size = match ret_ty {
types::I32 => 's',
types::I64 => 'd',
types::I128 => 't',
_ => unreachable!("{from_ty:?}"),
},
);
let ret =
fx.lib_call(&name, vec![AbiParam::new(from_ty)], vec![AbiParam::new(to_ty)], &[from])
[0];
let val = if ret_ty == to_ty {
ret
} else {
let (min, max) = match (to_ty, to_signed) {
(types::I8, false) => (0, i64::from(u8::MAX)),
(types::I16, false) => (0, i64::from(u16::MAX)),
(types::I8, true) => (i64::from(i8::MIN as u32), i64::from(i8::MAX as u32)),
(types::I16, true) => (i64::from(i16::MIN as u32), i64::from(i16::MAX as u32)),
_ => unreachable!("{to_ty:?}"),
};
let min_val = fx.bcx.ins().iconst(types::I32, min);
let max_val = fx.bcx.ins().iconst(types::I32, max);
let val = if to_signed {
let has_underflow = fx.bcx.ins().icmp_imm(IntCC::SignedLessThan, ret, min);
let has_overflow = fx.bcx.ins().icmp_imm(IntCC::SignedGreaterThan, ret, max);
let bottom_capped = fx.bcx.ins().select(has_underflow, min_val, ret);
fx.bcx.ins().select(has_overflow, max_val, bottom_capped)
} else {
let has_overflow = fx.bcx.ins().icmp_imm(IntCC::UnsignedGreaterThan, ret, max);
fx.bcx.ins().select(has_overflow, max_val, ret)
};
fx.bcx.ins().ireduce(to_ty, val)
};
if let Some(false) = fx.tcx.sess.opts.unstable_opts.saturating_float_casts {
return val;
}
let is_not_nan = fcmp(fx, FloatCC::Equal, from, from);
let zero = type_zero_value(&mut fx.bcx, to_ty);
fx.bcx.ins().select(is_not_nan, val, zero)
} else {

@eggyal

This comment was marked as resolved.

@tgross35
Copy link
Contributor

What @okaneco should be accurate. The fix is coming via #152512, but you can cherry pick the linked commit if you'd like to get testing before that merges.

@eggyal

This comment was marked as resolved.

@tgross35
Copy link
Contributor

That linking error should be what the commit (c4582c8) fixes. The symbols starts with __fixunstf, not __fixuntf.

It should merge sometime later today, probably easiest to just rebase once it does.

@eggyal
Copy link
Contributor Author

eggyal commented Feb 14, 2026

Ah, indeed - I messed up the cherry-pick, apologies.

eggyal added a commit to eggyal/rust that referenced this pull request Feb 14, 2026
Per rust-lang#152597 (comment)

Co-authored-by: okaneco <47607823+okaneco@users.noreply.github.com>
@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2026

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

eggyal pushed a commit to eggyal/rust that referenced this pull request Feb 14, 2026
@eggyal eggyal force-pushed the float-casting-coretests branch from 6771c64 to 0e1b15d Compare February 14, 2026 10:59
@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2026

The Miri subtree was changed

cc @rust-lang/miri

@RalfJung
Copy link
Member

Miri does run coretests but not in its own CI, so I don't think we should remove existing tests.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

If tests from tests/ui/numbers-arithmetic/saturating-float-casts-impl.rs and tests/ui/numbers-arithmetic/u128-as-f32.rs are covered here, I think it would be fine to remove those UI tests.

View changes since this review

Comment on lines 664 to 677
#[test]
fn nan_casts() {
let nan1 = f64::from_bits(0x7FF0_0001_0000_0001u64);
let nan2 = f64::from_bits(0x7FF0_0000_0000_0001u64);

assert!(nan1.is_nan());
assert!(nan2.is_nan());

let nan1_32 = nan1 as f32;
let nan2_32 = nan2 as f32;

assert!(nan1_32.is_nan());
assert!(nan2_32.is_nan());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you cover f16 and f128 here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All starting from f64, or do we want to test NaN casts going in other directions too?

@rustbot rustbot 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 Feb 16, 2026
@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

eggyal pushed a commit to eggyal/rust that referenced this pull request Feb 16, 2026
@eggyal eggyal force-pushed the float-casting-coretests branch from 0e1b15d to ee9e05d Compare February 16, 2026 12:28
@rust-bors

This comment has been minimized.

@eggyal eggyal force-pushed the float-casting-coretests branch from ee9e05d to 5c66c74 Compare February 16, 2026 20:13
eggyal pushed a commit to eggyal/rust that referenced this pull request Feb 16, 2026
@rustbot

This comment has been minimized.

@rust-bors

This comment has been minimized.

eggyal and others added 2 commits February 17, 2026 08:06
@eggyal eggyal force-pushed the float-casting-coretests branch from 5c66c74 to 9528514 Compare February 17, 2026 08:06
@rustbot
Copy link
Collaborator

rustbot commented Feb 17, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better float casting tests are needed in coretests

6 participants