Skip to content

Commit

Permalink
Address review comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
@cfallin
cfallin committed Jun 3, 2020
1 parent 6153620 commit fe97659
Show file tree
Hide file tree
Showing 13 changed files with 224 additions and 169 deletions.
179 changes: 97 additions & 82 deletions cranelift/codegen/src/isa/aarch64/abi.rs
View file

Large diffs are not rendered by default.

28 changes: 12 additions & 16 deletions cranelift/codegen/src/isa/aarch64/inst/args.rs
View file
Expand Up @@ -145,11 +145,15 @@ pub enum MemArg {
/// Reference to a "label": e.g., a symbol.
Label(MemLabel),

/// Arbitrary offset from a register. Converted to generation of large
/// offsets with multiple instructions as necessary during code emission.
RegOffset(Reg, i64, Type),

/// Offset from the stack pointer.
SPOffset(i64),
SPOffset(i64, Type),

/// Offset from the frame pointer.
FPOffset(i64),
FPOffset(i64, Type),

/// Offset from the "nominal stack pointer", which is where the real SP is
/// just after stack and spill slots are allocated in the function prologue.
Expand All @@ -163,7 +167,7 @@ pub enum MemArg {
/// SP" is where the actual SP is after the function prologue and before
/// clobber pushes. See the diagram in the documentation for
/// [crate::isa::aarch64::abi](the ABI module) for more details.
NominalSPOffset(i64),
NominalSPOffset(i64, Type),
}

impl MemArg {
Expand All @@ -174,17 +178,6 @@ impl MemArg {
MemArg::UnsignedOffset(reg, UImm12Scaled::zero(I64))
}

/// Memory reference using an address in a register and an offset, if possible.
pub fn reg_maybe_offset(reg: Reg, offset: i64, value_type: Type) -> Option<MemArg> {
if let Some(simm9) = SImm9::maybe_from_i64(offset) {
Some(MemArg::Unscaled(reg, simm9))
} else if let Some(uimm12s) = UImm12Scaled::maybe_from_i64(offset, value_type) {
Some(MemArg::UnsignedOffset(reg, uimm12s))
} else {
None
}
}

/// Memory reference using the sum of two registers as an address.
pub fn reg_plus_reg(reg1: Reg, reg2: Reg) -> MemArg {
MemArg::RegReg(reg1, reg2)
Expand Down Expand Up @@ -431,8 +424,11 @@ impl ShowWithRRU for MemArg {
simm9.show_rru(mb_rru)
),
// Eliminated by `mem_finalize()`.
&MemArg::SPOffset(..) | &MemArg::FPOffset(..) | &MemArg::NominalSPOffset(..) => {
panic!("Unexpected stack-offset mem-arg mode!")
&MemArg::SPOffset(..)
| &MemArg::FPOffset(..)
| &MemArg::NominalSPOffset(..)
| &MemArg::RegOffset(..) => {
panic!("Unexpected pseudo mem-arg mode (stack-offset or generic reg-offset)!")
}
}
}
Expand Down
70 changes: 37 additions & 33 deletions cranelift/codegen/src/isa/aarch64/inst/emit.rs
View file
Expand Up @@ -5,6 +5,7 @@ use crate::ir::constant::ConstantData;
use crate::ir::types::*;
use crate::ir::TrapCode;
use crate::isa::aarch64::inst::*;
use crate::isa::aarch64::lower::ty_bits;

use regalloc::{Reg, RegClass, Writable};

Expand All @@ -29,8 +30,12 @@ pub fn mem_finalize(
state: &EmitState,
) -> (SmallVec<[Inst; 4]>, MemArg) {
match mem {
&MemArg::SPOffset(off) | &MemArg::FPOffset(off) | &MemArg::NominalSPOffset(off) => {
&MemArg::RegOffset(_, off, ty)
| &MemArg::SPOffset(off, ty)
| &MemArg::FPOffset(off, ty)
| &MemArg::NominalSPOffset(off, ty) => {
let basereg = match mem {
&MemArg::RegOffset(reg, _, _) => reg,
&MemArg::SPOffset(..) | &MemArg::NominalSPOffset(..) => stack_reg(),
&MemArg::FPOffset(..) => fp_reg(),
_ => unreachable!(),
Expand All @@ -52,6 +57,9 @@ pub fn mem_finalize(
if let Some(simm9) = SImm9::maybe_from_i64(off) {
let mem = MemArg::Unscaled(basereg, simm9);
(smallvec![], mem)
} else if let Some(uimm12s) = UImm12Scaled::maybe_from_i64(off, ty) {
let mem = MemArg::UnsignedOffset(basereg, uimm12s);
(smallvec![], mem)
} else {
let tmp = writable_spilltmp_reg();
let mut const_insts = Inst::load_constant(tmp, off as u64);
Expand Down Expand Up @@ -654,17 +662,17 @@ impl MachInstEmit for Inst {
// This is the base opcode (top 10 bits) for the "unscaled
// immediate" form (Unscaled). Other addressing modes will OR in
// other values for bits 24/25 (bits 1/2 of this constant).
let op = match self {
&Inst::ULoad8 { .. } => 0b0011100001,
&Inst::SLoad8 { .. } => 0b0011100010,
&Inst::ULoad16 { .. } => 0b0111100001,
&Inst::SLoad16 { .. } => 0b0111100010,
&Inst::ULoad32 { .. } => 0b1011100001,
&Inst::SLoad32 { .. } => 0b1011100010,
&Inst::ULoad64 { .. } => 0b1111100001,
&Inst::FpuLoad32 { .. } => 0b1011110001,
&Inst::FpuLoad64 { .. } => 0b1111110001,
&Inst::FpuLoad128 { .. } => 0b0011110011,
let (op, bits) = match self {
&Inst::ULoad8 { .. } => (0b0011100001, 8),
&Inst::SLoad8 { .. } => (0b0011100010, 8),
&Inst::ULoad16 { .. } => (0b0111100001, 16),
&Inst::SLoad16 { .. } => (0b0111100010, 16),
&Inst::ULoad32 { .. } => (0b1011100001, 32),
&Inst::SLoad32 { .. } => (0b1011100010, 32),
&Inst::ULoad64 { .. } => (0b1111100001, 64),
&Inst::FpuLoad32 { .. } => (0b1011110001, 32),
&Inst::FpuLoad64 { .. } => (0b1111110001, 64),
&Inst::FpuLoad128 { .. } => (0b0011110011, 128),
_ => unreachable!(),
};

Expand All @@ -678,6 +686,9 @@ impl MachInstEmit for Inst {
sink.put4(enc_ldst_simm9(op, simm9, 0b00, reg, rd));
}
&MemArg::UnsignedOffset(reg, uimm12scaled) => {
if uimm12scaled.value() != 0 {
assert_eq!(bits, ty_bits(uimm12scaled.scale_ty()));
}
sink.put4(enc_ldst_uimm12(op, uimm12scaled, reg, rd));
}
&MemArg::RegReg(r1, r2) => {
Expand All @@ -686,19 +697,7 @@ impl MachInstEmit for Inst {
));
}
&MemArg::RegScaled(r1, r2, ty) | &MemArg::RegScaledExtended(r1, r2, ty, _) => {
match (ty, self) {
(I8, &Inst::ULoad8 { .. }) => {}
(I8, &Inst::SLoad8 { .. }) => {}
(I16, &Inst::ULoad16 { .. }) => {}
(I16, &Inst::SLoad16 { .. }) => {}
(I32, &Inst::ULoad32 { .. }) => {}
(I32, &Inst::SLoad32 { .. }) => {}
(I64, &Inst::ULoad64 { .. }) => {}
(F32, &Inst::FpuLoad32 { .. }) => {}
(F64, &Inst::FpuLoad64 { .. }) => {}
(I128, &Inst::FpuLoad128 { .. }) => {}
_ => panic!("Mismatching reg-scaling type in MemArg"),
}
assert_eq!(bits, ty_bits(ty));
let extendop = match &mem {
&MemArg::RegScaled(..) => None,
&MemArg::RegScaledExtended(_, _, _, op) => Some(op),
Expand Down Expand Up @@ -746,6 +745,7 @@ impl MachInstEmit for Inst {
&MemArg::SPOffset(..)
| &MemArg::FPOffset(..)
| &MemArg::NominalSPOffset(..) => panic!("Should not see stack-offset here!"),
&MemArg::RegOffset(..) => panic!("SHould not see generic reg-offset here!"),
}
}

Expand Down Expand Up @@ -791,14 +791,14 @@ impl MachInstEmit for Inst {
inst.emit(sink, flags, state);
}

let op = match self {
&Inst::Store8 { .. } => 0b0011100000,
&Inst::Store16 { .. } => 0b0111100000,
&Inst::Store32 { .. } => 0b1011100000,
&Inst::Store64 { .. } => 0b1111100000,
&Inst::FpuStore32 { .. } => 0b1011110000,
&Inst::FpuStore64 { .. } => 0b1111110000,
&Inst::FpuStore128 { .. } => 0b0011110010,
let (op, bits) = match self {
&Inst::Store8 { .. } => (0b0011100000, 8),
&Inst::Store16 { .. } => (0b0111100000, 16),
&Inst::Store32 { .. } => (0b1011100000, 32),
&Inst::Store64 { .. } => (0b1111100000, 64),
&Inst::FpuStore32 { .. } => (0b1011110000, 32),
&Inst::FpuStore64 { .. } => (0b1111110000, 64),
&Inst::FpuStore128 { .. } => (0b0011110010, 128),
_ => unreachable!(),
};

Expand All @@ -812,6 +812,9 @@ impl MachInstEmit for Inst {
sink.put4(enc_ldst_simm9(op, simm9, 0b00, reg, rd));
}
&MemArg::UnsignedOffset(reg, uimm12scaled) => {
if uimm12scaled.value() != 0 {
assert_eq!(bits, ty_bits(uimm12scaled.scale_ty()));
}
sink.put4(enc_ldst_uimm12(op, uimm12scaled, reg, rd));
}
&MemArg::RegReg(r1, r2) => {
Expand Down Expand Up @@ -843,6 +846,7 @@ impl MachInstEmit for Inst {
&MemArg::SPOffset(..)
| &MemArg::FPOffset(..)
| &MemArg::NominalSPOffset(..) => panic!("Should not see stack-offset here!"),
&MemArg::RegOffset(..) => panic!("SHould not see generic reg-offset here!"),
}
}

Expand Down
38 changes: 34 additions & 4 deletions cranelift/codegen/src/isa/aarch64/inst/emit_tests.rs
View file
Expand Up @@ -1311,7 +1311,7 @@ fn test_aarch64_binemit() {
insns.push((
Inst::ULoad64 {
rd: writable_xreg(1),
mem: MemArg::FPOffset(32768),
mem: MemArg::FPOffset(32768, I8),
srcloc: None,
},
"100090D2B063308B010240F9",
Expand All @@ -1320,7 +1320,7 @@ fn test_aarch64_binemit() {
insns.push((
Inst::ULoad64 {
rd: writable_xreg(1),
mem: MemArg::FPOffset(-32768),
mem: MemArg::FPOffset(-32768, I8),
srcloc: None,
},
"F0FF8F92B063308B010240F9",
Expand All @@ -1329,7 +1329,7 @@ fn test_aarch64_binemit() {
insns.push((
Inst::ULoad64 {
rd: writable_xreg(1),
mem: MemArg::FPOffset(1048576), // 2^20
mem: MemArg::FPOffset(1048576, I8), // 2^20
srcloc: None,
},
"1002A0D2B063308B010240F9",
Expand All @@ -1338,13 +1338,43 @@ fn test_aarch64_binemit() {
insns.push((
Inst::ULoad64 {
rd: writable_xreg(1),
mem: MemArg::FPOffset(1048576 + 1), // 2^20 + 1
mem: MemArg::FPOffset(1048576 + 1, I8), // 2^20 + 1
srcloc: None,
},
"300080D21002A0F2B063308B010240F9",
"movz x16, #1 ; movk x16, #16, LSL #16 ; add x16, fp, x16, UXTX ; ldr x1, [x16]",
));

insns.push((
Inst::ULoad64 {
rd: writable_xreg(1),
mem: MemArg::RegOffset(xreg(7), 8, I64),
srcloc: None,
},
"E18040F8",
"ldur x1, [x7, #8]",
));

insns.push((
Inst::ULoad64 {
rd: writable_xreg(1),
mem: MemArg::RegOffset(xreg(7), 1024, I64),
srcloc: None,
},
"E10042F9",
"ldr x1, [x7, #1024]",
));

insns.push((
Inst::ULoad64 {
rd: writable_xreg(1),
mem: MemArg::RegOffset(xreg(7), 1048576, I64),
srcloc: None,
},
"1002A0D2F060308B010240F9",
"movz x16, #16, LSL #16 ; add x16, x7, x16, UXTX ; ldr x1, [x16]",
));

insns.push((
Inst::Store8 {
rd: xreg(1),
Expand Down
7 changes: 6 additions & 1 deletion cranelift/codegen/src/isa/aarch64/inst/imms.rs
View file
Expand Up @@ -259,7 +259,12 @@ impl UImm12Scaled {

/// Value after scaling.
pub fn value(&self) -> u32 {
self.value as u32 * self.scale_ty.bytes()
self.value as u32
}

/// The value type which is the scaling base.
pub fn scale_ty(&self) -> Type {
self.scale_ty
}
}

Expand Down
4 changes: 4 additions & 0 deletions cranelift/codegen/src/isa/aarch64/inst/mod.rs
View file
Expand Up @@ -1004,6 +1004,9 @@ fn memarg_regs(memarg: &MemArg, collector: &mut RegUsageCollector) {
&MemArg::SPOffset(..) | &MemArg::NominalSPOffset(..) => {
collector.add_use(stack_reg());
}
&MemArg::RegOffset(r, ..) => {
collector.add_use(r);
}
}
}

Expand Down Expand Up @@ -1318,6 +1321,7 @@ fn aarch64_map_regs<RUM: RegUsageMapper>(inst: &mut Inst, mapper: &RUM) {
&mut MemArg::FPOffset(..)
| &mut MemArg::SPOffset(..)
| &mut MemArg::NominalSPOffset(..) => {}
&mut MemArg::RegOffset(ref mut r, ..) => map_use(m, r),
};
}

Expand Down
6 changes: 2 additions & 4 deletions cranelift/codegen/src/isa/aarch64/lower.rs
View file
Expand Up @@ -539,12 +539,10 @@ pub(crate) fn lower_address<C: LowerCtx<I = Inst>>(
// TODO: support base_reg + scale * index_reg. For this, we would need to pattern-match shl or
// mul instructions (Load/StoreComplex don't include scale factors).

// Handle one reg and offset that fits in immediate, if possible.
// Handle one reg and offset.
if addends.len() == 1 {
let reg = input_to_reg(ctx, addends[0], NarrowValueMode::ZeroExtend64);
if let Some(memarg) = MemArg::reg_maybe_offset(reg, offset as i64, elem_ty) {
return memarg;
}
return MemArg::RegOffset(reg, offset as i64, elem_ty);
}

// Handle two regs and a zero offset, if possible.
Expand Down
4 changes: 2 additions & 2 deletions cranelift/codegen/src/isa/aarch64/lower_inst.rs
View file
Expand Up @@ -1335,7 +1335,7 @@ pub(crate) fn lower_insn_to_regs<C: LowerCtx<I = Inst>>(
assert!(inputs.len() == sig.params.len());
assert!(outputs.len() == sig.returns.len());
(
AArch64ABICall::from_func(sig, &extname, dist, loc),
AArch64ABICall::from_func(sig, &extname, dist, loc)?,
&inputs[..],
)
}
Expand All @@ -1344,7 +1344,7 @@ pub(crate) fn lower_insn_to_regs<C: LowerCtx<I = Inst>>(
let sig = ctx.call_sig(insn).unwrap();
assert!(inputs.len() - 1 == sig.params.len());
assert!(outputs.len() == sig.returns.len());
(AArch64ABICall::from_ptr(sig, ptr, loc, op), &inputs[1..])
(AArch64ABICall::from_ptr(sig, ptr, loc, op)?, &inputs[1..])
}
_ => unreachable!(),
};
Expand Down
2 changes: 1 addition & 1 deletion cranelift/codegen/src/isa/aarch64/mod.rs
View file
Expand Up @@ -46,7 +46,7 @@ impl AArch64Backend {
func: &Function,
flags: settings::Flags,
) -> CodegenResult<VCode<inst::Inst>> {
let abi = Box::new(abi::AArch64ABIBody::new(func, flags));
let abi = Box::new(abi::AArch64ABIBody::new(func, flags)?);
compile::compile::<AArch64Backend>(func, self, abi)
}
}
Expand Down
10 changes: 5 additions & 5 deletions cranelift/codegen/src/isa/x64/abi.rs
View file
Expand Up @@ -184,11 +184,11 @@ impl X64ABIBody {
impl ABIBody for X64ABIBody {
type I = Inst;

fn needed_tmps(&self) -> usize {
0
fn temp_needed(&self) -> bool {
false
}

fn init_with_tmps(&mut self, _: &[Writable<Reg>]) {}
fn init(&mut self, _: Option<Writable<Reg>>) {}

fn flags(&self) -> &settings::Flags {
&self.flags
Expand Down Expand Up @@ -239,8 +239,8 @@ impl ABIBody for X64ABIBody {
}
}

fn gen_retval_area_setup(&self) -> Vec<Inst> {
vec![]
fn gen_retval_area_setup(&self) -> Option<Inst> {
None
}

fn gen_copy_reg_to_retval(
Expand Down

0 comments on commit fe97659

Please sign in to comment.