From 25315a80517a8eb9e47451262f4e3bef9954c136 Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Mon, 10 Feb 2025 14:53:41 -0800 Subject: [PATCH] asm: add types for sign-extended immediates As pointed out in #10200, it could be confusing for users for `cranelift-assembler-x64` to pass unsigned integers to certain assembler instructions and have them unexpectedly sign-extended. Well, it can't be too surprising since these instructions have a `_sx*` suffix, but this change implements @alexcrichton's additional suggestion to create separate types for the immediates that may be sign-extended. These new types (`Simm8`, `Simm16`, `Simm32`) are quite similar to their vanilla counterparts (`Imm8`, `Imm16`, `Imm32`) but have additional sign-extension logic when pretty-printed. This means the vanilla versions can be simplified and the pre-existing `Simm32` is renamed to the more appropriate `AmodeOffset`. --- .../assembler-x64/meta/src/dsl/format.rs | 10 +- cranelift/assembler-x64/meta/src/generate.rs | 3 + .../assembler-x64/meta/src/generate/inst.rs | 9 +- .../meta/src/generate/operand.rs | 27 ++- cranelift/assembler-x64/src/fuzz.rs | 6 +- cranelift/assembler-x64/src/imm.rs | 157 +++++++++--------- cranelift/assembler-x64/src/inst.rs | 2 +- cranelift/assembler-x64/src/lib.rs | 4 +- cranelift/assembler-x64/src/mem.rs | 89 +++++++++- cranelift/codegen/src/isa/x64/inst.isle | 8 +- .../codegen/src/isa/x64/inst/external.rs | 6 +- cranelift/codegen/src/isa/x64/lower/isle.rs | 48 ++++-- .../filetests/filetests/isa/x64/i128.clif | 2 +- .../filetests/filetests/isa/x64/ishl.clif | 10 +- .../filetests/isa/x64/simd-arith-avx.clif | 6 +- .../isa/x64/simd-bitwise-compile.clif | 4 +- .../filetests/filetests/isa/x64/sshr.clif | 10 +- .../filetests/filetests/isa/x64/ushr.clif | 10 +- 18 files changed, 274 insertions(+), 137 deletions(-) diff --git a/cranelift/assembler-x64/meta/src/dsl/format.rs b/cranelift/assembler-x64/meta/src/dsl/format.rs index 90faefe36d8c..d4c7c8a34dde 100644 --- a/cranelift/assembler-x64/meta/src/dsl/format.rs +++ b/cranelift/assembler-x64/meta/src/dsl/format.rs @@ -349,7 +349,14 @@ pub enum Extension { SignExtendQuad, SignExtendLong, SignExtendWord, - ZeroExtend, +} + +impl Extension { + /// Check if the extension is sign-extended. + #[must_use] + pub fn is_sign_extended(&self) -> bool { + matches!(self, Self::SignExtendQuad | Self::SignExtendLong | Self::SignExtendWord) + } } impl Default for Extension { @@ -365,7 +372,6 @@ impl core::fmt::Display for Extension { Extension::SignExtendQuad => write!(f, "sxq"), Extension::SignExtendLong => write!(f, "sxl"), Extension::SignExtendWord => write!(f, "sxw"), - Extension::ZeroExtend => write!(f, "zx"), } } } diff --git a/cranelift/assembler-x64/meta/src/generate.rs b/cranelift/assembler-x64/meta/src/generate.rs index 36c67ce8c969..8021f6044943 100644 --- a/cranelift/assembler-x64/meta/src/generate.rs +++ b/cranelift/assembler-x64/meta/src/generate.rs @@ -53,8 +53,11 @@ pub fn isle_macro(f: &mut Formatter, insts: &[dsl::Inst]) { /// above. pub fn isle_definitions(f: &mut Formatter, insts: &[dsl::Inst]) { f.line("(type AssemblerImm8 extern (enum))", None); + f.line("(type AssemblerSimm8 extern (enum))", None); f.line("(type AssemblerImm16 extern (enum))", None); + f.line("(type AssemblerSimm16 extern (enum))", None); f.line("(type AssemblerImm32 extern (enum))", None); + f.line("(type AssemblerSimm32 extern (enum))", None); f.line("(type AssemblerReadGpr extern (enum))", None); f.line("(type AssemblerReadWriteGpr extern (enum))", None); f.line("(type AssemblerReadGprMem extern (enum))", None); diff --git a/cranelift/assembler-x64/meta/src/generate/inst.rs b/cranelift/assembler-x64/meta/src/generate/inst.rs index d69407132660..006cee9f9717 100644 --- a/cranelift/assembler-x64/meta/src/generate/inst.rs +++ b/cranelift/assembler-x64/meta/src/generate/inst.rs @@ -281,7 +281,14 @@ impl dsl::Inst { .iter() .filter_map(|o| match o.location.kind() { FixedReg(_) => None, - Imm(loc) => Some(format!("AssemblerImm{}", loc.bits())), + Imm(loc) => { + let bits = loc.bits(); + if o.extension.is_sign_extended() { + Some(format!("AssemblerSimm{bits}")) + } else { + Some(format!("AssemblerImm{bits}")) + } + } Reg(_) => Some(format!("Assembler{}Gpr", o.mutability.generate_type())), RegMem(_) => Some(format!("Assembler{}GprMem", o.mutability.generate_type())), }) diff --git a/cranelift/assembler-x64/meta/src/generate/operand.rs b/cranelift/assembler-x64/meta/src/generate/operand.rs index bed9141d4dfd..212cf70e8172 100644 --- a/cranelift/assembler-x64/meta/src/generate/operand.rs +++ b/cranelift/assembler-x64/meta/src/generate/operand.rs @@ -6,7 +6,14 @@ impl dsl::Operand { use dsl::OperandKind::*; match self.location.kind() { FixedReg(_) => None, - Imm(loc) => Some(format!("Imm{}", loc.bits())), + Imm(loc) => { + let bits = loc.bits(); + if self.extension.is_sign_extended() { + Some(format!("Simm{bits}")) + } else { + Some(format!("Imm{bits}")) + } + } Reg(_) => Some(format!("Gpr", self.mutability.generate_type())), RegMem(_) => Some(format!("GprMem", self.mutability.generate_type())), } @@ -22,7 +29,14 @@ impl dsl::Operand { }; match self.location.kind() { FixedReg(_) => None, - Imm(loc) => Some(format!("Imm{}", loc.bits())), + Imm(loc) => { + let bits = loc.bits(); + if self.extension.is_sign_extended() { + Some(format!("Simm{bits}")) + } else { + Some(format!("Imm{bits}")) + } + } Reg(_) => Some(format!("Gpr<{pick_ty}>")), RegMem(_) => Some(format!("GprMem<{pick_ty}, {read_ty}>")), } @@ -58,8 +72,12 @@ impl dsl::Location { eax => "\"%eax\"".into(), rax => "\"%rax\"".into(), imm8 | imm16 | imm32 => { - let variant = extension.generate_variant(); - format!("self.{self}.to_string({variant})") + if extension.is_sign_extended() { + let variant = extension.generate_variant(); + format!("self.{self}.to_string({variant})") + } else { + format!("self.{self}.to_string()") + } } r8 | r16 | r32 | r64 | rm8 | rm16 | rm32 | rm64 => match self.generate_size() { Some(size) => format!("self.{self}.to_string({size})"), @@ -120,7 +138,6 @@ impl dsl::Extension { SignExtendWord => "Extension::SignExtendWord", SignExtendLong => "Extension::SignExtendLong", SignExtendQuad => "Extension::SignExtendQuad", - ZeroExtend => "Extension::ZeroExtend", } } } diff --git a/cranelift/assembler-x64/src/fuzz.rs b/cranelift/assembler-x64/src/fuzz.rs index ae7727266c0c..4568ecf6fb08 100644 --- a/cranelift/assembler-x64/src/fuzz.rs +++ b/cranelift/assembler-x64/src/fuzz.rs @@ -4,7 +4,7 @@ //! throughout this crate to avoid depending on the `arbitrary` crate //! unconditionally (use the `fuzz` feature instead). -use crate::{AsReg, Gpr, Inst, NonRspGpr, Registers, Simm32, Simm32PlusKnownOffset}; +use crate::{AmodeOffset, AsReg, Gpr, Inst, NonRspGpr, Registers, AmodeOffsetPlusKnownOffset}; use arbitrary::{Arbitrary, Result, Unstructured}; use capstone::{arch::x86, arch::BuildsCapstone, arch::BuildsCapstoneSyntax, Capstone}; @@ -175,11 +175,11 @@ impl AsReg for FuzzReg { } } -impl Arbitrary<'_> for Simm32PlusKnownOffset { +impl Arbitrary<'_> for AmodeOffsetPlusKnownOffset { fn arbitrary(u: &mut Unstructured<'_>) -> Result { // For now, we don't generate offsets (TODO). Ok(Self { - simm32: Simm32::arbitrary(u)?, + simm32: AmodeOffset::arbitrary(u)?, offset: None, }) } diff --git a/cranelift/assembler-x64/src/imm.rs b/cranelift/assembler-x64/src/imm.rs index 57b11d24fd67..6e7fc2e78857 100644 --- a/cranelift/assembler-x64/src/imm.rs +++ b/cranelift/assembler-x64/src/imm.rs @@ -2,7 +2,8 @@ #![allow(clippy::module_name_repetitions)] -use crate::api::{CodeSink, KnownOffset, KnownOffsetTable}; +use crate::api::CodeSink; +use std::fmt; /// This helper function prints the unsigned hexadecimal representation of the /// immediate value: e.g., this prints `$0xfe` to represent both the signed `-2` @@ -41,16 +42,42 @@ impl Imm8 { pub fn encode(&self, sink: &mut impl CodeSink) { sink.put1(self.0); } +} + +impl fmt::Display for Imm8 { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "$0x{:x}", self.0) + } +} + +/// A _signed_ 8-bit immediate operand (suitable for sign extension). +#[derive(Clone, Copy, Debug)] +#[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))] +pub struct Simm8(i8); + +impl Simm8 { + #[must_use] + pub fn new(value: i8) -> Self { + Self(value) + } + + #[must_use] + pub fn value(&self) -> i8 { + self.0 + } + + pub fn encode(&self, sink: &mut impl CodeSink) { + sink.put1(self.0 as u8); + } #[must_use] pub fn to_string(&self, extend: Extension) -> String { - use Extension::{None, SignExtendLong, SignExtendQuad, SignExtendWord, ZeroExtend}; + use Extension::{None, SignExtendLong, SignExtendQuad, SignExtendWord}; match extend { None => hexify!(self.0), SignExtendWord => hexify_sign_extend!(self.0, i8 => i16), SignExtendLong => hexify_sign_extend!(self.0, i8 => i32), SignExtendQuad => hexify_sign_extend!(self.0, i8 => i64), - ZeroExtend => hexify!(u64::from(self.0)), } } } @@ -74,16 +101,42 @@ impl Imm16 { pub fn encode(&self, sink: &mut impl CodeSink) { sink.put2(self.0); } +} + +impl fmt::Display for Imm16 { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "$0x{:x}", self.0) + } +} + +/// A _signed_ 16-bit immediate operand (suitable for sign extension). +#[derive(Clone, Debug)] +#[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))] +pub struct Simm16(i16); + +impl Simm16 { + #[must_use] + pub fn new(value: i16) -> Self { + Self(value) + } + + #[must_use] + pub fn value(&self) -> i16 { + self.0 + } + + pub fn encode(&self, sink: &mut impl CodeSink) { + sink.put2(self.0 as u16); + } #[must_use] pub fn to_string(&self, extend: Extension) -> String { - use Extension::{None, SignExtendLong, SignExtendQuad, SignExtendWord, ZeroExtend}; + use Extension::{None, SignExtendLong, SignExtendQuad, SignExtendWord}; match extend { None => hexify!(self.0), SignExtendWord => unreachable!("the 16-bit value is already 16 bits"), SignExtendLong => hexify_sign_extend!(self.0, i16 => i32), SignExtendQuad => hexify_sign_extend!(self.0, i16 => i64), - ZeroExtend => hexify!(u64::from(self.0)), } } } @@ -111,23 +164,20 @@ impl Imm32 { pub fn encode(&self, sink: &mut impl CodeSink) { sink.put4(self.0); } +} - #[must_use] - pub fn to_string(&self, extend: Extension) -> String { - use Extension::{None, SignExtendLong, SignExtendQuad, SignExtendWord, ZeroExtend}; - match extend { - None => hexify!(self.0), - SignExtendWord => unreachable!("cannot sign extend a 32-bit value to 16 bits"), - SignExtendLong => unreachable!("the 32-bit value is already 32 bits"), - SignExtendQuad => hexify_sign_extend!(self.0, i32 => i64), - ZeroExtend => hexify!(u64::from(self.0)), - } +impl fmt::Display for Imm32 { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "$0x{:x}", self.0) } } -/// A 32-bit immediate like [`Imm32`], but with slightly different -/// pretty-printing. -#[derive(Clone, Copy, Debug)] +/// A _signed_ 32-bit immediate operand (suitable for sign extension). +/// +/// Note that, "in 64-bit mode, the typical size of immediate operands remains +/// 32 bits. When the operand size is 64 bits, the processor sign-extends all +/// immediates to 64 bits prior to their use" (Intel SDM Vol. 2, 2.2.1.5). +#[derive(Clone, Debug)] #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))] pub struct Simm32(i32); @@ -138,73 +188,23 @@ impl Simm32 { } #[must_use] - pub fn value(self) -> i32 { + pub fn value(&self) -> i32 { self.0 } -} - -impl From for Simm32 { - fn from(value: i32) -> Self { - Self(value) - } -} -impl std::fmt::LowerHex for Simm32 { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - // This rather complex implementation is necessary to match how - // `capstone` pretty-prints memory immediates. - if self.0 == 0 { - return Ok(()); - } - if self.0 < 0 { - write!(f, "-")?; - } - if self.0 > 9 || self.0 < -9 { - write!(f, "0x")?; - } - let abs = match self.0.checked_abs() { - Some(i) => i, - None => -2_147_483_648, - }; - std::fmt::LowerHex::fmt(&abs, f) + pub fn encode(&self, sink: &mut impl CodeSink) { + sink.put4(self.0 as u32); } -} - -/// A [`Simm32`] immediate with an optional known offset. -/// -/// Cranelift does not know certain offsets until emission time. To accommodate -/// Cranelift, this structure stores an optional [`KnownOffset`]. The following -/// happens immediately before emission: -/// - the [`KnownOffset`] is looked up, mapping it to an offset value -/// - the [`Simm32`] value is added to the offset value -#[derive(Clone, Debug)] -pub struct Simm32PlusKnownOffset { - pub simm32: Simm32, - pub offset: Option, -} -impl Simm32PlusKnownOffset { - /// # Panics - /// - /// Panics if the sum of the immediate and the known offset value overflows. #[must_use] - pub fn value(&self, offsets: &impl KnownOffsetTable) -> i32 { - let known_offset = match self.offset { - Some(offset) => offsets[offset], - None => 0, - }; - known_offset - .checked_add(self.simm32.value()) - .expect("no wrapping") - } -} - -impl std::fmt::LowerHex for Simm32PlusKnownOffset { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - if let Some(offset) = self.offset { - write!(f, "+")?; + pub fn to_string(&self, extend: Extension) -> String { + use Extension::{None, SignExtendLong, SignExtendQuad, SignExtendWord}; + match extend { + None => hexify!(self.0), + SignExtendWord => unreachable!("cannot sign extend a 32-bit value to 16 bits"), + SignExtendLong => unreachable!("the 32-bit value is already 32 bits"), + SignExtendQuad => hexify_sign_extend!(self.0, i32 => i64), } - std::fmt::LowerHex::fmt(&self.simm32, f) } } @@ -215,5 +215,4 @@ pub enum Extension { SignExtendQuad, SignExtendLong, SignExtendWord, - ZeroExtend, } diff --git a/cranelift/assembler-x64/src/inst.rs b/cranelift/assembler-x64/src/inst.rs index cadabbca8772..6f0284bf4181 100644 --- a/cranelift/assembler-x64/src/inst.rs +++ b/cranelift/assembler-x64/src/inst.rs @@ -4,7 +4,7 @@ //! See also: [`Inst`], an `enum` containing all these instructions. use crate::api::{AsReg, CodeSink, KnownOffsetTable, RegisterVisitor, Registers}; -use crate::imm::{Extension, Imm16, Imm32, Imm8}; +use crate::imm::{Extension, Imm16, Imm32, Imm8, Simm32, Simm8}; use crate::mem::{emit_modrm_sib_disp, GprMem}; use crate::reg::{self, Gpr, Size}; use crate::rex::{self, RexFlags}; diff --git a/cranelift/assembler-x64/src/lib.rs b/cranelift/assembler-x64/src/lib.rs index d182d748a85b..0d5c8aa3142f 100644 --- a/cranelift/assembler-x64/src/lib.rs +++ b/cranelift/assembler-x64/src/lib.rs @@ -72,8 +72,8 @@ pub use api::{ AsReg, CodeSink, Constant, KnownOffset, KnownOffsetTable, Label, RegisterVisitor, Registers, TrapCode, }; -pub use imm::{Extension, Imm16, Imm32, Imm8, Simm32, Simm32PlusKnownOffset}; -pub use mem::{Amode, DeferredTarget, GprMem, Scale}; +pub use imm::{Extension, Imm16, Imm32, Imm8, Simm16, Simm32, Simm8}; +pub use mem::{Amode, AmodeOffset, AmodeOffsetPlusKnownOffset, DeferredTarget, GprMem, Scale}; pub use reg::{Gpr, NonRspGpr, Size}; pub use rex::RexFlags; diff --git a/cranelift/assembler-x64/src/mem.rs b/cranelift/assembler-x64/src/mem.rs index 1535d37f6581..3bc942a4ddcd 100644 --- a/cranelift/assembler-x64/src/mem.rs +++ b/cranelift/assembler-x64/src/mem.rs @@ -1,7 +1,6 @@ //! Memory operands to instructions. -use crate::api::{AsReg, CodeSink, Constant, KnownOffsetTable, Label, TrapCode}; -use crate::imm::{Simm32, Simm32PlusKnownOffset}; +use crate::api::{AsReg, CodeSink, Constant, KnownOffset, KnownOffsetTable, Label, TrapCode}; use crate::reg::{self, NonRspGpr, Size}; use crate::rex::{encode_modrm, encode_sib, Imm, RexFlags}; @@ -11,14 +10,14 @@ use crate::rex::{encode_modrm, encode_sib, Imm, RexFlags}; pub enum Amode { ImmReg { base: R, - simm32: Simm32PlusKnownOffset, + simm32: AmodeOffsetPlusKnownOffset, trap: Option, }, ImmRegRegShift { base: R, index: NonRspGpr, scale: Scale, - simm32: Simm32, + simm32: AmodeOffset, trap: Option, }, RipRelative { @@ -73,6 +72,88 @@ impl Amode { } } +/// A 32-bit immediate for address offsets. +#[derive(Clone, Copy, Debug)] +#[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))] +pub struct AmodeOffset(i32); + +impl AmodeOffset { + #[must_use] + pub fn new(value: i32) -> Self { + Self(value) + } + + #[must_use] + pub fn value(self) -> i32 { + self.0 + } +} + +impl From for AmodeOffset { + fn from(value: i32) -> Self { + Self(value) + } +} + +impl std::fmt::LowerHex for AmodeOffset { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + // This rather complex implementation is necessary to match how + // `capstone` pretty-prints memory immediates. + if self.0 == 0 { + return Ok(()); + } + if self.0 < 0 { + write!(f, "-")?; + } + if self.0 > 9 || self.0 < -9 { + write!(f, "0x")?; + } + let abs = match self.0.checked_abs() { + Some(i) => i, + None => -2_147_483_648, + }; + std::fmt::LowerHex::fmt(&abs, f) + } +} + +/// An [`AmodeOffset`] immediate with an optional known offset. +/// +/// Cranelift does not know certain offsets until emission time. To accommodate +/// Cranelift, this structure stores an optional [`KnownOffset`]. The following +/// happens immediately before emission: +/// - the [`KnownOffset`] is looked up, mapping it to an offset value +/// - the [`Simm32`] value is added to the offset value +#[derive(Clone, Debug)] +pub struct AmodeOffsetPlusKnownOffset { + pub simm32: AmodeOffset, + pub offset: Option, +} + +impl AmodeOffsetPlusKnownOffset { + /// # Panics + /// + /// Panics if the sum of the immediate and the known offset value overflows. + #[must_use] + pub fn value(&self, offsets: &impl KnownOffsetTable) -> i32 { + let known_offset = match self.offset { + Some(offset) => offsets[offset], + None => 0, + }; + known_offset + .checked_add(self.simm32.value()) + .expect("no wrapping") + } +} + +impl std::fmt::LowerHex for AmodeOffsetPlusKnownOffset { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + if let Some(offset) = self.offset { + write!(f, "+")?; + } + std::fmt::LowerHex::fmt(&self.simm32, f) + } +} + /// For RIP-relative addressing, keep track of the [`CodeSink`]-specific target. #[derive(Clone, Debug)] #[cfg_attr(any(test, feature = "fuzz"), derive(arbitrary::Arbitrary))] diff --git a/cranelift/codegen/src/isa/x64/inst.isle b/cranelift/codegen/src/isa/x64/inst.isle index b9d907e0f6e6..9f961e22e279 100644 --- a/cranelift/codegen/src/isa/x64/inst.isle +++ b/cranelift/codegen/src/isa/x64/inst.isle @@ -2914,10 +2914,16 @@ ;; Helpers for matching operands, extracting them into their assembler types. (decl is_imm8 (AssemblerImm8) GprMemImm) (extern extractor is_imm8 is_imm8) +(decl is_simm8 (AssemblerSimm8) GprMemImm) +(extern extractor is_simm8 is_simm8) (decl is_imm16 (AssemblerImm16) GprMemImm) (extern extractor is_imm16 is_imm16) +(decl is_simm16 (AssemblerSimm16) GprMemImm) +(extern extractor is_simm16 is_simm16) (decl is_imm32 (AssemblerImm32) GprMemImm) (extern extractor is_imm32 is_imm32) +(decl is_simm32 (AssemblerSimm32) GprMemImm) +(extern extractor is_simm32 is_simm32) (decl is_gpr (AssemblerReadGprMem) GprMemImm) (extern extractor is_gpr is_gpr) (decl is_mem (AssemblerReadGprMem) GprMemImm) @@ -2951,7 +2957,7 @@ (rule 10 (x64_and $I8 src1 (is_imm8 src2)) (x64_andb_mi src1 src2)) (rule 9 (x64_and $I16 src1 (is_imm16 src2)) (x64_andw_mi src1 src2)) (rule 8 (x64_and $I32 src1 (is_imm32 src2)) (x64_andl_mi src1 src2)) -(rule 7 (x64_and $I64 src1 (is_imm32 src2)) (x64_andq_mi_sxl src1 src2)) +(rule 7 (x64_and $I64 src1 (is_simm32 src2)) (x64_andq_mi_sxl src1 src2)) (rule 6 (x64_and $I8 src1 (is_gpr src2)) (x64_andl_rm src1 src2)) (rule 5 (x64_and $I8 src1 (is_mem src2)) (x64_andb_rm src1 src2)) (rule 4 (x64_and $I16 src1 (is_gpr src2)) (x64_andl_rm src1 src2)) diff --git a/cranelift/codegen/src/isa/x64/inst/external.rs b/cranelift/codegen/src/isa/x64/inst/external.rs index d9d49af902b1..664f31f32f2d 100644 --- a/cranelift/codegen/src/isa/x64/inst/external.rs +++ b/cranelift/codegen/src/isa/x64/inst/external.rs @@ -114,7 +114,7 @@ impl Into> for SyntheticAmode { base, flags, } => asm::Amode::ImmReg { - simm32: asm::Simm32PlusKnownOffset { + simm32: asm::AmodeOffsetPlusKnownOffset { simm32: simm32.into(), offset: None, }, @@ -140,7 +140,7 @@ impl Into> for SyntheticAmode { }, SyntheticAmode::IncomingArg { offset } => asm::Amode::ImmReg { base: Gpr::unwrap_new(regs::rbp()), - simm32: asm::Simm32PlusKnownOffset { + simm32: asm::AmodeOffsetPlusKnownOffset { simm32: (-i32::try_from(offset).unwrap()).into(), offset: Some(offsets::KEY_INCOMING_ARG), }, @@ -148,7 +148,7 @@ impl Into> for SyntheticAmode { }, SyntheticAmode::SlotOffset { simm32 } => asm::Amode::ImmReg { base: Gpr::unwrap_new(regs::rbp()), - simm32: asm::Simm32PlusKnownOffset { + simm32: asm::AmodeOffsetPlusKnownOffset { simm32: simm32.into(), offset: Some(offsets::KEY_SLOT_OFFSET), }, diff --git a/cranelift/codegen/src/isa/x64/lower/isle.rs b/cranelift/codegen/src/isa/x64/lower/isle.rs index b5ef8cfcc92b..bea748a26339 100644 --- a/cranelift/codegen/src/isa/x64/lower/isle.rs +++ b/cranelift/codegen/src/isa/x64/lower/isle.rs @@ -47,8 +47,11 @@ type AssemblerReadGprMem = asm::GprMem; type AssemblerReadWriteGprMem = asm::GprMem; type AssemblerInst = asm::Inst; type AssemblerImm8 = asm::Imm8; +type AssemblerSimm8 = asm::Simm8; type AssemblerImm16 = asm::Imm16; +type AssemblerSimm16 = asm::Simm16; type AssemblerImm32 = asm::Imm32; +type AssemblerSimm32 = asm::Simm32; pub struct SinkableLoad { inst: Inst, @@ -962,20 +965,18 @@ impl Context for IsleContext<'_, '_, MInst, X64Backend> { fn is_imm8(&mut self, src: &GprMemImm) -> Option { match src.clone().to_reg_mem_imm() { RegMemImm::Imm { simm32 } => { - // TODO fix down-convert logic: if an assembly instruction can - // only fit 8 bits, we check if down-converting the 32 bits from - // CLIF we have here will fit. Some assembler instructions will - // sign-extend this immediate, however, and we don't have a way - // to distinguish this yet. For a CLIF value `-2i8`, this - // conversion will both pass on the appropriate bytes and the - // emitted instruction will sign-extend them as expected. But, - // for a CLIF value `254u8` (the same bit pattern), we could - // pass on the right bits but the sign-extension will break - // Cranelift's semantics. For the time being, we conservatively - // only allow down-converting to `i8` values, meaning some valid - // constants will be rejected. + let imm = u8::try_from(simm32).ok()?; + Some(AssemblerImm8::new(imm)) + } + _ => None, + } + } + + fn is_simm8(&mut self, src: &GprMemImm) -> Option { + match src.clone().to_reg_mem_imm() { + RegMemImm::Imm { simm32 } => { let imm = i8::try_from(simm32).ok()?; - Some(AssemblerImm8::new(imm as u8)) + Some(AssemblerSimm8::new(imm)) } _ => None, } @@ -984,13 +985,23 @@ impl Context for IsleContext<'_, '_, MInst, X64Backend> { fn is_imm16(&mut self, src: &GprMemImm) -> Option { match src.clone().to_reg_mem_imm() { RegMemImm::Imm { simm32 } => { - // TODO fix down-convert logic: see notes in `is_imm8`. + let imm = u16::try_from(simm32).ok()?; + Some(AssemblerImm16::new(imm)) + } + _ => None, + } + } + + fn is_simm16(&mut self, src: &GprMemImm) -> Option { + match src.clone().to_reg_mem_imm() { + RegMemImm::Imm { simm32 } => { let imm = i16::try_from(simm32).ok()?; - Some(AssemblerImm16::new(imm as u16)) + Some(AssemblerSimm16::new(imm)) } _ => None, } } + fn is_imm32(&mut self, src: &GprMemImm) -> Option { match src.clone().to_reg_mem_imm() { RegMemImm::Imm { simm32 } => Some(AssemblerImm32::new(simm32)), @@ -998,6 +1009,13 @@ impl Context for IsleContext<'_, '_, MInst, X64Backend> { } } + fn is_simm32(&mut self, src: &GprMemImm) -> Option { + match src.clone().to_reg_mem_imm() { + RegMemImm::Imm { simm32 } => Some(AssemblerSimm32::new(simm32 as i32)), + _ => None, + } + } + fn is_gpr(&mut self, src: &GprMemImm) -> Option { match src.clone().to_reg_mem_imm() { RegMemImm::Reg { reg } => { diff --git a/cranelift/filetests/filetests/isa/x64/i128.clif b/cranelift/filetests/filetests/isa/x64/i128.clif index 1404a8beabf7..b4b2dd68d717 100644 --- a/cranelift/filetests/filetests/isa/x64/i128.clif +++ b/cranelift/filetests/filetests/isa/x64/i128.clif @@ -1431,7 +1431,7 @@ block0(v0: i8, v1: i128): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; shlb %cl, %al, %al ; movq %rbp, %rsp diff --git a/cranelift/filetests/filetests/isa/x64/ishl.clif b/cranelift/filetests/filetests/isa/x64/ishl.clif index ea9feb9e2993..0c26c6b2d72b 100644 --- a/cranelift/filetests/filetests/isa/x64/ishl.clif +++ b/cranelift/filetests/filetests/isa/x64/ishl.clif @@ -391,7 +391,7 @@ block0(v0: i8, v1: i128): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; shlb %cl, %al, %al ; movq %rbp, %rsp @@ -778,7 +778,7 @@ block0(v0: i8, v1: i64): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; shlb %cl, %al, %al ; movq %rbp, %rsp @@ -809,7 +809,7 @@ block0(v0: i8, v1: i32): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; shlb %cl, %al, %al ; movq %rbp, %rsp @@ -840,7 +840,7 @@ block0(v0: i8, v1: i16): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; shlb %cl, %al, %al ; movq %rbp, %rsp @@ -871,7 +871,7 @@ block0(v0: i8, v1: i8): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; shlb %cl, %al, %al ; movq %rbp, %rsp diff --git a/cranelift/filetests/filetests/isa/x64/simd-arith-avx.clif b/cranelift/filetests/filetests/isa/x64/simd-arith-avx.clif index e03bf86581f1..f6a36d0a86a7 100644 --- a/cranelift/filetests/filetests/isa/x64/simd-arith-avx.clif +++ b/cranelift/filetests/filetests/isa/x64/simd-arith-avx.clif @@ -914,7 +914,7 @@ block0(v0: i8x16, v1: i32): ; pushq %rbp ; movq %rsp, %rbp ; block0: -; andq $7, %rdi +; andq $0x7, %rdi ; vpunpcklbw %xmm0, %xmm0, %xmm5 ; vpunpckhbw %xmm0, %xmm0, %xmm7 ; addl %edi, $8, %edi @@ -1381,7 +1381,7 @@ block0(v0: i8x16, v1: i32): ; pushq %rbp ; movq %rsp, %rbp ; block0: -; andq $7, %rdi +; andq $0x7, %rdi ; vmovd %edi, %xmm5 ; vpsllw %xmm0, %xmm5, %xmm7 ; lea const(0), %rsi @@ -1618,7 +1618,7 @@ block0(v0: i8x16, v1: i32): ; pushq %rbp ; movq %rsp, %rbp ; block0: -; andq $7, %rdi +; andq $0x7, %rdi ; vmovd %edi, %xmm5 ; vpsrlw %xmm0, %xmm5, %xmm7 ; lea const(0), %rsi diff --git a/cranelift/filetests/filetests/isa/x64/simd-bitwise-compile.clif b/cranelift/filetests/filetests/isa/x64/simd-bitwise-compile.clif index 0b8328102747..e0a129a5a2d4 100644 --- a/cranelift/filetests/filetests/isa/x64/simd-bitwise-compile.clif +++ b/cranelift/filetests/filetests/isa/x64/simd-bitwise-compile.clif @@ -325,7 +325,7 @@ block0(v0: i32): ; movq %rsp, %rbp ; block0: ; movdqu const(1), %xmm0 -; andq $7, %rdi +; andq $0x7, %rdi ; movd %edi, %xmm5 ; psllw %xmm0, %xmm5, %xmm0 ; lea const(0), %rsi @@ -598,7 +598,7 @@ block0(v0: i32): ; movq %rsp, %rbp ; block0: ; movdqu const(0), %xmm1 -; andq $7, %rdi +; andq $0x7, %rdi ; movdqa %xmm1, %xmm0 ; punpcklbw %xmm0, %xmm1, %xmm0 ; punpckhbw %xmm1, %xmm1, %xmm1 diff --git a/cranelift/filetests/filetests/isa/x64/sshr.clif b/cranelift/filetests/filetests/isa/x64/sshr.clif index 53d0d456ea17..58a1c71d11ed 100644 --- a/cranelift/filetests/filetests/isa/x64/sshr.clif +++ b/cranelift/filetests/filetests/isa/x64/sshr.clif @@ -430,7 +430,7 @@ block0(v0: i8, v1: i128): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; sarb %cl, %al, %al ; movq %rbp, %rsp @@ -817,7 +817,7 @@ block0(v0: i8, v1: i64): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; sarb %cl, %al, %al ; movq %rbp, %rsp @@ -848,7 +848,7 @@ block0(v0: i8, v1: i32): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; sarb %cl, %al, %al ; movq %rbp, %rsp @@ -879,7 +879,7 @@ block0(v0: i8, v1: i16): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; sarb %cl, %al, %al ; movq %rbp, %rsp @@ -910,7 +910,7 @@ block0(v0: i8, v1: i8): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; sarb %cl, %al, %al ; movq %rbp, %rsp diff --git a/cranelift/filetests/filetests/isa/x64/ushr.clif b/cranelift/filetests/filetests/isa/x64/ushr.clif index a26ba9868afa..3fa265f0707c 100644 --- a/cranelift/filetests/filetests/isa/x64/ushr.clif +++ b/cranelift/filetests/filetests/isa/x64/ushr.clif @@ -400,7 +400,7 @@ block0(v0: i8, v1: i128): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; shrb %cl, %al, %al ; movq %rbp, %rsp @@ -787,7 +787,7 @@ block0(v0: i8, v1: i64): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; shrb %cl, %al, %al ; movq %rbp, %rsp @@ -818,7 +818,7 @@ block0(v0: i8, v1: i32): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; shrb %cl, %al, %al ; movq %rbp, %rsp @@ -849,7 +849,7 @@ block0(v0: i8, v1: i16): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; shrb %cl, %al, %al ; movq %rbp, %rsp @@ -880,7 +880,7 @@ block0(v0: i8, v1: i8): ; movq %rsp, %rbp ; block0: ; movq %rsi, %rcx -; andq $7, %rcx +; andq $0x7, %rcx ; movq %rdi, %rax ; shrb %cl, %al, %al ; movq %rbp, %rsp