Repository: bytecodealliance/wasmtime
Pull request #8117 (2024-03-13 17:03:30)

x64: Remove load sinking from `XmmCmove`
This commit completely replaces the `XmmMemAligned` operand in `XmmCmove` with `Xmm` instead. Looking more into the fix #8113 I was looking to add some `*.wast` runtime tests to assert that the fix works at the wasm layer in addition to the instruction selection layer. I was poking around and there's a second user of `XmmCmove` which wasn't addressed in #8113.

In #8113 the only caller of `cmove_xmm` was updated to ensure that everything was always in memory. This was done to both prevent the upgrade-to-an-aligned-load from loading too much but additionally to ensure that the load always happened, regardless of the condition. There was a second constructor of `XmmCmove`, however, from `cmove_or_xmm`. This is triggered when the condition is a `f64.ne` instruction, for example, and ran into the same bug that #8112 was exposing.

To fix both of these and prevent any future issues about skipping a load by accident due to control flow this commit removes the `XmmMemAligned` argument entirely from `XmmCmove` and replaces it with `Xmm`. This prevents sinking loads entirely and sidesteps all these issues at the type level.

<!--
Please make sure you include the following information:

- If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

- Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->



Patch:
diff --git a/cranelift/codegen/src/isa/x64/inst.isle b/cranelift/codegen/src/isa/x64/inst.isle
index e3cf5c69a..ea698b189 100644
--- a/cranelift/codegen/src/isa/x64/inst.isle
+++ b/cranelift/codegen/src/isa/x64/inst.isle
@@ -243,7 +243,7 @@
        ;; XMM conditional move; overwrites the destination register.
        (XmmCmove (ty Type)
                  (cc CC)
-                 (consequent XmmMemAligned)
+                 (consequent Xmm)
                  (alternative Xmm)
                  (dst WritableXmm))
 
@@ -2844,7 +2844,7 @@
          (MInst.Cmove size cc consequent alternative dst)
          dst)))
 
-(decl cmove_xmm (Type CC XmmMemAligned Xmm) ConsumesFlags)
+(decl cmove_xmm (Type CC Xmm Xmm) ConsumesFlags)
 (rule (cmove_xmm ty cc consequent alternative)
       (let ((dst WritableXmm (temp_writable_xmm)))
         (ConsumesFlags.ConsumesFlagsReturnsReg
@@ -2880,14 +2880,7 @@
       (cmove ty cc consequent alternative))
 
 (rule (cmove_from_values (is_xmm_type ty) cc consequent alternative)
-      ;; force the `Value`s to `Xmm`s (in-register values) here:
-      ;; `cmove_xmm` can take an `XmmMemAligned`, but we don't want to
-      ;; allow load fusion here because `ty` may not be
-      ;; XMM-register-wide even if it is `is_xmm_type`. (E.g.,
-      ;; consider a cmove of `f64` values.)
-      (let ((cons Xmm consequent)
-            (alt Xmm alternative))
-        (cmove_xmm ty cc cons alt)))
+      (cmove_xmm ty cc consequent alternative))
 
 ;; Helper for creating `cmove` instructions with the logical OR of multiple
 ;; flags. Note that these instructions will always result in more than one
@@ -2904,7 +2897,7 @@
          cmove2
          dst)))
 
