Skip to content

Commit e051061

Browse files
committed
Update CmpResult to use a pointer-sized return type
As seen at [1], LLVM uses `long long` on LLP64 (to get a 64-bit integer matching pointer size) and `long` on everything else, with exceptions for AArch64 and AVR. Our current logic always uses an `i32`. This happens to work because LLVM uses 32-bit instructions to check the output on x86-64, but the GCC checks the full 64-bit register so garbage in the upper half leads to incorrect results. Update our return type to be `isize`, with exceptions for AArch64 and AVR. Fixes: rust-lang#919 [1]: https://github.com/llvm/llvm-project/blob/0cf3c437c18ed27d9663d87804a9a15ff6874af2/compiler-rt/lib/builtins/fp_compare_impl.inc#L11-L27
1 parent 157a0b7 commit e051061

File tree

4 files changed

+49
-26
lines changed

4 files changed

+49
-26
lines changed

builtins-test/benches/float_cmp.rs

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,39 @@
11
#![cfg_attr(f128_enabled, feature(f128))]
22

33
use builtins_test::float_bench;
4-
use compiler_builtins::float::cmp;
4+
use compiler_builtins::float::cmp::{self, CmpResult};
55
use criterion::{Criterion, criterion_main};
66

77
/// `gt` symbols are allowed to return differing results, they just get compared
88
/// to 0.
9-
fn gt_res_eq(a: i32, b: i32) -> bool {
9+
fn gt_res_eq(mut a: CmpResult, mut b: CmpResult) -> bool {
10+
// FIXME: Our CmpResult used to be `u32`, but GCC/LLVM expect `usize`. on 64-bit platforms,
11+
// this means the top half of the word may be garbage if built with an old version of
12+
// `compiler-builtins`, so add a hack around this.
13+
//
14+
// This can be removed once a version of `compiler-builtins` with the return type fix makes
15+
// it upstream.
16+
if size_of::<CmpResult>() == 64 {
17+
let mask = u32::MAX as CmpResult;
18+
a |= mask;
19+
b |= mask;
20+
}
21+
1022
let a_lt_0 = a <= 0;
1123
let b_lt_0 = b <= 0;
1224
(a_lt_0 && b_lt_0) || (!a_lt_0 && !b_lt_0)
1325
}
1426

1527
float_bench! {
1628
name: cmp_f32_gt,
17-
sig: (a: f32, b: f32) -> i32,
29+
sig: (a: f32, b: f32) -> CmpResult,
1830
crate_fn: cmp::__gtsf2,
1931
sys_fn: __gtsf2,
2032
sys_available: all(),
2133
output_eq: gt_res_eq,
2234
asm: [
2335
#[cfg(target_arch = "x86_64")] {
24-
let ret: i32;
36+
let ret: CmpResult;
2537
asm!(
2638
"xor {ret:e}, {ret:e}",
2739
"ucomiss {a}, {b}",
@@ -36,7 +48,7 @@ float_bench! {
3648
};
3749

3850
#[cfg(target_arch = "aarch64")] {
39-
let ret: i32;
51+
let ret: CmpResult;
4052
asm!(
4153
"fcmp {a:s}, {b:s}",
4254
"cset {ret:w}, gt",
@@ -53,13 +65,13 @@ float_bench! {
5365

5466
float_bench! {
5567
name: cmp_f32_unord,
56-
sig: (a: f32, b: f32) -> i32,
68+
sig: (a: f32, b: f32) -> CmpResult,
5769
crate_fn: cmp::__unordsf2,
5870
sys_fn: __unordsf2,
5971
sys_available: all(),
6072
asm: [
6173
#[cfg(target_arch = "x86_64")] {
62-
let ret: i32;
74+
let ret: CmpResult;
6375
asm!(
6476
"xor {ret:e}, {ret:e}",
6577
"ucomiss {a}, {b}",
@@ -74,7 +86,7 @@ float_bench! {
7486
};
7587

7688
#[cfg(target_arch = "aarch64")] {
77-
let ret: i32;
89+
let ret: CmpResult;
7890
asm!(
7991
"fcmp {a:s}, {b:s}",
8092
"cset {ret:w}, vs",
@@ -91,14 +103,14 @@ float_bench! {
91103

92104
float_bench! {
93105
name: cmp_f64_gt,
94-
sig: (a: f64, b: f64) -> i32,
106+
sig: (a: f64, b: f64) -> CmpResult,
95107
crate_fn: cmp::__gtdf2,
96108
sys_fn: __gtdf2,
97109
sys_available: all(),
98110
output_eq: gt_res_eq,
99111
asm: [
100112
#[cfg(target_arch = "x86_64")] {
101-
let ret: i32;
113+
let ret: CmpResult;
102114
asm!(
103115
"xor {ret:e}, {ret:e}",
104116
"ucomisd {a}, {b}",
@@ -113,7 +125,7 @@ float_bench! {
113125
};
114126

115127
#[cfg(target_arch = "aarch64")] {
116-
let ret: i32;
128+
let ret: CmpResult;
117129
asm!(
118130
"fcmp {a:d}, {b:d}",
119131
"cset {ret:w}, gt",
@@ -130,13 +142,13 @@ float_bench! {
130142

131143
float_bench! {
132144
name: cmp_f64_unord,
133-
sig: (a: f64, b: f64) -> i32,
145+
sig: (a: f64, b: f64) -> CmpResult,
134146
crate_fn: cmp::__unorddf2,
135147
sys_fn: __unorddf2,
136148
sys_available: all(),
137149
asm: [
138150
#[cfg(target_arch = "x86_64")] {
139-
let ret: i32;
151+
let ret: CmpResult;
140152
asm!(
141153
"xor {ret:e}, {ret:e}",
142154
"ucomisd {a}, {b}",
@@ -151,7 +163,7 @@ float_bench! {
151163
};
152164

153165
#[cfg(target_arch = "aarch64")] {
154-
let ret: i32;
166+
let ret: CmpResult;
155167
asm!(
156168
"fcmp {a:d}, {b:d}",
157169
"cset {ret:w}, vs",
@@ -168,7 +180,7 @@ float_bench! {
168180

169181
float_bench! {
170182
name: cmp_f128_gt,
171-
sig: (a: f128, b: f128) -> i32,
183+
sig: (a: f128, b: f128) -> CmpResult,
172184
crate_fn: cmp::__gttf2,
173185
crate_fn_ppc: cmp::__gtkf2,
174186
sys_fn: __gttf2,
@@ -180,7 +192,7 @@ float_bench! {
180192

181193
float_bench! {
182194
name: cmp_f128_unord,
183-
sig: (a: f128, b: f128) -> i32,
195+
sig: (a: f128, b: f128) -> CmpResult,
184196
crate_fn: cmp::__unordtf2,
185197
crate_fn_ppc: cmp::__unordkf2,
186198
sys_fn: __unordtf2,

builtins-test/src/bench.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -358,8 +358,8 @@ impl_testio!(float f16);
358358
impl_testio!(float f32, f64);
359359
#[cfg(f128_enabled)]
360360
impl_testio!(float f128);
361-
impl_testio!(int i16, i32, i64, i128);
362-
impl_testio!(int u16, u32, u64, u128);
361+
impl_testio!(int i8, i16, i32, i64, i128, isize);
362+
impl_testio!(int u8, u16, u32, u64, u128, usize);
363363
impl_testio!((float, int)(f32, i32));
364364
impl_testio!((float, int)(f64, i32));
365365
#[cfg(f128_enabled)]

compiler-builtins/src/float/cmp.rs

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,23 @@
22

33
use crate::float::Float;
44
use crate::int::MinInt;
5-
6-
// https://github.com/llvm/llvm-project/blob/1e6ba3cd2fe96be00b6ed6ba28b3d9f9271d784d/compiler-rt/lib/builtins/fp_compare_impl.inc#L22
7-
#[cfg(target_arch = "avr")]
8-
pub type CmpResult = i8;
9-
10-
// https://github.com/llvm/llvm-project/blob/1e6ba3cd2fe96be00b6ed6ba28b3d9f9271d784d/compiler-rt/lib/builtins/fp_compare_impl.inc#L25
11-
#[cfg(not(target_arch = "avr"))]
12-
pub type CmpResult = i32;
5+
use crate::support::cfg_if;
6+
7+
// Taken from LLVM config:
8+
// https://github.com/llvm/llvm-project/blob/0cf3c437c18ed27d9663d87804a9a15ff6874af2/compiler-rt/lib/builtins/fp_compare_impl.inc#L11-L27
9+
cfg_if! {
10+
if #[cfg(any(target_arch = "aarch64", target_arch = "arm64ec"))] {
11+
// Aarch64 uses `int` rather than a pointer-sized value.
12+
pub type CmpResult = i32;
13+
} else if #[cfg(target_arch = "avr")] {
14+
// AVR uses a single byte.
15+
pub type CmpResult = i8;
16+
} else {
17+
// In compiler-rt, LLP64 ABIs use `long long` and everything else uses `long`. In effect,
18+
// this means the return value is always pointer-sized.
19+
pub type CmpResult = isize;
20+
}
21+
}
1322

1423
#[derive(Clone, Copy)]
1524
enum Result {

libm/src/math/support/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ mod int_traits;
1111

1212
#[allow(unused_imports)]
1313
pub use big::{i256, u256};
14+
#[allow(unused_imports)]
15+
pub(crate) use cfg_if;
1416
pub use env::{FpResult, Round, Status};
1517
#[allow(unused_imports)]
1618
pub use float_traits::{DFloat, Float, HFloat, IntTy};

0 commit comments

Comments
 (0)