From 4155d15e69eb4f5fbf39758560283f5a0362d12a Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Sat, 2 May 2020 15:03:40 -0700 Subject: [PATCH] Fix masking of vector shift values Previously, the logic was wrong on two counts: - It used the bits of the entire vector (e.g. i32x4 -> 128) instead of just the lane bits (e.g. i32x4 -> 32). - It used the type of the first operand before it was bitcast to its correct type. Remember that, by default, vectors are handed around as i8x16 and we must bitcast them to their correct type for Cranelift's verifier; see https://github.com/bytecodealliance/wasmtime/issues/1147 for discussion on this. This fix simply uses the type of the instruction itself, which is equivalent and hopefully less fragile to any changes. --- cranelift/wasm/src/code_translator.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cranelift/wasm/src/code_translator.rs b/cranelift/wasm/src/code_translator.rs index f98dee0232..7513e456e3 100644 --- a/cranelift/wasm/src/code_translator.rs +++ b/cranelift/wasm/src/code_translator.rs @@ -1402,7 +1402,7 @@ pub fn translate_operator( Operator::I8x16Shl | Operator::I16x8Shl | Operator::I32x4Shl | Operator::I64x2Shl => { let (a, b) = state.pop2(); let bitcast_a = optionally_bitcast_vector(a, type_of(op), builder); - let bitwidth = i64::from(builder.func.dfg.value_type(a).bits()); + let bitwidth = i64::from(type_of(op).lane_bits()); // The spec expects to shift with `b mod lanewidth`; so, e.g., for 16 bit lane-width // we do `b AND 15`; this means fewer instructions than `iconst + urem`. let b_mod_bitwidth = builder.ins().band_imm(b, bitwidth - 1); @@ -1411,16 +1411,16 @@ pub fn translate_operator( Operator::I8x16ShrU | Operator::I16x8ShrU | Operator::I32x4ShrU | Operator::I64x2ShrU => { let (a, b) = state.pop2(); let bitcast_a = optionally_bitcast_vector(a, type_of(op), builder); - let bitwidth = i64::from(builder.func.dfg.value_type(a).bits()); + let bitwidth = i64::from(type_of(op).lane_bits()); // The spec expects to shift with `b mod lanewidth`; so, e.g., for 16 bit lane-width // we do `b AND 15`; this means fewer instructions than `iconst + urem`. let b_mod_bitwidth = builder.ins().band_imm(b, bitwidth - 1); state.push1(builder.ins().ushr(bitcast_a, b_mod_bitwidth)) } - Operator::I8x16ShrS | Operator::I16x8ShrS | Operator::I32x4ShrS => { + Operator::I8x16ShrS | Operator::I16x8ShrS | Operator::I32x4ShrS | Operator::I64x2ShrS => { let (a, b) = state.pop2(); let bitcast_a = optionally_bitcast_vector(a, type_of(op), builder); - let bitwidth = i64::from(builder.func.dfg.value_type(a).bits()); + let bitwidth = i64::from(type_of(op).lane_bits()); // The spec expects to shift with `b mod lanewidth`; so, e.g., for 16 bit lane-width // we do `b AND 15`; this means fewer instructions than `iconst + urem`. let b_mod_bitwidth = builder.ins().band_imm(b, bitwidth - 1); @@ -1544,7 +1544,6 @@ pub fn translate_operator( } Operator::I8x16Mul | Operator::I64x2Mul - | Operator::I64x2ShrS | Operator::I32x4TruncSatF32x4S | Operator::I32x4TruncSatF32x4U | Operator::I64x2TruncSatF64x2S