-(decl cmove_or_xmm (Type CC CC XmmMemAligned Xmm) ConsumesFlags)
+(decl cmove_or_xmm (Type CC CC Xmm Xmm) ConsumesFlags)
 (rule (cmove_or_xmm ty cc1 cc2 consequent alternative)
       (let ((dst WritableXmm (temp_writable_xmm))
             (tmp WritableXmm (temp_writable_xmm))
diff --git a/cranelift/codegen/src/isa/x64/inst/emit.rs b/cranelift/codegen/src/isa/x64/inst/emit.rs
index 3f947dae2..fb4d843f3 100644
--- a/cranelift/codegen/src/isa/x64/inst/emit.rs
+++ b/cranelift/codegen/src/isa/x64/inst/emit.rs
@@ -1428,7 +1428,7 @@ pub(crate) fn emit(
             let alternative = allocs.next(alternative.to_reg());
             let dst = allocs.next(dst.to_reg().to_reg());
             debug_assert_eq!(alternative, dst);
-            let consequent = consequent.clone().to_reg_mem().with_allocs(allocs);
+            let consequent = allocs.next(consequent.clone().to_reg());
 
             // Lowering of the Select IR opcode when the input is an fcmp relies on the fact that
             // this doesn't clobber flags. Make sure to not do so here.
@@ -1447,7 +1447,7 @@ pub(crate) fn emit(
                     SseOpcode::Movdqa
                 }
             };
-            let inst = Inst::xmm_unary_rm_r(op, consequent, Writable::from_reg(dst));
+            let inst = Inst::xmm_unary_rm_r(op, consequent.into(), Writable::from_reg(dst));
             inst.emit(&[], sink, info, state);
 
             sink.bind_label(next, state.ctrl_plane_mut());
diff --git a/cranelift/codegen/src/isa/x64/inst/mod.rs b/cranelift/codegen/src/isa/x64/inst/mod.rs
index c970888d7..c69e65ea9 100644
--- a/cranelift/codegen/src/isa/x64/inst/mod.rs
+++ b/cranelift/codegen/src/isa/x64/inst/mod.rs
@@ -1624,7 +1624,7 @@ impl PrettyPrint for Inst {
                 let size = u8::try_from(ty.bytes()).unwrap();
                 let alternative = pretty_print_reg(alternative.to_reg(), size, allocs);
                 let dst = pretty_print_reg(dst.to_reg().to_reg(), size, allocs);
-                let consequent = consequent.pretty_print(size, allocs);
+                let consequent = pretty_print_reg(consequent.to_reg(), size, allocs);
                 let suffix = match *ty {
                     types::F64 => "sd",
                     types::F32 => "ss",
@@ -2334,7 +2334,7 @@ fn x64_get_operands<F: Fn(VReg) -> VReg>(inst: &Inst, collector: &mut OperandCol
         } => {
             collector.reg_use(alternative.to_reg());
             collector.reg_reuse_def(dst.to_writable_reg(), 0);
-            consequent.get_operands(collector);
+            collector.reg_use(consequent.to_reg());
         }
         Inst::Push64 { src } => {
             src.get_operands(collector);
diff --git a/cranelift/codegen/src/isa/x64/pcc.rs b/cranelift/codegen/src/isa/x64/pcc.rs
index 3952c2f4c..bc826c0d8 100644
--- a/cranelift/codegen/src/isa/x64/pcc.rs
+++ b/cranelift/codegen/src/isa/x64/pcc.rs
@@ -514,19 +514,7 @@ pub(crate) fn check(
             _ => undefined_result(ctx, vcode, dst, 64, 64),
         },
 
-        Inst::XmmCmove {
-            dst,
-            ref consequent,
-            ..
-        } => {
-            match <&RegMem>::from(consequent) {
-                RegMem::Mem { ref addr } => {
-                    check_load(ctx, None, addr, vcode, I8X16, 128)?;
-                }
-                RegMem::Reg { .. } => {}
-            }
-            ensure_no_fact(vcode, dst.to_writable_reg().to_reg())
-        }
+        Inst::XmmCmove { dst, .. } => ensure_no_fact(vcode, dst.to_writable_reg().to_reg()),
 
         Inst::Push64 { ref src } => match <&RegMemImm>::from(src) {
             RegMemImm::Mem { ref addr } => {
diff --git a/cranelift/wasm/src/code_translator.rs b/cranelift/wasm/src/code_translator.rs
index ac65abc82..f4a284d5e 100644
--- a/cranelift/wasm/src/code_translator.rs
+++ b/cranelift/wasm/src/code_translator.rs
@@ -645,7 +645,6 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
             // `table_index` is the index of the table to search the function
             // in.
             let (sigref, num_args) = state.get_indirect_sig(builder.func, *type_index, environ)?;
-            let table = state.get_or_create_table(builder.func, *table_index, environ)?;
             let callee = state.pop1();
 
             // Bitcast any vector arguments to their default type, I8X16, before calling.
@@ -655,7 +654,6 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
             let call = environ.translate_call_indirect(
                 builder,
                 TableIndex::from_u32(*table_index),
-                table,
                 TypeIndex::from_u32(*type_index),
                 sigref,
                 callee,
@@ -707,7 +705,6 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
             // `table_index` is the index of the table to search the function
             // in.
             let (sigref, num_args) = state.get_indirect_sig(builder.func, *type_index, environ)?;
-            let table = state.get_or_create_table(builder.func, *table_index, environ)?;
             let callee = state.pop1();
 
             // Bitcast any vector arguments to their default type, I8X16, before calling.
@@ -717,7 +714,6 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
             environ.translate_return_call_indirect(
                 builder,
                 TableIndex::from_u32(*table_index),
-                table,
                 TypeIndex::from_u32(*type_index),
                 sigref,
                 callee,
@@ -1542,54 +1538,43 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
             environ.translate_data_drop(builder.cursor(), *data_index)?;
         }
         Operator::TableSize { table: index } => {
-            let table = state.get_or_create_table(builder.func, *index, environ)?;
-            state.push1(environ.translate_table_size(
-                builder.cursor(),
-                TableIndex::from_u32(*index),
-                table,
-            )?);
+            state.push1(
+                environ.translate_table_size(builder.cursor(), TableIndex::from_u32(*index))?,
+            );
         }
         Operator::TableGrow { table: index } => {
             let table_index = TableIndex::from_u32(*index);
-            let table = state.get_or_create_table(builder.func, *index, environ)?;
             let delta = state.pop1();
             let init_value = state.pop1();
             state.push1(environ.translate_table_grow(
                 builder.cursor(),
                 table_index,
-                table,
                 delta,
                 init_value,
             )?);
         }
         Operator::TableGet { table: index } => {
             let table_index = TableIndex::from_u32(*index);
-            let table = state.get_or_create_table(builder.func, *index, environ)?;
             let index = state.pop1();
-            state.push1(environ.translate_table_get(builder, table_index, table, index)?);
+            state.push1(environ.translate_table_get(builder, table_index, index)?);
         }
         Operator::TableSet { table: index } => {
             let table_index = TableIndex::from_u32(*index);
-            let table = state.get_or_create_table(builder.func, *index, environ)?;
             let value = state.pop1();
             let index = state.pop1();
-            environ.translate_table_set(builder, table_index, table, value, index)?;
+            environ.translate_table_set(builder, table_index, value, index)?;
         }
         Operator::TableCopy {
             dst_table: dst_table_index,
             src_table: src_table_index,
         } => {
-            let dst_table = state.get_or_create_table(builder.func, *dst_table_index, environ)?;
-            let src_table = state.get_or_create_table(builder.func, *src_table_index, environ)?;
             let len = state.pop1();
             let src = state.pop1();
             let dest = state.pop1();
             environ.translate_table_copy(
                 builder.cursor(),
                 TableIndex::from_u32(*dst_table_index),
-                dst_table,
                 TableIndex::from_u32(*src_table_index),
-                src_table,
                 dest,
                 src,
                 len,
@@ -1606,7 +1591,6 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
             elem_index,
             table: table_index,
         } => {
-            let table = state.get_or_create_table(builder.func, *table_index, environ)?;
             let len = state.pop1();
             let src = state.pop1();
             let dest = state.pop1();
@@ -1614,7 +1598,6 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
                 builder.cursor(),
                 *elem_index,
                 TableIndex::from_u32(*table_index),
-                table,
                 dest,
                 src,
                 len,
diff --git a/cranelift/wasm/src/environ/dummy.rs b/cranelift/wasm/src/environ/dummy.rs
index 1d35f7466..a381ce33c 100644
--- a/cranelift/wasm/src/environ/dummy.rs
+++ b/cranelift/wasm/src/environ/dummy.rs
@@ -19,6 +19,7 @@ use cranelift_codegen::ir::immediates::{Offset32, Uimm64};
 use cranelift_codegen::ir::{self, InstBuilder};
 use cranelift_codegen::ir::{types::*, UserFuncName};
 use cranelift_codegen::isa::{CallConv, TargetFrontendConfig};
+use cranelift_entity::packed_option::ReservedValue;
 use cranelift_entity::{EntityRef, PrimaryMap, SecondaryMap};
 use cranelift_frontend::FunctionBuilder;
 use std::boxed::Box;
@@ -211,6 +212,9 @@ pub struct DummyFuncEnvironment<'dummy_environment> {
 
     /// Heaps we have created to implement Wasm linear memories.
     pub heaps: PrimaryMap<Heap, HeapData>,
+
+    /// Cranelift tables we have created to implement Wasm tables.
+    tables: SecondaryMap<TableIndex, ir::Table>,
 }
 
 impl<'dummy_environment> DummyFuncEnvironment<'dummy_environment> {
@@ -223,6 +227,7 @@ impl<'dummy_environment> DummyFuncEnvironment<'dummy_environment> {
             mod_info,
             expected_reachability,
             heaps: Default::default(),
+            tables: SecondaryMap::with_default(ir::Table::reserved_value()),
         }
     }
 
@@ -244,6 +249,36 @@ impl<'dummy_environment> DummyFuncEnvironment<'dummy_environment> {
             _ => panic!("unsupported pointer type"),
         }
     }
+
+    fn ensure_table_exists(&mut self, func: &mut ir::Function, index: TableIndex) {
+        if !self.tables[index].is_reserved_value() {
+            return;
+        }
+
+        // Create a table whose base address is stored at `vmctx+0`.
+        let vmctx = func.create_global_value(ir::GlobalValueData::VMContext);
+        let base_gv = func.create_global_value(ir::GlobalValueData::Load {
+            base: vmctx,
+            offset: Offset32::new(0),
+            global_type: self.pointer_type(),
+            // When tables in wasm become "growable", revisit whether this can be readonly or not.
+            flags: ir::MemFlags::trusted().with_readonly(),
+        });
+        let bound_gv = func.create_global_value(ir::GlobalValueData::Load {
+            base: vmctx,
+            offset: Offset32::new(0),
+            global_type: I32,
+            flags: ir::MemFlags::trusted().with_readonly(),
+        });
+
+        self.tables[index] = func.create_table(ir::TableData {
+            base_gv,
+            min_size: Uimm64::new(0),
+            bound_gv,
+            element_size: Uimm64::from(u64::from(self.pointer_bytes()) * 2),
+            index_type: I32,
+        });
+    }
 }
 
 impl<'dummy_environment> TypeConvert for DummyFuncEnvironment<'dummy_environment> {
@@ -316,32 +351,6 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
         }))
     }
 
-    fn make_table(&mut self, func: &mut ir::Function, _index: TableIndex) -> WasmResult<ir::Table> {
-        // Create a table whose base address is stored at `vmctx+0`.
-        let vmctx = func.create_global_value(ir::GlobalValueData::VMContext);
-        let base_gv = func.create_global_value(ir::GlobalValueData::Load {
-            base: vmctx,
-            offset: Offset32::new(0),
-            global_type: self.pointer_type(),
-            // When tables in wasm become "growable", revisit whether this can be readonly or not.
-            flags: ir::MemFlags::trusted().with_readonly(),
-        });
-        let bound_gv = func.create_global_value(ir::GlobalValueData::Load {
-            base: vmctx,
-            offset: Offset32::new(0),
-            global_type: I32,
-            flags: ir::MemFlags::trusted().with_readonly(),
-        });
-
-        Ok(func.create_table(ir::TableData {
-            base_gv,
-            min_size: Uimm64::new(0),
-            bound_gv,
-            element_size: Uimm64::from(u64::from(self.pointer_bytes()) * 2),
-            index_type: I32,
-        }))
-    }
-
     fn make_indirect_sig(
         &mut self,
         func: &mut ir::Function,
@@ -412,7 +421,6 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
         &mut self,
         builder: &mut FunctionBuilder,
         _table_index: TableIndex,
-        _table: ir::Table,
         _sig_index: TypeIndex,
         sig_ref: ir::SigRef,
         callee: ir::Value,
@@ -454,7 +462,6 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
         &mut self,
         _builder: &mut FunctionBuilder,
         _table_index: TableIndex,
-        _table: ir::Table,
         _sig_index: TypeIndex,
         _sig_ref: ir::SigRef,
         _callee: ir::Value,
@@ -574,7 +581,6 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
         &mut self,
         mut pos: FuncCursor,
         _index: TableIndex,
-        _table: ir::Table,
     ) -> WasmResult<ir::Value> {
         Ok(pos.ins().iconst(I32, -1i32 as u32 as i64))
     }
@@ -583,7 +589,6 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
         &mut self,
         mut pos: FuncCursor,
         _table_index: TableIndex,
-        _table: ir::Table,
         _delta: ir::Value,
         _init_value: ir::Value,
     ) -> WasmResult<ir::Value> {
@@ -593,11 +598,12 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
     fn translate_table_get(
         &mut self,
         builder: &mut FunctionBuilder,
-        _table_index: TableIndex,
-        table: ir::Table,
+        table_index: TableIndex,
         index: ir::Value,
     ) -> WasmResult<ir::Value> {
         let pointer_type = self.pointer_type();
+        self.ensure_table_exists(builder.func, table_index);
+        let table = self.tables[table_index];
         let table_entry_addr = builder.ins().table_addr(pointer_type, table, index, 0);
         let flags = ir::MemFlags::trusted().with_table();
         let value = builder
@@ -609,12 +615,13 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
     fn translate_table_set(
         &mut self,
         builder: &mut FunctionBuilder,
-        _table_index: TableIndex,
-        table: ir::Table,
+        table_index: TableIndex,
         value: ir::Value,
         index: ir::Value,
     ) -> WasmResult<()> {
         let pointer_type = self.pointer_type();
+        self.ensure_table_exists(builder.func, table_index);
+        let table = self.tables[table_index];
         let table_entry_addr = builder.ins().table_addr(pointer_type, table, index, 0);
         let flags = ir::MemFlags::trusted().with_table();
         builder.ins().store(flags, value, table_entry_addr, 0);
@@ -625,9 +632,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
         &mut self,
         _pos: FuncCursor,
         _dst_index: TableIndex,
-        _dst_table: ir::Table,
         _src_index: TableIndex,
-        _src_table: ir::Table,
         _dst: ir::Value,
         _src: ir::Value,
         _len: ir::Value,
@@ -651,7 +656,6 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
         _pos: FuncCursor,
         _seg_index: u32,
         _table_index: TableIndex,
-        _table: ir::Table,
         _dst: ir::Value,
         _src: ir::Value,
         _len: ir::Value,
diff --git a/cranelift/wasm/src/environ/spec.rs b/cranelift/wasm/src/environ/spec.rs
index 4841529c9..9d13f4e5a 100644
--- a/cranelift/wasm/src/environ/spec.rs
+++ b/cranelift/wasm/src/environ/spec.rs
@@ -135,12 +135,6 @@ pub trait FuncEnvironment: TargetEnvironment {
     /// The index space covers both imported and locally declared memories.
     fn make_heap(&mut self, func: &mut ir::Function, index: MemoryIndex) -> WasmResult<Heap>;
 
-    /// Set up the necessary preamble definitions in `func` to access the table identified
-    /// by `index`.
-    ///
-    /// The index space covers both imported and locally declared tables.
-    fn make_table(&mut self, func: &mut ir::Function, index: TableIndex) -> WasmResult<ir::Table>;
-
     /// Set up a signature definition in the preamble of `func` that can be used for an indirect
     /// call with signature `index`.
     ///
@@ -203,7 +197,6 @@ pub trait FuncEnvironment: TargetEnvironment {
         &mut self,
         builder: &mut FunctionBuilder,
         table_index: TableIndex,
-        table: ir::Table,
         sig_index: TypeIndex,
         sig_ref: ir::SigRef,
         callee: ir::Value,
@@ -243,7 +236,6 @@ pub trait FuncEnvironment: TargetEnvironment {
         &mut self,
         builder: &mut FunctionBuilder,
         table_index: TableIndex,
-        table: ir::Table,
         sig_index: TypeIndex,
         sig_ref: ir::SigRef,
         callee: ir::Value,
@@ -365,19 +357,14 @@ pub trait FuncEnvironment: TargetEnvironment {
     fn translate_data_drop(&mut self, pos: FuncCursor, seg_index: u32) -> WasmResult<()>;
 
     /// Translate a `table.size` WebAssembly instruction.
-    fn translate_table_size(
-        &mut self,
-        pos: FuncCursor,
-        index: TableIndex,
-        table: ir::Table,
-    ) -> WasmResult<ir::Value>;
+    fn translate_table_size(&mut self, pos: FuncCursor, index: TableIndex)
+        -> WasmResult<ir::Value>;
 
     /// Translate a `table.grow` WebAssembly instruction.
     fn translate_table_grow(
         &mut self,
         pos: FuncCursor,
         table_index: TableIndex,
-        table: ir::Table,
         delta: ir::Value,
         init_value: ir::Value,
     ) -> WasmResult<ir::Value>;
@@ -387,7 +374,6 @@ pub trait FuncEnvironment: TargetEnvironment {
         &mut self,
         builder: &mut FunctionBuilder,
         table_index: TableIndex,
-        table: ir::Table,
         index: ir::Value,
     ) -> WasmResult<ir::Value>;
 
@@ -396,7 +382,6 @@ pub trait FuncEnvironment: TargetEnvironment {
         &mut self,
         builder: &mut FunctionBuilder,
         table_index: TableIndex,
-        table: ir::Table,
         value: ir::Value,
         index: ir::Value,
     ) -> WasmResult<()>;
@@ -406,9 +391,7 @@ pub trait FuncEnvironment: TargetEnvironment {
         &mut self,
         pos: FuncCursor,
         dst_table_index: TableIndex,
-        dst_table: ir::Table,
         src_table_index: TableIndex,
-        src_table: ir::Table,
         dst: ir::Value,
         src: ir::Value,
         len: ir::Value,
@@ -430,7 +413,6 @@ pub trait FuncEnvironment: TargetEnvironment {
         pos: FuncCursor,
         seg_index: u32,
         table_index: TableIndex,
-        table: ir::Table,
         dst: ir::Value,
         src: ir::Value,
         len: ir::Value,
diff --git a/cranelift/wasm/src/state.rs b/cranelift/wasm/src/state.rs
index 3d775e05e..1d0e5a747 100644
--- a/cranelift/wasm/src/state.rs
+++ b/cranelift/wasm/src/state.rs
@@ -4,7 +4,7 @@
 //! value and control stacks during the translation of a single function.
 
 use crate::environ::{FuncEnvironment, GlobalVariable};
-use crate::{FuncIndex, GlobalIndex, Heap, MemoryIndex, TableIndex, TypeIndex, WasmResult};
+use crate::{FuncIndex, GlobalIndex, Heap, MemoryIndex, TypeIndex, WasmResult};
 use crate::{HashMap, Occupied, Vacant};
 use cranelift_codegen::ir::{self, Block, Inst, Value};
 use std::vec::Vec;
@@ -229,9 +229,6 @@ pub struct FuncTranslationState {
     // Map of heaps that have been created by `FuncEnvironment::make_heap`.
     memory_to_heap: HashMap<MemoryIndex, Heap>,
 
-    // Map of tables that have been created by `FuncEnvironment::make_table`.
-    pub(crate) tables: HashMap<TableIndex, ir::Table>,
-
     // Map of indirect call signatures that have been created by
     // `FuncEnvironment::make_indirect_sig()`.
     // Stores both the signature reference and the number of WebAssembly arguments
@@ -261,7 +258,6 @@ impl FuncTranslationState {
             reachable: true,
             globals: HashMap::new(),
             memory_to_heap: HashMap::new(),
-            tables: HashMap::new(),
             signatures: HashMap::new(),
             functions: HashMap::new(),
         }
@@ -273,7 +269,6 @@ impl FuncTranslationState {
         self.reachable = true;
         self.globals.clear();
         self.memory_to_heap.clear();
-        self.tables.clear();
         self.signatures.clear();
         self.functions.clear();
     }
@@ -472,21 +467,6 @@ impl FuncTranslationState {
         }
     }
 
-    /// Get the `Table` reference that should be used to access table `index`.
-    /// Create the reference if necessary.
-    pub(crate) fn get_or_create_table<FE: FuncEnvironment + ?Sized>(
-        &mut self,
-        func: &mut ir::Function,
-        index: u32,
-        environ: &mut FE,
-    ) -> WasmResult<ir::Table> {
-        let index = TableIndex::from_u32(index);
-        match self.tables.entry(index) {
-            Occupied(entry) => Ok(*entry.get()),
-            Vacant(entry) => Ok(*entry.insert(environ.make_table(func, index)?)),
-        }
-    }
-
     /// Get the `SigRef` reference that should be used to make an indirect call with signature
     /// `index`. Also return the number of WebAssembly arguments in the signature.
     ///
diff --git a/crates/cranelift/src/func_environ.rs b/crates/cranelift/src/func_environ.rs
index b0b0cb5ac..73576ee57 100644
--- a/crates/cranelift/src/func_environ.rs
+++ b/crates/cranelift/src/func_environ.rs
@@ -9,7 +9,8 @@ use cranelift_codegen::ir::{
     AbiParam, ArgumentPurpose, Function, InstBuilder, MemFlags, Signature, UserFuncName, Value,
 };
 use cranelift_codegen::isa::{self, CallConv, TargetFrontendConfig, TargetIsa};
-use cranelift_entity::{EntityRef, PrimaryMap};
+use cranelift_entity::packed_option::ReservedValue;
+use cranelift_entity::{EntityRef, PrimaryMap, SecondaryMap};
 use cranelift_frontend::FunctionBuilder;
 use cranelift_frontend::Variable;
 use cranelift_wasm::{
@@ -135,6 +136,9 @@ pub struct FuncEnvironment<'module_environment> {
     /// Heaps implementing WebAssembly linear memories.
     heaps: PrimaryMap<Heap, HeapData>,
 
+    /// Cranelift tables we have created to implement Wasm tables.
+    tables: SecondaryMap<TableIndex, ir::Table>,
+
     /// The Cranelift global holding the vmctx address.
     vmctx: Option<ir::GlobalValue>,
 
@@ -208,6 +212,7 @@ impl<'module_environment> FuncEnvironment<'module_environment> {
             module: &translation.module,
             types,
             heaps: PrimaryMap::default(),
+            tables: SecondaryMap::with_default(ir::Table::reserved_value()),
             vmctx: None,
             pcc_vmctx_memtype: None,
             builtin_function_signatures,
@@ -865,14 +870,82 @@ impl<'module_environment> FuncEnvironment<'module_environment> {
         }
     }
 
+    /// Set up the necessary preamble definitions in `func` to access the table identified
+    /// by `index`.
+    ///
+    /// The index space covers both imported and locally declared tables.
+    fn ensure_table_exists(&mut self, func: &mut ir::Function, index: TableIndex) {
+        if !self.tables[index].is_reserved_value() {
+            return;
+        }
+
+        let pointer_type = self.pointer_type();
+
+        let (ptr, base_offset, current_elements_offset) = {
+            let vmctx = self.vmctx(func);
+            if let Some(def_index) = self.module.defined_table_index(index) {
+                let base_offset =
+                    i32::try_from(self.offsets.vmctx_vmtable_definition_base(def_index)).unwrap();
+                let current_elements_offset = i32::try_from(
+                    self.offsets
+                        .vmctx_vmtable_definition_current_elements(def_index),
+                )
+                .unwrap();
+                (vmctx, base_offset, current_elements_offset)
+            } else {
+                let from_offset = self.offsets.vmctx_vmtable_import_from(index);
+                let table = func.create_global_value(ir::GlobalValueData::Load {
+                    base: vmctx,
+                    offset: Offset32::new(i32::try_from(from_offset).unwrap()),
+                    global_type: pointer_type,
+                    flags: MemFlags::trusted().with_readonly(),
+                });
+                let base_offset = i32::from(self.offsets.vmtable_definition_base());
+                let current_elements_offset =
+                    i32::from(self.offsets.vmtable_definition_current_elements());
+                (table, base_offset, current_elements_offset)
+            }
+        };
+
+        let base_gv = func.create_global_value(ir::GlobalValueData::Load {
+            base: ptr,
+            offset: Offset32::new(base_offset),
+            global_type: pointer_type,
+            flags: MemFlags::trusted(),
+        });
+        let bound_gv = func.create_global_value(ir::GlobalValueData::Load {
+            base: ptr,
+            offset: Offset32::new(current_elements_offset),
+            global_type: ir::Type::int(
+                u16::from(self.offsets.size_of_vmtable_definition_current_elements()) * 8,
+            )
+            .unwrap(),
+            flags: MemFlags::trusted(),
+        });
+
+        let element_size = u64::from(
+            self.reference_type(self.module.table_plans[index].table.wasm_ty.heap_type)
+                .bytes(),
+        );
+
+        self.tables[index] = func.create_table(ir::TableData {
+            base_gv,
+            min_size: Uimm64::new(0),
+            bound_gv,
+            element_size: Uimm64::new(element_size),
+            index_type: I32,
+        });
+    }
+
     fn get_or_init_func_ref_table_elem(
         &mut self,
         builder: &mut FunctionBuilder,
         table_index: TableIndex,
-        table: ir::Table,
         index: ir::Value,
     ) -> ir::Value {
         let pointer_type = self.pointer_type();
+        self.ensure_table_exists(builder.func, table_index);
+        let table = self.tables[table_index];
 
         // To support lazy initialization of table
         // contents, we check for a null entry here, and
@@ -1061,7 +1134,6 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> {
     pub fn indirect_call(
         mut self,
         table_index: TableIndex,
-        table: ir::Table,
         ty_index: TypeIndex,
         sig_ref: ir::SigRef,
         callee: ir::Value,
@@ -1072,7 +1144,7 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> {
         // Get the funcref pointer from the table.
         let funcref_ptr =
             self.env
-                .get_or_init_func_ref_table_elem(self.builder, table_index, table, callee);
+                .get_or_init_func_ref_table_elem(self.builder, table_index, callee);
 
         // Check for whether the table element is null, and trap if so.
         self.builder
@@ -1264,70 +1336,10 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
         self.epoch_ptr_var = Variable::new(num_locals + 3);
     }
 
-    fn make_table(&mut self, func: &mut ir::Function, index: TableIndex) -> WasmResult<ir::Table> {
-        let pointer_type = self.pointer_type();
-
-        let (ptr, base_offset, current_elements_offset) = {
-            let vmctx = self.vmctx(func);
-            if let Some(def_index) = self.module.defined_table_index(index) {
-                let base_offset =
-                    i32::try_from(self.offsets.vmctx_vmtable_definition_base(def_index)).unwrap();
-                let current_elements_offset = i32::try_from(
-                    self.offsets
-                        .vmctx_vmtable_definition_current_elements(def_index),
-                )
-                .unwrap();
-                (vmctx, base_offset, current_elements_offset)
-            } else {
-                let from_offset = self.offsets.vmctx_vmtable_import_from(index);
-                let table = func.create_global_value(ir::GlobalValueData::Load {
-                    base: vmctx,
-                    offset: Offset32::new(i32::try_from(from_offset).unwrap()),
-                    global_type: pointer_type,
-                    flags: MemFlags::trusted().with_readonly(),
-                });
-                let base_offset = i32::from(self.offsets.vmtable_definition_base());
-                let current_elements_offset =
-                    i32::from(self.offsets.vmtable_definition_current_elements());
-                (table, base_offset, current_elements_offset)
-            }
-        };
-
-        let base_gv = func.create_global_value(ir::GlobalValueData::Load {
-            base: ptr,
-            offset: Offset32::new(base_offset),
-            global_type: pointer_type,
-            flags: MemFlags::trusted(),
-        });
-        let bound_gv = func.create_global_value(ir::GlobalValueData::Load {
-            base: ptr,
-            offset: Offset32::new(current_elements_offset),
-            global_type: ir::Type::int(
-                u16::from(self.offsets.size_of_vmtable_definition_current_elements()) * 8,
-            )
-            .unwrap(),
-            flags: MemFlags::trusted(),
-        });
-
-        let element_size = u64::from(
-            self.reference_type(self.module.table_plans[index].table.wasm_ty.heap_type)
-                .bytes(),
-        );
-
-        Ok(func.create_table(ir::TableData {
-            base_gv,
-            min_size: Uimm64::new(0),
-            bound_gv,
-            element_size: Uimm64::new(element_size),
-            index_type: I32,
-        }))
-    }
-
     fn translate_table_grow(
         &mut self,
         mut pos: cranelift_codegen::cursor::FuncCursor<'_>,
         table_index: TableIndex,
-        _table: ir::Table,
         delta: ir::Value,
         init_value: ir::Value,
     ) -> WasmResult<ir::Value> {
@@ -1369,18 +1381,17 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
         &mut self,
         builder: &mut FunctionBuilder,
         table_index: TableIndex,
-        table: ir::Table,
         index: ir::Value,
     ) -> WasmResult<ir::Value> {
         let plan = &self.module.table_plans[table_index];
         match plan.table.wasm_ty.heap_type {
-            WasmHeapType::Func | WasmHeapType::Concrete(_) | WasmHeapType::NoFunc => match plan
-                .style
-            {
-                TableStyle::CallerChecksSignature => {
-                    Ok(self.get_or_init_func_ref_table_elem(builder, table_index, table, index))
+            WasmHeapType::Func | WasmHeapType::Concrete(_) | WasmHeapType::NoFunc => {
+                match plan.style {
+                    TableStyle::CallerChecksSignature => {
+                        Ok(self.get_or_init_func_ref_table_elem(builder, table_index, index))
+                    }
                 }
-            },
+            }
             #[cfg(feature = "gc")]
             WasmHeapType::Extern => {
                 // Our read barrier for `externref` tables is roughly equivalent
@@ -1418,6 +1429,8 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
                 builder.insert_block_after(continue_block, gc_block);
 
                 // Load the table element.
+                self.ensure_table_exists(builder.func, table_index);
+                let table = self.tables[table_index];
                 let elem_addr = builder.ins().table_addr(pointer_type, table, index, 0);
                 let flags = ir::MemFlags::trusted().with_table();
                 let elem = builder.ins().load(reference_type, flags, elem_addr, 0);
@@ -1513,12 +1526,13 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
         &mut self,
         builder: &mut FunctionBuilder,
         table_index: TableIndex,
-        table: ir::Table,
         value: ir::Value,
         index: ir::Value,
     ) -> WasmResult<()> {
         let pointer_type = self.pointer_type();
         let plan = &self.module.table_plans[table_index];
+        self.ensure_table_exists(builder.func, table_index);
+        let table = self.tables[table_index];
         match plan.table.wasm_ty.heap_type {
             WasmHeapType::Func | WasmHeapType::Concrete(_) | WasmHeapType::NoFunc => match plan
                 .style
@@ -2188,20 +2202,12 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
         &mut self,
         builder: &mut FunctionBuilder,
         table_index: TableIndex,
-        table: ir::Table,
         ty_index: TypeIndex,
         sig_ref: ir::SigRef,
         callee: ir::Value,
         call_args: &[ir::Value],
     ) -> WasmResult<ir::Inst> {
-        Call::new(builder, self).indirect_call(
-            table_index,
-            table,
-            ty_index,
-            sig_ref,
-            callee,
-            call_args,
-        )
+        Call::new(builder, self).indirect_call(table_index, ty_index, sig_ref, callee, call_args)
     }
 
     fn translate_call(
@@ -2239,7 +2245,6 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
         &mut self,
         builder: &mut FunctionBuilder,
         table_index: TableIndex,
-        table: ir::Table,
         ty_index: TypeIndex,
         sig_ref: ir::SigRef,
         callee: ir::Value,
@@ -2247,7 +2252,6 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
     ) -> WasmResult<()> {
         Call::new_tail(builder, self).indirect_call(
             table_index,
-            table,
             ty_index,
             sig_ref,
             callee,
@@ -2475,9 +2479,10 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
     fn translate_table_size(
         &mut self,
         mut pos: FuncCursor,
-        _table_index: TableIndex,
-        table: ir::Table,
+        table_index: TableIndex,
     ) -> WasmResult<ir::Value> {
+        self.ensure_table_exists(pos.func, table_index);
+        let table = self.tables[table_index];
         let size_gv = pos.func.tables[table].bound_gv;
         Ok(pos.ins().global_value(ir::types::I32, size_gv))
     }
@@ -2486,9 +2491,7 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
         &mut self,
         mut pos: FuncCursor,
         dst_table_index: TableIndex,
-        _dst_table: ir::Table,
         src_table_index: TableIndex,
-        _src_table: ir::Table,
         dst: ir::Value,
         src: ir::Value,
         len: ir::Value,
@@ -2522,7 +2525,6 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
         mut pos: FuncCursor,
         seg_index: u32,
         table_index: TableIndex,
-        _table: ir::Table,
         dst: ir::Value,
         src: ir::Value,
         len: ir::Value,
diff --git a/winch/codegen/src/isa/x64/asm.rs b/winch/codegen/src/isa/x64/asm.rs
index 2a1a84c32..a82296e67 100644
--- a/winch/codegen/src/isa/x64/asm.rs
+++ b/winch/codegen/src/isa/x64/asm.rs
@@ -492,7 +492,7 @@ impl Assembler {
         self.emit(Inst::XmmCmove {
             ty,
             cc: cc.into(),
-            consequent: XmmMemAligned::new(src.into()).expect("valid XmmMemAligned"),
+            consequent: Xmm::new(src.into()).expect("valid Xmm"),
             alternative: dst.into(),
             dst: dst.into(),
         })


Test patch:
diff --git a/cranelift/filetests/src/test_wasm/env.rs b/cranelift/filetests/src/test_wasm/env.rs
index fb76f3560..8f705d4d4 100644
--- a/cranelift/filetests/src/test_wasm/env.rs
+++ b/cranelift/filetests/src/test_wasm/env.rs
@@ -361,14 +361,6 @@ impl<'a> FuncEnvironment for FuncEnv<'a> {
         self.inner.make_global(func, index)
     }
 
-    fn make_table(
-        &mut self,
-        func: &mut ir::Function,
-        index: cranelift_wasm::TableIndex,
-    ) -> cranelift_wasm::WasmResult<ir::Table> {
-        self.inner.make_table(func, index)
-    }
-
     fn make_indirect_sig(
         &mut self,
         func: &mut ir::Function,
@@ -389,7 +381,6 @@ impl<'a> FuncEnvironment for FuncEnv<'a> {
         &mut self,
         builder: &mut cranelift_frontend::FunctionBuilder,
         table_index: cranelift_wasm::TableIndex,
-        table: ir::Table,
         sig_index: cranelift_wasm::TypeIndex,
         sig_ref: ir::SigRef,
         callee: ir::Value,
@@ -398,7 +389,6 @@ impl<'a> FuncEnvironment for FuncEnv<'a> {
         self.inner.translate_call_indirect(
             builder,
             table_index,
-            table,
             sig_index,
             sig_ref,
             callee,
@@ -410,7 +400,6 @@ impl<'a> FuncEnvironment for FuncEnv<'a> {
         &mut self,
         builder: &mut cranelift_frontend::FunctionBuilder,
         table_index: cranelift_wasm::TableIndex,
-        table: ir::Table,
         sig_index: TypeIndex,
         sig_ref: ir::SigRef,
         callee: ir::Value,
@@ -419,7 +408,6 @@ impl<'a> FuncEnvironment for FuncEnv<'a> {
         self.inner.translate_return_call_indirect(
             builder,
             table_index,
-            table,
             sig_index,
             sig_ref,
             callee,
@@ -511,67 +499,52 @@ impl<'a> FuncEnvironment for FuncEnv<'a> {
         &mut self,
         pos: cranelift_codegen::cursor::FuncCursor,
         index: cranelift_wasm::TableIndex,
-        table: ir::Table,
     ) -> cranelift_wasm::WasmResult<ir::Value> {
-        self.inner.translate_table_size(pos, index, table)
+        self.inner.translate_table_size(pos, index)
     }
 
     fn translate_table_grow(
         &mut self,
         pos: cranelift_codegen::cursor::FuncCursor,
         table_index: cranelift_wasm::TableIndex,
-        table: ir::Table,
         delta: ir::Value,
         init_value: ir::Value,
     ) -> cranelift_wasm::WasmResult<ir::Value> {
         self.inner
-            .translate_table_grow(pos, table_index, table, delta, init_value)
+            .translate_table_grow(pos, table_index, delta, init_value)
     }
 
     fn translate_table_get(
         &mut self,
         builder: &mut cranelift_frontend::FunctionBuilder,
         table_index: cranelift_wasm::TableIndex,
-        table: ir::Table,
         index: ir::Value,
     ) -> cranelift_wasm::WasmResult<ir::Value> {
-        self.inner
-            .translate_table_get(builder, table_index, table, index)
+        self.inner.translate_table_get(builder, table_index, index)
     }
 
     fn translate_table_set(
         &mut self,
         builder: &mut cranelift_frontend::FunctionBuilder,
         table_index: cranelift_wasm::TableIndex,
-        table: ir::Table,
         value: ir::Value,
         index: ir::Value,
     ) -> cranelift_wasm::WasmResult<()> {
         self.inner
-            .translate_table_set(builder, table_index, table, value, index)
+            .translate_table_set(builder, table_index, value, index)
     }
 
     fn translate_table_copy(
         &mut self,
         pos: cranelift_codegen::cursor::FuncCursor,
         dst_table_index: cranelift_wasm::TableIndex,
-        dst_table: ir::Table,
         src_table_index: cranelift_wasm::TableIndex,
-        src_table: ir::Table,
         dst: ir::Value,
         src: ir::Value,
         len: ir::Value,
     ) -> cranelift_wasm::WasmResult<()> {
-        self.inner.translate_table_copy(
-            pos,
-            dst_table_index,
-            dst_table,
-            src_table_index,
-            src_table,
-            dst,
-            src,
-            len,
-        )
+        self.inner
+            .translate_table_copy(pos, dst_table_index, src_table_index, dst, src, len)
     }
 
     fn translate_table_fill(
@@ -591,13 +564,12 @@ impl<'a> FuncEnvironment for FuncEnv<'a> {
         pos: cranelift_codegen::cursor::FuncCursor,
         seg_index: u32,
         table_index: cranelift_wasm::TableIndex,
-        table: ir::Table,
         dst: ir::Value,
         src: ir::Value,
         len: ir::Value,
     ) -> cranelift_wasm::WasmResult<()> {
         self.inner
-            .translate_table_init(pos, seg_index, table_index, table, dst, src, len)
+            .translate_table_init(pos, seg_index, table_index, dst, src, len)
     }
 
     fn translate_elem_drop(
diff --git a/tests/misc_testsuite/sink-float-but-dont-trap.wast b/tests/misc_testsuite/sink-float-but-dont-trap.wast
new file mode 100644
index 000000000..395243fcc
--- /dev/null
+++ b/tests/misc_testsuite/sink-float-but-dont-trap.wast
@@ -0,0 +1,51 @@
+(module
+  (memory 1)
+
+  ;; make sure that the sunk load here doesn't try to load past the end of
+  ;; memory.
+  (func (export "select-with-sink") (param i32) (result f64)
+    local.get 0
+    f64.load
+    f64.const 1
+    local.get 0
+    select
+    return)
+
+  ;; same as above but with a slightly different codegen pattern.
+  (func (export "select-with-fcmp-and-sink") (param i32 f64 f64) (result f64)
+    local.get 0
+    f64.load
+    f64.const 1
+    local.get 1
+    local.get 2
+    f64.ne
+    select
+    return)
+
+  ;; Same as the above two but the order of operands to the `select` are
+  ;; swapped.
+  (func (export "select-with-sink-other-way") (param i32) (result f64)
+    f64.const 1
+    local.get 0
+    f64.load
+    local.get 0
+    select
+    return)
+  (func (export "select-with-fcmp-and-sink-other-way") (param i32 f64 f64) (result f64)
+    f64.const 1
+    local.get 0
+    f64.load
+    local.get 1
+    local.get 2
+    f64.ne
+    select
+    return)
+)
+
+(assert_return (invoke "select-with-sink" (i32.const 0xfff8)) (f64.const 0))
+(assert_return (invoke "select-with-fcmp-and-sink" (i32.const 0xfff8) (f64.const 0) (f64.const 0)) (f64.const 1))
+
+(assert_trap (invoke "select-with-sink" (i32.const 0xfff9)) "out of bounds")
+(assert_trap (invoke "select-with-fcmp-and-sink" (i32.const 0xfff9) (f64.const 0) (f64.const 0)) "out of bounds")
+(assert_trap (invoke "select-with-sink-other-way" (i32.const 0xfff9)) "out of bounds")
+(assert_trap (invoke "select-with-fcmp-and-sink-other-way" (i32.const 0xfff9) (f64.const 0) (f64.const 0)) "out of bounds")