Use ConstantData exclusively for inserting data into the constant pool

Previously we allowed anything that could be converted into ConstantData (e.g. a Vec).

cranelift/codegen/meta/src/gen_legalizer.rs

@@ -470,7 +470,7 @@ fn gen_transform<'a>(
         for (name, value) in transform.const_pool.iter() {
             fmtln!(
                 fmt,
-                "let {} = pos.func.dfg.constants.insert(vec!{:?});",
+                "let {} = pos.func.dfg.constants.insert(vec!{:?}.into());",
                 name,
                 value
             );

cranelift/codegen/src/ir/constant.rs

@@ -14,6 +14,7 @@ use crate::HashMap;
 use alloc::collections::BTreeMap;
 use alloc::vec::Vec;
 use core::fmt;
+use core::iter::FromIterator;
 use core::slice::Iter;
 use core::str::{from_utf8, FromStr};
 use cranelift_entity::EntityRef;
@@ -25,6 +26,13 @@ use cranelift_entity::EntityRef;
 #[derive(Clone, Hash, Eq, PartialEq, Debug, Default)]
 pub struct ConstantData(Vec<u8>);
 
+impl FromIterator<u8> for ConstantData {
+    fn from_iter<T: IntoIterator<Item = u8>>(iter: T) -> Self {
+        let v = iter.into_iter().collect();
+        ConstantData(v)
+    }
+}
+
 impl From<Vec<u8>> for ConstantData {
     fn from(v: Vec<u8>) -> Self {
         Self(v)
@@ -211,8 +219,7 @@ impl ConstantPool {
     /// Insert constant data into the pool, returning a handle for later referencing; when constant
     /// data is inserted that is a duplicate of previous constant data, the existing handle will be
     /// returned.
-    pub fn insert(&mut self, constant_value: impl Into<ConstantData>) -> Constant {
+    pub fn insert(&mut self, constant_value: ConstantData) -> Constant {
-        let constant_value = constant_value.into();
         if self.values_to_handles.contains_key(&constant_value) {
             self.values_to_handles.get(&constant_value).unwrap().clone()
         } else {
@@ -297,24 +304,24 @@ mod tests {
     #[test]
     fn insert() {
         let mut sut = ConstantPool::new();
-        sut.insert(vec![1, 2, 3]);
+        sut.insert(vec![1, 2, 3].into());
-        sut.insert(vec![4, 5, 6]);
+        sut.insert(vec![4, 5, 6].into());
         assert_eq!(sut.len(), 2);
     }
 
     #[test]
     fn insert_duplicate() {
         let mut sut = ConstantPool::new();
-        let a = sut.insert(vec![1, 2, 3]);
+        let a = sut.insert(vec![1, 2, 3].into());
-        sut.insert(vec![4, 5, 6]);
+        sut.insert(vec![4, 5, 6].into());
-        let b = sut.insert(vec![1, 2, 3]);
+        let b = sut.insert(vec![1, 2, 3].into());
         assert_eq!(a, b);
     }
 
     #[test]
     fn clear() {
         let mut sut = ConstantPool::new();
-        sut.insert(vec![1, 2, 3]);
+        sut.insert(vec![1, 2, 3].into());
         assert_eq!(sut.len(), 1);
 
         sut.clear();
@@ -324,9 +331,9 @@ mod tests {
     #[test]
     fn iteration_order() {
         let mut sut = ConstantPool::new();
-        sut.insert(vec![1, 2, 3]);
+        sut.insert(vec![1, 2, 3].into());
-        sut.insert(vec![4, 5, 6]);
+        sut.insert(vec![4, 5, 6].into());
-        sut.insert(vec![1, 2, 3]);
+        sut.insert(vec![1, 2, 3].into());
         let data = sut.iter().map(|(_, v)| v).collect::<Vec<&ConstantData>>();
         assert_eq!(data, vec![&vec![1, 2, 3].into(), &vec![4, 5, 6].into()]);
     }
@@ -335,7 +342,7 @@ mod tests {
     fn get() {
         let mut sut = ConstantPool::new();
         let data = vec![1, 2, 3];
-        let handle = sut.insert(data.clone());
+        let handle = sut.insert(data.clone().into());
         assert_eq!(sut.get(handle), &data.into());
     }
 
@@ -350,7 +357,7 @@ mod tests {
     #[test]
     fn get_offset() {
         let mut sut = ConstantPool::new();
-        let a = sut.insert(vec![1]);
+        let a = sut.insert(vec![1].into());
         sut.set_offset(a, 42);
         assert_eq!(sut.get_offset(a), 42)
     }
@@ -359,7 +366,7 @@ mod tests {
     #[should_panic]
     fn get_nonexistent_offset() {
         let mut sut = ConstantPool::new();
-        let a = sut.insert(vec![1]);
+        let a = sut.insert(vec![1].into());
         sut.get_offset(a); // panics, set_offset should have been called
     }
 

cranelift/codegen/src/isa/x86/enc_tables.rs

@@ -6,7 +6,7 @@ use crate::cursor::{Cursor, FuncCursor};
 use crate::flowgraph::ControlFlowGraph;
 use crate::ir::condcodes::{FloatCC, IntCC};
 use crate::ir::types::*;
-use crate::ir::{self, ConstantData, Function, Inst, InstBuilder};
+use crate::ir::{self, Function, Inst, InstBuilder};
 use crate::isa::constraints::*;
 use crate::isa::enc_tables::*;
 use crate::isa::encoding::base_size;
@@ -15,7 +15,6 @@ use crate::isa::RegUnit;
 use crate::isa::{self, TargetIsa};
 use crate::predicates;
 use crate::regalloc::RegDiversions;
-use std::vec::Vec;
 
 include!(concat!(env!("OUT_DIR"), "/encoding-x86.rs"));
 include!(concat!(env!("OUT_DIR"), "/legalize-x86.rs"));
@@ -928,7 +927,7 @@ fn convert_shuffle(
             .clone();
         if a == b {
             // PSHUFB the first argument (since it is the same as the second).
-            let constructed_mask: Vec<u8> = mask
+            let constructed_mask = mask
                 .iter()
                 // If the mask is greater than 15 it still may be referring to a lane in b.
                 .map(|&b| if b > 15 { b.wrapping_sub(16) } else { b })
@@ -942,8 +941,7 @@ fn convert_shuffle(
             pos.func.dfg.replace(inst).x86_pshufb(a, mask_value);
         } else {
             // PSHUFB the first argument, placing zeroes for unused lanes.
-            let constructed_mask: Vec<u8> =
+            let constructed_mask = mask.iter().cloned().map(zero_unknown_lane_index).collect();
-                mask.iter().cloned().map(zero_unknown_lane_index).collect();
             let handle = pos.func.dfg.constants.insert(constructed_mask);
             // Move the built mask into another XMM register.
             let a_type = pos.func.dfg.value_type(a);
@@ -952,7 +950,7 @@ fn convert_shuffle(
             let shuffled_first_arg = pos.ins().x86_pshufb(a, mask_value);
 
             // PSHUFB the second argument, placing zeroes for unused lanes.
-            let constructed_mask: Vec<u8> = mask
+            let constructed_mask = mask
                 .iter()
                 .map(|b| b.wrapping_sub(16))
                 .map(zero_unknown_lane_index)
@@ -1110,11 +1108,7 @@ fn convert_ineg(
     {
         let value_type = pos.func.dfg.value_type(arg);
         if value_type.is_vector() && value_type.lane_type().is_int() {
-            let zero_immediate = pos
+            let zero_immediate = pos.func.dfg.constants.insert(vec![0; 16].into());
-                .func
-                .dfg
-                .constants
-                .insert(ConstantData::from(vec![0; 16]));
             let zero_value = pos.ins().vconst(value_type, zero_immediate); // this should be legalized to a PXOR
             pos.func.dfg.replace(inst).isub(zero_value, arg);
         }

cranelift/wasm/src/code_translator.rs

@@ -998,7 +998,8 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
             return Err(wasm_unsupported!("proposed bulk memory operator {:?}", op));
         }
         Operator::V128Const { value } => {
-            let handle = builder.func.dfg.constants.insert(value.bytes().to_vec());
+            let data = value.bytes().to_vec().into();
+            let handle = builder.func.dfg.constants.insert(data);
             let value = builder.ins().vconst(I8X16, handle);
             // the v128.const is typed in CLIF as a I8x16 but raw_bitcast to a different type before use
             state.push1(value)

cranelift/wasm/src/func_translator.rs

@@ -189,7 +189,7 @@ fn declare_locals<FE: FuncEnvironment + ?Sized>(
         F32 => builder.ins().f32const(ir::immediates::Ieee32::with_bits(0)),
         F64 => builder.ins().f64const(ir::immediates::Ieee64::with_bits(0)),
         V128 => {
-            let constant_handle = builder.func.dfg.constants.insert([0; 16].to_vec());
+            let constant_handle = builder.func.dfg.constants.insert([0; 16].to_vec().into());
             builder.ins().vconst(ir::types::I8X16, constant_handle)
         }
         AnyRef => builder.ins().null(environ.reference_type()),