From a371355e142fe6492e527595e5cb7c5a8556a2c8 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 | 216 ++++++++++++++++-- 4 files changed, 310 insertions(+), 114 deletions(-) diff --git a/cranelift/codegen/src/isa/x64/lower.isle b/cranelift/codegen/src/isa/x64/lower.isle index d082fda8ef5e..cb7636939d6f 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 b1f03e50d893..ebb1176c915f 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 b409fad5c4b3..ab52c9cadd61 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); - } - &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); + 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::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 524177de1054..42e77f7f0bb6 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 +;;;; basic uextend + +function %uextend8_16(i8) -> i16 { +block0(v0: i8): + v1 = uextend.i16 v0 + return v1 +} +; 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 - v2 = icmp_imm eq v1, 0xffff_ee00 - return v2 + return v1 } -; run +; 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 %sextend() -> b1 { -block0: - v0 = iconst.i32 0xffff_ee00 +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 - v2 = icmp_imm eq v1, 0xffff_ffff_ffff_ee00 - return v2 + return v1 } -; run +; 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 +