From 3778fa025cc00e0d315333025991c3fdba595ceb Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Wed, 7 Oct 2020 09:57:51 -0700 Subject: [PATCH] Switch DataValue to use Ieee32/Ieee64 As discussed in #2251, in order to be very confident that NaN signaling bits are correctly handled by the compiler, this switches `DataValue` to use Cranelift's `Ieee32` and `Ieee64` structures. This makes it a bit more inconvenient to interpreter Cranelift FP operations but this should change to something like `rustc_apfloat` in the future. --- cranelift/codegen/src/data_value.rs | 24 +++++++--------------- cranelift/codegen/src/ir/immediates.rs | 2 ++ cranelift/filetests/src/function_runner.rs | 9 ++++---- cranelift/interpreter/src/interpreter.rs | 11 +++++----- cranelift/reader/src/parser.rs | 4 ++-- 5 files changed, 22 insertions(+), 28 deletions(-) diff --git a/cranelift/codegen/src/data_value.rs b/cranelift/codegen/src/data_value.rs index 4568923bfc06..6ffcb99842d8 100644 --- a/cranelift/codegen/src/data_value.rs +++ b/cranelift/codegen/src/data_value.rs @@ -18,8 +18,8 @@ pub enum DataValue { I16(i16), I32(i32), I64(i64), - F32(f32), - F64(f64), + F32(Ieee32), + F64(Ieee64), V128([u8; 16]), } @@ -97,19 +97,9 @@ build_conversion_impl!(i8, I8, I8); build_conversion_impl!(i16, I16, I16); build_conversion_impl!(i32, I32, I32); build_conversion_impl!(i64, I64, I64); -build_conversion_impl!(f32, F32, F32); -build_conversion_impl!(f64, F64, F64); +build_conversion_impl!(Ieee32, F32, F32); +build_conversion_impl!(Ieee64, F64, F64); build_conversion_impl!([u8; 16], V128, I8X16); -impl From for DataValue { - fn from(f: Ieee64) -> Self { - DataValue::from(f64::from_bits(f.bits())) - } -} -impl From for DataValue { - fn from(f: Ieee32) -> Self { - DataValue::from(f32::from_bits(f.bits())) - } -} impl From for DataValue { fn from(o: Offset32) -> Self { DataValue::from(Into::::into(o)) @@ -124,9 +114,9 @@ impl Display for DataValue { DataValue::I16(dv) => write!(f, "{}", dv), DataValue::I32(dv) => write!(f, "{}", dv), DataValue::I64(dv) => write!(f, "{}", dv), - // Use the Ieee* wrappers here to maintain a consistent syntax. - DataValue::F32(dv) => write!(f, "{}", Ieee32::from(*dv)), - DataValue::F64(dv) => write!(f, "{}", Ieee64::from(*dv)), + // The Ieee* wrappers here print the expected syntax. + DataValue::F32(dv) => write!(f, "{}", dv), + DataValue::F64(dv) => write!(f, "{}", dv), // Again, for syntax consistency, use ConstantData, which in this case displays as hex. DataValue::V128(dv) => write!(f, "{}", ConstantData::from(&dv[..])), } diff --git a/cranelift/codegen/src/ir/immediates.rs b/cranelift/codegen/src/ir/immediates.rs index 27746315d68b..0320676be20b 100644 --- a/cranelift/codegen/src/ir/immediates.rs +++ b/cranelift/codegen/src/ir/immediates.rs @@ -450,6 +450,7 @@ impl FromStr for Offset32 { /// /// All bit patterns are allowed. #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)] +#[repr(C)] pub struct Ieee32(u32); /// An IEEE binary64 immediate floating point value, represented as a u64 @@ -457,6 +458,7 @@ pub struct Ieee32(u32); /// /// All bit patterns are allowed. #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)] +#[repr(C)] pub struct Ieee64(u64); /// Format a floating point number in a way that is reasonably human-readable, and that can be diff --git a/cranelift/filetests/src/function_runner.rs b/cranelift/filetests/src/function_runner.rs index 0f8931dce709..e798e4d072a2 100644 --- a/cranelift/filetests/src/function_runner.rs +++ b/cranelift/filetests/src/function_runner.rs @@ -2,6 +2,7 @@ use core::{mem, ptr}; use cranelift_codegen::binemit::{NullRelocSink, NullStackMapSink, NullTrapSink}; use cranelift_codegen::data_value::DataValue; +use cranelift_codegen::ir::immediates::{Ieee32, Ieee64}; use cranelift_codegen::ir::{condcodes::IntCC, Function, InstBuilder, Signature, Type}; use cranelift_codegen::isa::TargetIsa; use cranelift_codegen::{ir, settings, CodegenError, Context}; @@ -233,8 +234,8 @@ impl UnboxedValues { DataValue::I16(i) => ptr::write(p as *mut i16, *i), DataValue::I32(i) => ptr::write(p as *mut i32, *i), DataValue::I64(i) => ptr::write(p as *mut i64, *i), - DataValue::F32(f) => ptr::write(p as *mut f32, *f), - DataValue::F64(f) => ptr::write(p as *mut f64, *f), + DataValue::F32(f) => ptr::write(p as *mut Ieee32, *f), + DataValue::F64(f) => ptr::write(p as *mut Ieee64, *f), DataValue::V128(b) => ptr::write(p as *mut [u8; 16], *b), } } @@ -246,8 +247,8 @@ impl UnboxedValues { ir::types::I16 => DataValue::I16(ptr::read(p as *const i16)), ir::types::I32 => DataValue::I32(ptr::read(p as *const i32)), ir::types::I64 => DataValue::I64(ptr::read(p as *const i64)), - ir::types::F32 => DataValue::F32(ptr::read(p as *const f32)), - ir::types::F64 => DataValue::F64(ptr::read(p as *const f64)), + ir::types::F32 => DataValue::F32(ptr::read(p as *const Ieee32)), + ir::types::F64 => DataValue::F64(ptr::read(p as *const Ieee64)), _ if ty.is_bool() => DataValue::B(ptr::read(p as *const bool)), _ if ty.is_vector() && ty.bytes() == 16 => { DataValue::V128(ptr::read(p as *const [u8; 16])) diff --git a/cranelift/interpreter/src/interpreter.rs b/cranelift/interpreter/src/interpreter.rs index 06cc7fd27111..b4a9c6f2d1af 100644 --- a/cranelift/interpreter/src/interpreter.rs +++ b/cranelift/interpreter/src/interpreter.rs @@ -12,7 +12,7 @@ use cranelift_codegen::ir::{ Value as ValueRef, ValueList, }; use log::trace; -use std::ops::{Add, Div, Mul, Sub}; +use std::ops::{Add, Mul, Sub}; use thiserror::Error; /// The valid control flow states. @@ -153,10 +153,11 @@ impl Interpreter { Iadd => binary_op!(Add::add[arg1, arg2]; [I8, I16, I32, I64]; inst), Isub => binary_op!(Sub::sub[arg1, arg2]; [I8, I16, I32, I64]; inst), Imul => binary_op!(Mul::mul[arg1, arg2]; [I8, I16, I32, I64]; inst), - Fadd => binary_op!(Add::add[arg1, arg2]; [F32, F64]; inst), - Fsub => binary_op!(Sub::sub[arg1, arg2]; [F32, F64]; inst), - Fmul => binary_op!(Mul::mul[arg1, arg2]; [F32, F64]; inst), - Fdiv => binary_op!(Div::div[arg1, arg2]; [F32, F64]; inst), + // TODO re-enable by importing something like rustc_apfloat for correctness. + // Fadd => binary_op!(Add::add[arg1, arg2]; [F32, F64]; inst), + // Fsub => binary_op!(Sub::sub[arg1, arg2]; [F32, F64]; inst), + // Fmul => binary_op!(Mul::mul[arg1, arg2]; [F32, F64]; inst), + // Fdiv => binary_op!(Div::div[arg1, arg2]; [F32, F64]; inst), _ => unimplemented!("interpreter does not support opcode yet: {}", opcode), }?; frame.set(first_result(frame.function, inst), result); diff --git a/cranelift/reader/src/parser.rs b/cranelift/reader/src/parser.rs index a67dcfe01942..33b18da6b1e9 100644 --- a/cranelift/reader/src/parser.rs +++ b/cranelift/reader/src/parser.rs @@ -2700,8 +2700,8 @@ impl<'a> Parser<'a> { I16 => DataValue::from(self.match_imm16("expected an i16")?), I32 => DataValue::from(self.match_imm32("expected an i32")?), I64 => DataValue::from(Into::::into(self.match_imm64("expected an i64")?)), - F32 => DataValue::from(f32::from_bits(self.match_ieee32("expected an f32")?.bits())), - F64 => DataValue::from(f64::from_bits(self.match_ieee64("expected an f64")?.bits())), + F32 => DataValue::from(self.match_ieee32("expected an f32")?), + F64 => DataValue::from(self.match_ieee64("expected an f64")?), _ if ty.is_vector() => { let as_vec = self.match_uimm128(ty)?.into_vec(); if as_vec.len() == 16 {