Make DataValue, not Ieee32/64, respect IEEE754 (#4860)

* cranelift-codegen: Remove all uses of DataValue

This type is only used by the interpreter, cranelift-fuzzgen, and
filetests. I haven't found another convenient crate for those to all
depend on where this type can live instead, but this small refactor at
least makes it obvious that code generation does not in any way depend
on the implementation of this type.

* Make DataValue, not Ieee32/64, respect IEEE754

This fixes #4857 by partially reverting #4849.

It turns out that Ieee32 and Ieee64 need bitwise equality semantics so
they can be used as hash-table keys.

Moving the IEEE754 semantics up a layer to DataValue makes sense in
conjunction with #4855, where we introduced a DataValue::bitwise_eq
alternative implementation of equality for those cases where users of
DataValue still want the bitwise equality semantics.

* cranelift-interpreter: Use eq/ord from DataValue

This fixes #4828, again, now that the comparison operators on DataValue
have the right IEEE754 semantics.

* Add regression test from issue #4857

cranelift/codegen/src/data_value.rs

@@ -10,7 +10,7 @@ use core::fmt::{self, Display, Formatter};
 ///
 /// [Value]: crate::ir::Value
 #[allow(missing_docs)]
-#[derive(Clone, Debug, PartialEq, PartialOrd)]
+#[derive(Clone, Debug, PartialOrd)]
 pub enum DataValue {
     B(bool),
     I8(i8),
@@ -29,6 +29,44 @@ pub enum DataValue {
     V64([u8; 8]),
 }
 
+impl PartialEq for DataValue {
+    fn eq(&self, other: &Self) -> bool {
+        use DataValue::*;
+        match (self, other) {
+            (B(l), B(r)) => l == r,
+            (B(_), _) => false,
+            (I8(l), I8(r)) => l == r,
+            (I8(_), _) => false,
+            (I16(l), I16(r)) => l == r,
+            (I16(_), _) => false,
+            (I32(l), I32(r)) => l == r,
+            (I32(_), _) => false,
+            (I64(l), I64(r)) => l == r,
+            (I64(_), _) => false,
+            (I128(l), I128(r)) => l == r,
+            (I128(_), _) => false,
+            (U8(l), U8(r)) => l == r,
+            (U8(_), _) => false,
+            (U16(l), U16(r)) => l == r,
+            (U16(_), _) => false,
+            (U32(l), U32(r)) => l == r,
+            (U32(_), _) => false,
+            (U64(l), U64(r)) => l == r,
+            (U64(_), _) => false,
+            (U128(l), U128(r)) => l == r,
+            (U128(_), _) => false,
+            (F32(l), F32(r)) => l.as_f32() == r.as_f32(),
+            (F32(_), _) => false,
+            (F64(l), F64(r)) => l.as_f64() == r.as_f64(),
+            (F64(_), _) => false,
+            (V128(l), V128(r)) => l == r,
+            (V128(_), _) => false,
+            (V64(l), V64(r)) => l == r,
+            (V64(_), _) => false,
+        }
+    }
+}
+
 impl DataValue {
     /// Try to cast an immediate integer (a wrapped `i64` on most Cranelift instructions) to the
     /// given Cranelift [Type].

cranelift/codegen/src/ir/immediates.rs

@@ -475,8 +475,11 @@ impl FromStr for Offset32 {
 /// We specifically avoid using a f32 here since some architectures may silently alter floats.
 /// See: https://github.com/bytecodealliance/wasmtime/pull/2251#discussion_r498508646
 ///
+/// The [PartialEq] and [Hash] implementations are over the underlying bit pattern, but
+/// [PartialOrd] respects IEEE754 semantics.
+///
 /// All bit patterns are allowed.
-#[derive(Copy, Clone, Debug, Eq, Hash)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
 #[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
 #[repr(C)]
 pub struct Ieee32(u32);
@@ -487,8 +490,11 @@ pub struct Ieee32(u32);
 /// We specifically avoid using a f64 here since some architectures may silently alter floats.
 /// See: https://github.com/bytecodealliance/wasmtime/pull/2251#discussion_r498508646
 ///
+/// The [PartialEq] and [Hash] implementations are over the underlying bit pattern, but
+/// [PartialOrd] respects IEEE754 semantics.
+///
 /// All bit patterns are allowed.
-#[derive(Copy, Clone, Debug, Eq, Hash)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
 #[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
 #[repr(C)]
 pub struct Ieee64(u64);
@@ -845,12 +851,6 @@ impl PartialOrd for Ieee32 {
     }
 }
 
-impl PartialEq<Ieee32> for Ieee32 {
-    fn eq(&self, other: &Ieee32) -> bool {
-        self.as_f32().eq(&other.as_f32())
-    }
-}
-
 impl Display for Ieee32 {
     fn fmt(&self, f: &mut Formatter) -> fmt::Result {
         let bits: u32 = self.0;
@@ -1037,12 +1037,6 @@ impl PartialOrd for Ieee64 {
     }
 }
 
-impl PartialEq<Ieee64> for Ieee64 {
-    fn eq(&self, other: &Ieee64) -> bool {
-        self.as_f64().eq(&other.as_f64())
-    }
-}
-
 impl Display for Ieee64 {
     fn fmt(&self, f: &mut Formatter) -> fmt::Result {
         let bits: u64 = self.0;

cranelift/codegen/src/ir/instructions.rs

@@ -17,7 +17,6 @@ use core::str::FromStr;
 use serde::{Deserialize, Serialize};
 
 use crate::bitset::BitSet;
-use crate::data_value::DataValue;
 use crate::entity;
 use crate::ir::{
     self,
@@ -289,39 +288,6 @@ impl InstructionData {
         }
     }
 
-    /// Return the value of an immediate if the instruction has one or `None` otherwise. Only
-    /// immediate values are considered, not global values, constant handles, condition codes, etc.
-    pub fn imm_value(&self) -> Option<DataValue> {
-        match self {
-            &InstructionData::UnaryBool { imm, .. } => Some(DataValue::from(imm)),
-            // 8-bit.
-            &InstructionData::BinaryImm8 { imm, .. }
-            | &InstructionData::TernaryImm8 { imm, .. } => Some(DataValue::from(imm as i8)), // Note the switch from unsigned to signed.
-            // 32-bit
-            &InstructionData::UnaryIeee32 { imm, .. } => Some(DataValue::from(imm)),
-            &InstructionData::HeapAddr { imm, .. } => {
-                let imm: u32 = imm.into();
-                Some(DataValue::from(imm as i32)) // Note the switch from unsigned to signed.
-            }
-            &InstructionData::Load { offset, .. }
-            | &InstructionData::Store { offset, .. }
-            | &InstructionData::StackLoad { offset, .. }
-            | &InstructionData::StackStore { offset, .. }
-            | &InstructionData::TableAddr { offset, .. } => Some(DataValue::from(offset)),
-            // 64-bit.
-            &InstructionData::UnaryImm { imm, .. }
-            | &InstructionData::BinaryImm64 { imm, .. }
-            | &InstructionData::IntCompareImm { imm, .. } => Some(DataValue::from(imm.bits())),
-            &InstructionData::UnaryIeee64 { imm, .. } => Some(DataValue::from(imm)),
-            // 128-bit; though these immediates are present logically in the IR they are not
-            // included in the `InstructionData` for memory-size reasons. This case, returning
-            // `None`, is left here to alert users of this method that they should retrieve the
-            // value using the `DataFlowGraph`.
-            &InstructionData::Shuffle { imm: _, .. } => None,
-            _ => None,
-        }
-    }
-
     /// If this is a trapping instruction, get its trap code. Otherwise, return
     /// `None`.
     pub fn trap_code(&self) -> Option<TrapCode> {

cranelift/codegen/src/machinst/isle.rs

@@ -7,7 +7,6 @@ use std::cell::Cell;
 use target_lexicon::Triple;
 
 pub use super::MachLabel;
-pub use crate::data_value::DataValue;
 pub use crate::ir::{
     dynamic_to_fixed, ArgumentExtension, Constant, DynamicStackSlot, ExternalName, FuncRef,
     GlobalValue, Immediate, SigRef, StackSlot,

cranelift/interpreter/src/step.rs

@@ -114,7 +114,28 @@ where
                     length => panic!("unexpected Shuffle mask length {}", length),
                 }
             }
-            _ => inst.imm_value().unwrap(),
+            InstructionData::UnaryBool { imm, .. } => DataValue::from(imm),
+            // 8-bit.
+            InstructionData::BinaryImm8 { imm, .. } | InstructionData::TernaryImm8 { imm, .. } => {
+                DataValue::from(imm as i8) // Note the switch from unsigned to signed.
+            }
+            // 32-bit
+            InstructionData::UnaryIeee32 { imm, .. } => DataValue::from(imm),
+            InstructionData::HeapAddr { imm, .. } => {
+                let imm: u32 = imm.into();
+                DataValue::from(imm as i32) // Note the switch from unsigned to signed.
+            }
+            InstructionData::Load { offset, .. }
+            | InstructionData::Store { offset, .. }
+            | InstructionData::StackLoad { offset, .. }
+            | InstructionData::StackStore { offset, .. }
+            | InstructionData::TableAddr { offset, .. } => DataValue::from(offset),
+            // 64-bit.
+            InstructionData::UnaryImm { imm, .. }
+            | InstructionData::BinaryImm64 { imm, .. }
+            | InstructionData::IntCompareImm { imm, .. } => DataValue::from(imm.bits()),
+            InstructionData::UnaryIeee64 { imm, .. } => DataValue::from(imm),
+            _ => unreachable!(),
         })
     };
 

cranelift/interpreter/src/value.rs

@@ -208,14 +208,6 @@ macro_rules! binary_match {
         }
     };
 }
-macro_rules! comparison_match {
-    ( $op:path[$arg1:expr, $arg2:expr]; [ $( $data_value_ty:ident ),* ] ) => {
-        match ($arg1, $arg2) {
-            $( (DataValue::$data_value_ty(a), DataValue::$data_value_ty(b)) => { Ok($op(a, b)) } )*
-            _ => unimplemented!("comparison: {:?}, {:?}", $arg1, $arg2)
-        }
-    };
-}
 
 impl Value for DataValue {
     fn ty(&self) -> Type {
@@ -475,11 +467,11 @@ impl Value for DataValue {
     }
 
     fn eq(&self, other: &Self) -> ValueResult<bool> {
-        comparison_match!(PartialEq::eq[&self, &other]; [I8, I16, I32, I64, I128, U8, U16, U32, U64, U128, F32, F64])
+        Ok(self == other)
     }
 
     fn gt(&self, other: &Self) -> ValueResult<bool> {
-        comparison_match!(PartialOrd::gt[&self, &other]; [I8, I16, I32, I64, I128, U8, U16, U32, U64, U128, F32, F64])
+        Ok(self > other)
     }
 
     fn uno(&self, other: &Self) -> ValueResult<bool> {

tests/misc_testsuite/issue4857.wast

@@ -0,0 +1,10 @@
+(module
+  (func
+    i32.const 0
+    if
+      unreachable
+    end
+    f32.const nan
+    drop
+  )
+)