From 27532410d2778a90b2ab0df1e2b5e57c4e657b4d Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Fri, 13 Mar 2020 15:11:41 -0700 Subject: [PATCH] Fix local.set and local.tee types for SIMD Because Wasm SIMD vectors store their type as `v128`, there is a mismatch between the more specific types Cranelift uses and Wasm SIMD. Because of this mismatch, Wasm SIMD translates to the default Cranelift type `I8X16`, causing issues when more specific type information is available (e.g. `I32x4`). To fix this, all incoming values to SIMD instructions are checked during translation (not runtime) and if necessary cast from `I8X16` to the appropriate type by functions like `optionally_bitcast_vector`, `pop1_with_bitcast` and `pop2_with_bitcast`. However, there are times when we must also cast to `I8X16` for outgoing values, as with `local.set` and `local.tee`. There are other ways of resolving this (e.g., see adding a new vector type, https://github.com/bytecodealliance/cranelift/pull/1251) but we discussed staying with this casting approach in https://github.com/bytecodealliance/wasmtime/issues/1147. --- cranelift/wasm/src/code_translator.rs | 18 ++++++++++++++++-- cranelift/wasmtests/simd.wat | 6 ++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/cranelift/wasm/src/code_translator.rs b/cranelift/wasm/src/code_translator.rs index b20d3091e7..2c237e7404 100644 --- a/cranelift/wasm/src/code_translator.rs +++ b/cranelift/wasm/src/code_translator.rs @@ -73,13 +73,27 @@ pub fn translate_operator( builder.set_val_label(val, label); } Operator::LocalSet { local_index } => { - let val = state.pop1(); + let mut val = state.pop1(); + + // Ensure SIMD values are cast to their default Cranelift type, I8x16. + let ty = builder.func.dfg.value_type(val); + if ty.is_vector() { + val = optionally_bitcast_vector(val, I8X16, builder); + } + builder.def_var(Variable::with_u32(*local_index), val); let label = ValueLabel::from_u32(*local_index); builder.set_val_label(val, label); } Operator::LocalTee { local_index } => { - let val = state.peek1(); + let mut val = state.peek1(); + + // Ensure SIMD values are cast to their default Cranelift type, I8x16. + let ty = builder.func.dfg.value_type(val); + if ty.is_vector() { + val = optionally_bitcast_vector(val, I8X16, builder); + } + builder.def_var(Variable::with_u32(*local_index), val); let label = ValueLabel::from_u32(*local_index); builder.set_val_label(val, label); diff --git a/cranelift/wasmtests/simd.wat b/cranelift/wasmtests/simd.wat index 99b7d5c10d..0e40648993 100644 --- a/cranelift/wasmtests/simd.wat +++ b/cranelift/wasmtests/simd.wat @@ -17,6 +17,12 @@ i32x4.extract_lane 3 ) + (func $test_locals (local i32 v128) + local.get 0 + i32x4.splat + local.set 1 + ) + (export "test_splat" (func $test_splat)) (export "test_insert_lane" (func $test_insert_lane)) (export "test_const" (func $test_const))