From 26ce9a38534a02b1c8defa1d1d414151709ce554 Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Wed, 9 Mar 2022 11:10:59 -0800 Subject: [PATCH] Fix uextend on x64 for non-i32-source cases. (#3906) In #3849, I moved uextend over to ISLE in the x64 backend. Unfortunately, the lowering patterns had a bug in the i32-to-i64 special case (when we know the generating instruction zeroes the upper 32 bits): it wasn't actually special casing for an i32 source! This meant that e.g. zero extends of the results of i8 adds did not work properly. This PR fixes the bug and updates the runtest for extends significantly to cover the narrow-value cases. No security impact to Wasm as Wasm does not use narrow integer types. Thanks @bjorn3 for reporting! --- cranelift/codegen/src/isa/x64/lower.isle | 20 +- .../x64/lower/isle/generated_code.manifest | 2 +- .../src/isa/x64/lower/isle/generated_code.rs | 186 ++++++++------- .../filetests/filetests/runtests/extend.clif | 222 ++++++++++++++++-- 4 files changed, 313 insertions(+), 117 deletions(-) diff --git a/cranelift/codegen/src/isa/x64/lower.isle b/cranelift/codegen/src/isa/x64/lower.isle index d082fda8ef..cb7636939d 100644 --- a/cranelift/codegen/src/isa/x64/lower.isle +++ b/cranelift/codegen/src/isa/x64/lower.isle @@ -1894,34 +1894,34 @@ ;; keep these here than write an external extractor containing bits of ;; the instruction pattern.s) (rule (lower (has_type $I64 - (uextend src @ (iadd _ _)))) + (uextend src @ (has_type $I32 (iadd _ _))))) src) (rule (lower (has_type $I64 - (uextend src @ (iadd_ifcout _ _)))) + (uextend src @ (has_type $I32 (iadd_ifcout _ _))))) src) (rule (lower (has_type $I64 - (uextend src @ (isub _ _)))) + (uextend src @ (has_type $I32 (isub _ _))))) src) (rule (lower (has_type $I64 - (uextend src @ (imul _ _)))) + (uextend src @ (has_type $I32 (imul _ _))))) src) (rule (lower (has_type $I64 - (uextend src @ (band _ _)))) + (uextend src @ (has_type $I32 (band _ _))))) src) (rule (lower (has_type $I64 - (uextend src @ (bor _ _)))) + (uextend src @ (has_type $I32 (bor _ _))))) src) (rule (lower (has_type $I64 - (uextend src @ (bxor _ _)))) + (uextend src @ (has_type $I32 (bxor _ _))))) src) (rule (lower (has_type $I64 - (uextend src @ (ishl _ _)))) + (uextend src @ (has_type $I32 (ishl _ _))))) src) (rule (lower (has_type $I64 - (uextend src @ (ushr _ _)))) + (uextend src @ (has_type $I32 (ushr _ _))))) src) (rule (lower (has_type $I64 - (uextend src @ (uload32 _ _ _)))) + (uextend src @ (has_type $I32 (uload32 _ _ _))))) src) ;; Rules for `sextend` / `bextend` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/cranelift/codegen/src/isa/x64/lower/isle/generated_code.manifest b/cranelift/codegen/src/isa/x64/lower/isle/generated_code.manifest index b1f03e50d8..ebb1176c91 100644 --- a/cranelift/codegen/src/isa/x64/lower/isle/generated_code.manifest +++ b/cranelift/codegen/src/isa/x64/lower/isle/generated_code.manifest @@ -1,4 +1,4 @@ src/clif.isle 9ea75a6f790b5c03 src/prelude.isle b2bc986bcbbbb77 src/isa/x64/inst.isle 40f495d3ca5ae547 -src/isa/x64/lower.isle faa2a07bba48a813 +src/isa/x64/lower.isle c049f7d36db0e0fb diff --git a/cranelift/codegen/src/isa/x64/lower/isle/generated_code.rs b/cranelift/codegen/src/isa/x64/lower/isle/generated_code.rs index b409fad5c4..ab52c9cadd 100644 --- a/cranelift/codegen/src/isa/x64/lower/isle/generated_code.rs +++ b/cranelift/codegen/src/isa/x64/lower/isle/generated_code.rs @@ -4665,101 +4665,107 @@ pub fn constructor_lower(ctx: &mut C, arg0: Inst) -> Option { if let Some(pattern7_0) = C::def_inst(ctx, pattern5_1) { - let pattern8_0 = C::inst_data(ctx, pattern7_0); - match &pattern8_0 { - &InstructionData::Binary { - opcode: ref pattern9_0, - args: ref pattern9_1, - } => { - match pattern9_0 { - &Opcode::Iadd => { - let (pattern11_0, pattern11_1) = - C::unpack_value_array_2(ctx, pattern9_1); - // Rule at src/isa/x64/lower.isle line 1896. - let expr0_0 = - constructor_output_value(ctx, pattern5_1)?; - return Some(expr0_0); + if let Some(pattern8_0) = C::first_result(ctx, pattern7_0) { + let pattern9_0 = C::value_type(ctx, pattern8_0); + if pattern9_0 == I32 { + let pattern11_0 = C::inst_data(ctx, pattern7_0); + match &pattern11_0 { + &InstructionData::Binary { + opcode: ref pattern12_0, + args: ref pattern12_1, + } => { + match pattern12_0 { + &Opcode::Iadd => { + let (pattern14_0, pattern14_1) = + C::unpack_value_array_2(ctx, pattern12_1); + // Rule at src/isa/x64/lower.isle line 1896. + let expr0_0 = + constructor_output_value(ctx, pattern5_1)?; + return Some(expr0_0); + } + &Opcode::Isub => { + let (pattern14_0, pattern14_1) = + C::unpack_value_array_2(ctx, pattern12_1); + // Rule at src/isa/x64/lower.isle line 1902. + let expr0_0 = + constructor_output_value(ctx, pattern5_1)?; + return Some(expr0_0); + } + &Opcode::Imul => { + let (pattern14_0, pattern14_1) = + C::unpack_value_array_2(ctx, pattern12_1); + // Rule at src/isa/x64/lower.isle line 1905. + let expr0_0 = + constructor_output_value(ctx, pattern5_1)?; + return Some(expr0_0); + } + &Opcode::IaddIfcout => { + let (pattern14_0, pattern14_1) = + C::unpack_value_array_2(ctx, pattern12_1); + // Rule at src/isa/x64/lower.isle line 1899. + let expr0_0 = + constructor_output_value(ctx, pattern5_1)?; + return Some(expr0_0); + } + &Opcode::Band => { + let (pattern14_0, pattern14_1) = + C::unpack_value_array_2(ctx, pattern12_1); + // Rule at src/isa/x64/lower.isle line 1908. + let expr0_0 = + constructor_output_value(ctx, pattern5_1)?; + return Some(expr0_0); + } + &Opcode::Bor => { + let (pattern14_0, pattern14_1) = + C::unpack_value_array_2(ctx, pattern12_1); + // Rule at src/isa/x64/lower.isle line 1911. + let expr0_0 = + constructor_output_value(ctx, pattern5_1)?; + return Some(expr0_0); + } + &Opcode::Bxor => { + let (pattern14_0, pattern14_1) = + C::unpack_value_array_2(ctx, pattern12_1); + // Rule at src/isa/x64/lower.isle line 1914. + let expr0_0 = + constructor_output_value(ctx, pattern5_1)?; + return Some(expr0_0); + } + &Opcode::Ishl => { + let (pattern14_0, pattern14_1) = + C::unpack_value_array_2(ctx, pattern12_1); + // Rule at src/isa/x64/lower.isle line 1917. + let expr0_0 = + constructor_output_value(ctx, pattern5_1)?; + return Some(expr0_0); + } + &Opcode::Ushr => { + let (pattern14_0, pattern14_1) = + C::unpack_value_array_2(ctx, pattern12_1); + // Rule at src/isa/x64/lower.isle line 1920. + let expr0_0 = + constructor_output_value(ctx, pattern5_1)?; + return Some(expr0_0); + } + _ => {} + } } - &Opcode::Isub => { - let (pattern11_0, pattern11_1) = - C::unpack_value_array_2(ctx, pattern9_1); - // Rule at src/isa/x64/lower.isle line 1902. - let expr0_0 = - constructor_output_value(ctx, pattern5_1)?; - return Some(expr0_0); - } - &Opcode::Imul => { - let (pattern11_0, pattern11_1) = - C::unpack_value_array_2(ctx, pattern9_1); - // Rule at src/isa/x64/lower.isle line 1905. - let expr0_0 = - constructor_output_value(ctx, pattern5_1)?; - return Some(expr0_0); - } - &Opcode::IaddIfcout => { - let (pattern11_0, pattern11_1) = - C::unpack_value_array_2(ctx, pattern9_1); - // Rule at src/isa/x64/lower.isle line 1899. - let expr0_0 = - constructor_output_value(ctx, pattern5_1)?; - return Some(expr0_0); - } - &Opcode::Band => { - let (pattern11_0, pattern11_1) = - C::unpack_value_array_2(ctx, pattern9_1); - // Rule at src/isa/x64/lower.isle line 1908. - let expr0_0 = - constructor_output_value(ctx, pattern5_1)?; - return Some(expr0_0); - } - &Opcode::Bor => { - let (pattern11_0, pattern11_1) = - C::unpack_value_array_2(ctx, pattern9_1); - // Rule at src/isa/x64/lower.isle line 1911. - let expr0_0 = - constructor_output_value(ctx, pattern5_1)?; - return Some(expr0_0); - } - &Opcode::Bxor => { - let (pattern11_0, pattern11_1) = - C::unpack_value_array_2(ctx, pattern9_1); - // Rule at src/isa/x64/lower.isle line 1914. - let expr0_0 = - constructor_output_value(ctx, pattern5_1)?; - return Some(expr0_0); - } - &Opcode::Ishl => { - let (pattern11_0, pattern11_1) = - C::unpack_value_array_2(ctx, pattern9_1); - // Rule at src/isa/x64/lower.isle line 1917. - let expr0_0 = - constructor_output_value(ctx, pattern5_1)?; - return Some(expr0_0); - } - &Opcode::Ushr => { - let (pattern11_0, pattern11_1) = - C::unpack_value_array_2(ctx, pattern9_1); - // Rule at src/isa/x64/lower.isle line 1920. - let expr0_0 = - constructor_output_value(ctx, pattern5_1)?; - return Some(expr0_0); + &InstructionData::Load { + opcode: ref pattern12_0, + arg: pattern12_1, + flags: pattern12_2, + offset: pattern12_3, + } => { + if let &Opcode::Uload32 = pattern12_0 { + // Rule at src/isa/x64/lower.isle line 1923. + let expr0_0 = + constructor_output_value(ctx, pattern5_1)?; + return Some(expr0_0); + } } _ => {} } } - &InstructionData::Load { - opcode: ref pattern9_0, - arg: pattern9_1, - flags: pattern9_2, - offset: pattern9_3, - } => { - if let &Opcode::Uload32 = pattern9_0 { - // Rule at src/isa/x64/lower.isle line 1923. - let expr0_0 = constructor_output_value(ctx, pattern5_1)?; - return Some(expr0_0); - } - } - _ => {} } } let pattern7_0 = C::value_type(ctx, pattern5_1); diff --git a/cranelift/filetests/filetests/runtests/extend.clif b/cranelift/filetests/filetests/runtests/extend.clif index 524177de10..42e77f7f0b 100644 --- a/cranelift/filetests/filetests/runtests/extend.clif +++ b/cranelift/filetests/filetests/runtests/extend.clif @@ -1,23 +1,213 @@ test run target aarch64 -target arm target s390x target x86_64 -function %uextend() -> b1 { -block0: - v0 = iconst.i32 0xffff_ee00 - v1 = uextend.i64 v0 - v2 = icmp_imm eq v1, 0xffff_ee00 - return v2 -} -; run +;;;; basic uextend -function %sextend() -> b1 { -block0: - v0 = iconst.i32 0xffff_ee00 - v1 = sextend.i64 v0 - v2 = icmp_imm eq v1, 0xffff_ffff_ffff_ee00 - return v2 +function %uextend8_16(i8) -> i16 { +block0(v0: i8): + v1 = uextend.i16 v0 + return v1 } -; run +; run: %uextend8_16(0xfe) == 0xfe +; run: %uextend8_16(0x7e) == 0x7e + +function %uextend8_32(i8) -> i32 { +block0(v0: i8): + v1 = uextend.i32 v0 + return v1 +} +; run: %uextend8_32(0xfe) == 0xfe +; run: %uextend8_32(0x7e) == 0x7e + +function %uextend16_32(i16) -> i32 { +block0(v0: i16): + v1 = uextend.i32 v0 + return v1 +} +; run: %uextend16_32(0xfe00) == 0xfe00 +; run: %uextend16_32(0x7e00) == 0x7e00 + +function %uextend8_64(i8) -> i64 { +block0(v0: i8): + v1 = uextend.i64 v0 + return v1 +} +; run: %uextend8_64(0xfe) == 0xfe +; run: %uextend8_64(0x7e) == 0x7e + +function %uextend16_64(i16) -> i64 { +block0(v0: i16): + v1 = uextend.i64 v0 + return v1 +} +; run: %uextend16_64(0xfe00) == 0xfe00 +; run: %uextend16_64(0x7e00) == 0x7e00 + +function %uextend32_64(i32) -> i64 { +block0(v0: i32): + v1 = uextend.i64 v0 + return v1 +} +; run: %uextend32_64(0xffff_ee00) == 0xffff_ee00 +; run: %uextend32_64(0x7fff_ee00) == 0x7fff_ee00 + +;;;; basic sextend + +function %sextend8_16(i8) -> i16 { +block0(v0: i8): + v1 = sextend.i16 v0 + return v1 +} +; run: %sextend8_16(0xff) == 0xffff +; run: %sextend8_16(0x7f) == 0x7f + +function %sextend8_32(i8) -> i32 { +block0(v0: i8): + v1 = sextend.i32 v0 + return v1 +} +; run: %sextend8_32(0xff) == 0xffff_ffff +; run: %sextend8_32(0x7f) == 0x7f + +function %sextend16_32(i16) -> i32 { +block0(v0: i16): + v1 = sextend.i32 v0 + return v1 +} +; run: %sextend16_32(0xfe00) == 0xffff_fe00 +; run: %sextend16_32(0x7e00) == 0x7e00 + +function %sextend8_64(i8) -> i64 { +block0(v0: i8): + v1 = sextend.i64 v0 + return v1 +} +; run: %sextend8_64(0xff) == 0xffff_ffff_ffff_ffff +; run: %sextend8_64(0x7f) == 0x7f + +function %sextend16_64(i16) -> i64 { +block0(v0: i16): + v1 = sextend.i64 v0 + return v1 +} +; run: %sextend16_64(0xfe00) == 0xffff_ffff_ffff_fe00 +; run: %sextend16_64(0x7e00) == 0x7e00 + +function %sextend32_64(i32) -> i64 { +block0(v0: i32): + v1 = sextend.i64 v0 + return v1 +} +; run: %sextend32_64(0xffff_ee00) == 0xffff_ffff_ffff_ee00 +; run: %sextend32_64(0x7fff_ee00) == 0x7fff_ee00 + +;; uextend of an `add` that we know is likely to set undefined bits +;; above the narrow value + +function %add_uextend8_16(i8, i8) -> i16 { +block0(v0: i8, v1: i8): + v2 = iadd.i8 v0, v1 + v3 = uextend.i16 v2 + return v3 +} +; run: %add_uextend8_16(0xfe, 0x03) == 0x0001 + +function %add_uextend8_32(i8, i8) -> i32 { +block0(v0: i8, v1: i8): + v2 = iadd.i8 v0, v1 + v3 = uextend.i32 v2 + return v3 +} +; run: %add_uextend8_32(0xfe, 0x03) == 0x0000_0001 + +function %add_uextend16_32(i16, i16) -> i32 { +block0(v0: i16, v1: i16): + v2 = iadd.i16 v0, v1 + v3 = uextend.i32 v2 + return v3 +} +; run: %add_uextend16_32(0xfe00, 0x302) == 0x0000_0102 + +function %add_uextend8_64(i8, i8) -> i64 { +block0(v0: i8, v1: i8): + v2 = iadd.i8 v0, v1 + v3 = uextend.i64 v2 + return v3 +} +; run: %add_uextend8_64(0xfe, 0x03) == 0x0000_0000_0000_0001 + +function %add_uextend16_64(i16, i16) -> i64 { +block0(v0: i16, v1: i16): + v2 = iadd.i16 v0, v1 + v3 = uextend.i64 v2 + return v3 +} +; run: %add_uextend16_64(0xfe00, 0x302) == 0x0000_0000_0000_0102 + +function %add_uextend32_64(i32, i32) -> i64 { +block0(v0: i32, v1: i32): + v2 = iadd.i32 v0, v1 + v3 = uextend.i64 v2 + return v3 +} +; run: %add_uextend32_64(0xffff_ee00, 0x1000_0001) == 0x0000_0000_0fff_ee01 + +;; sextend of an `add` that we know is likely to set undefined bits +;; above the narrow value + +function %add_sextend8_16(i8, i8) -> i16 { +block0(v0: i8, v1: i8): + v2 = iadd.i8 v0, v1 + v3 = sextend.i16 v2 + return v3 +} +; run: %add_sextend8_16(0xfe, 0x03) == 0x0001 +; run: %add_sextend8_16(0xfe, 0x83) == 0xff81 + +function %add_sextend8_32(i8, i8) -> i32 { +block0(v0: i8, v1: i8): + v2 = iadd.i8 v0, v1 + v3 = sextend.i32 v2 + return v3 +} +; run: %add_sextend8_32(0xfe, 0x03) == 0x0000_0001 +; run: %add_sextend8_32(0xfe, 0x83) == 0xffff_ff81 + +function %add_sextend16_32(i16, i16) -> i32 { +block0(v0: i16, v1: i16): + v2 = iadd.i16 v0, v1 + v3 = sextend.i32 v2 + return v3 +} +; run: %add_sextend16_32(0xfe00, 0x302) == 0x0000_0102 +; run: %add_sextend16_32(0xfe00, 0x8302) == 0xffff_8102 + +function %add_sextend8_64(i8, i8) -> i64 { +block0(v0: i8, v1: i8): + v2 = iadd.i8 v0, v1 + v3 = sextend.i64 v2 + return v3 +} +; run: %add_sextend8_64(0xfe, 0x03) == 0x0000_0000_0000_0001 +; run: %add_sextend8_64(0xfe, 0x83) == 0xffff_ffff_ffff_ff81 + +function %add_sextend16_64(i16, i16) -> i64 { +block0(v0: i16, v1: i16): + v2 = iadd.i16 v0, v1 + v3 = sextend.i64 v2 + return v3 +} +; run: %add_sextend16_64(0xfe00, 0x302) == 0x0000_0000_0000_0102 +; run: %add_sextend16_64(0xfe00, 0x8302) == 0xffff_ffff_ffff_8102 + +function %add_sextend32_64(i32, i32) -> i64 { +block0(v0: i32, v1: i32): + v2 = iadd.i32 v0, v1 + v3 = sextend.i64 v2 + return v3 +} +; run: %add_sextend32_64(0xffff_ee00, 0x1000_0001) == 0x0000_0000_0fff_ee01 +; run: %add_sextend32_64(0xffff_ee00, 0x9000_0001) == 0xffff_ffff_8fff_ee01 +