From a470f1e0cd57578f2f8f5bd3979ea91583fd26d2 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 7 Oct 2020 13:31:29 +0200 Subject: [PATCH] machinst x64: remove dead code and allow(dead_code) annotation; The BranchTarget is always used as a label, so just use a plain MachLabel in this case. --- cranelift/codegen/src/isa/x64/abi.rs | 4 +- cranelift/codegen/src/isa/x64/inst/args.rs | 65 +---------- cranelift/codegen/src/isa/x64/inst/emit.rs | 109 ++++++++---------- .../codegen/src/isa/x64/inst/emit_tests.rs | 18 ++- cranelift/codegen/src/isa/x64/inst/mod.rs | 37 +++--- cranelift/codegen/src/isa/x64/lower.rs | 16 +-- cranelift/codegen/src/machinst/buffer.rs | 6 + 7 files changed, 90 insertions(+), 165 deletions(-) diff --git a/cranelift/codegen/src/isa/x64/abi.rs b/cranelift/codegen/src/isa/x64/abi.rs index a253a92bae..bbbabc5ff4 100644 --- a/cranelift/codegen/src/isa/x64/abi.rs +++ b/cranelift/codegen/src/isa/x64/abi.rs @@ -276,11 +276,11 @@ impl ABIMachineSpec for X64ABIMachineSpec { } fn gen_ret() -> Self::I { - Inst::Ret + Inst::ret() } fn gen_epilogue_placeholder() -> Self::I { - Inst::EpiloguePlaceholder + Inst::epilogue_placeholder() } fn gen_add_imm(into_reg: Writable, from_reg: Reg, imm: u32) -> SmallVec<[Self::I; 4]> { diff --git a/cranelift/codegen/src/isa/x64/inst/args.rs b/cranelift/codegen/src/isa/x64/inst/args.rs index 3eeec52bf0..63d8064b0f 100644 --- a/cranelift/codegen/src/isa/x64/inst/args.rs +++ b/cranelift/codegen/src/isa/x64/inst/args.rs @@ -4,13 +4,12 @@ use super::regs::{self, show_ireg_sized}; use super::EmitState; use crate::ir::condcodes::{FloatCC, IntCC}; use crate::machinst::*; -use core::fmt::Debug; use regalloc::{ PrettyPrint, PrettyPrintSized, RealRegUniverse, Reg, RegClass, RegUsageCollector, RegUsageMapper, Writable, }; use std::fmt; -use std::string::{String, ToString}; +use std::string::String; /// A possible addressing mode (amode) that can be used in instructions. /// These denote a 64-bit value only. @@ -29,7 +28,7 @@ pub enum Amode { /// sign-extend-32-to-64(Immediate) + RIP (instruction pointer). /// To wit: not supported in 32-bits mode. - RipRelative { target: BranchTarget }, + RipRelative { target: MachLabel }, } impl Amode { @@ -50,7 +49,7 @@ impl Amode { } } - pub(crate) fn rip_relative(target: BranchTarget) -> Self { + pub(crate) fn rip_relative(target: MachLabel) -> Self { Self::RipRelative { target } } @@ -89,13 +88,7 @@ impl PrettyPrint for Amode { index.show_rru(mb_rru), 1 << shift ), - Amode::RipRelative { ref target } => format!( - "{}(%rip)", - match target { - BranchTarget::Label(label) => format!("label{}", label.get()), - BranchTarget::ResolvedOffset(offset) => offset.to_string(), - } - ), + Amode::RipRelative { ref target } => format!("label{}(%rip)", target.get()), } } } @@ -1094,55 +1087,6 @@ impl From for FcmpImm { } } -/// A branch target. Either unresolved (basic-block index) or resolved (offset -/// from end of current instruction). -#[derive(Clone, Copy, Debug)] -pub enum BranchTarget { - /// An unresolved reference to a MachLabel. - Label(MachLabel), - - /// A resolved reference to another instruction, in bytes. - ResolvedOffset(isize), -} - -impl PrettyPrint for BranchTarget { - fn show_rru(&self, _mb_rru: Option<&RealRegUniverse>) -> String { - match self { - BranchTarget::Label(l) => format!("{:?}", l), - BranchTarget::ResolvedOffset(offs) => format!("(offset {})", offs), - } - } -} - -impl BranchTarget { - /// Get the label. - pub fn as_label(&self) -> Option { - match self { - &BranchTarget::Label(l) => Some(l), - _ => None, - } - } - - /// Get the offset as a signed 32 bit byte offset. This returns the - /// offset in bytes between the first byte of the source and the first - /// byte of the target. It does not take into account the Intel-specific - /// rule that a branch offset is encoded as relative to the start of the - /// following instruction. That is a problem for the emitter to deal - /// with. If a label, returns zero. - pub fn as_offset32_or_zero(&self) -> i32 { - match self { - &BranchTarget::ResolvedOffset(off) => { - // Leave a bit of slack so that the emitter is guaranteed to - // be able to add the length of the jump instruction encoding - // to this value and still have a value in signed-32 range. - assert!(off >= -0x7FFF_FF00 && off <= 0x7FFF_FF00); - off as i32 - } - _ => 0, - } - } -} - /// An operand's size in bits. #[derive(Clone, Copy, PartialEq)] pub enum OperandSize { @@ -1176,6 +1120,7 @@ impl OperandSize { /// An x64 memory fence kind. #[derive(Clone)] +#[allow(dead_code)] pub enum FenceKind { /// `mfence` instruction ("Memory Fence") MFence, diff --git a/cranelift/codegen/src/isa/x64/inst/emit.rs b/cranelift/codegen/src/isa/x64/inst/emit.rs index 4061df89b9..e94c100839 100644 --- a/cranelift/codegen/src/isa/x64/inst/emit.rs +++ b/cranelift/codegen/src/isa/x64/inst/emit.rs @@ -7,7 +7,6 @@ use crate::machinst::{inst_common, MachBuffer, MachInstEmit, MachLabel}; use core::convert::TryInto; use log::debug; use regalloc::{Reg, RegClass, Writable}; -use std::convert::TryFrom; fn low8_will_sign_extend_to_64(x: u32) -> bool { let xs = (x as i32) as i64; @@ -296,18 +295,9 @@ fn emit_std_enc_mem( // RIP-relative is mod=00, rm=101. sink.put1(encode_modrm(0, enc_g & 7, 0b101)); - match *target { - BranchTarget::Label(label) => { - let offset = sink.cur_offset(); - sink.use_label_at_offset(offset, label, LabelUse::JmpRel32); - sink.put4(0); - } - BranchTarget::ResolvedOffset(offset) => { - let offset = - u32::try_from(offset).expect("rip-relative can't hold >= U32_MAX values"); - sink.put4(offset); - } - } + let offset = sink.cur_offset(); + sink.use_label_at_offset(offset, *target, LabelUse::JmpRel32); + sink.put4(0); } } } @@ -808,7 +798,7 @@ pub(crate) fn emit( ); inst.emit(sink, info, state); - let inst = Inst::jmp_known(BranchTarget::Label(done_label)); + let inst = Inst::jmp_known(done_label); inst.emit(sink, info, state); (Some(do_op), Some(done_label)) @@ -1503,30 +1493,26 @@ pub(crate) fn emit( let br_start = sink.cur_offset(); let br_disp_off = br_start + 1; let br_end = br_start + 5; - if let Some(l) = dst.as_label() { - sink.use_label_at_offset(br_disp_off, l, LabelUse::JmpRel32); - sink.add_uncond_branch(br_start, br_end, l); - } - let disp = dst.as_offset32_or_zero(); - let disp = disp as u32; + sink.use_label_at_offset(br_disp_off, *dst, LabelUse::JmpRel32); + sink.add_uncond_branch(br_start, br_end, *dst); + sink.put1(0xE9); - sink.put4(disp); + // Placeholder for the label value. + sink.put4(0x0); } Inst::JmpIf { cc, taken } => { let cond_start = sink.cur_offset(); let cond_disp_off = cond_start + 2; - if let Some(l) = taken.as_label() { - sink.use_label_at_offset(cond_disp_off, l, LabelUse::JmpRel32); - // Since this is not a terminator, don't enroll in the branch inversion mechanism. - } - let taken_disp = taken.as_offset32_or_zero(); - let taken_disp = taken_disp as u32; + sink.use_label_at_offset(cond_disp_off, *taken, LabelUse::JmpRel32); + // Since this is not a terminator, don't enroll in the branch inversion mechanism. + sink.put1(0x0F); sink.put1(0x80 + cc.get_enc()); - sink.put4(taken_disp); + // Placeholder for the label value. + sink.put4(0x0); } Inst::JmpCond { @@ -1538,32 +1524,27 @@ pub(crate) fn emit( let cond_start = sink.cur_offset(); let cond_disp_off = cond_start + 2; let cond_end = cond_start + 6; - if let Some(l) = taken.as_label() { - sink.use_label_at_offset(cond_disp_off, l, LabelUse::JmpRel32); - let inverted: [u8; 6] = - [0x0F, 0x80 + (cc.invert().get_enc()), 0x00, 0x00, 0x00, 0x00]; - sink.add_cond_branch(cond_start, cond_end, l, &inverted[..]); - } - let taken_disp = taken.as_offset32_or_zero(); - let taken_disp = taken_disp as u32; + sink.use_label_at_offset(cond_disp_off, *taken, LabelUse::JmpRel32); + let inverted: [u8; 6] = [0x0F, 0x80 + (cc.invert().get_enc()), 0x00, 0x00, 0x00, 0x00]; + sink.add_cond_branch(cond_start, cond_end, *taken, &inverted[..]); + sink.put1(0x0F); sink.put1(0x80 + cc.get_enc()); - sink.put4(taken_disp); + // Placeholder for the label value. + sink.put4(0x0); // If not taken. let uncond_start = sink.cur_offset(); let uncond_disp_off = uncond_start + 1; let uncond_end = uncond_start + 5; - if let Some(l) = not_taken.as_label() { - sink.use_label_at_offset(uncond_disp_off, l, LabelUse::JmpRel32); - sink.add_uncond_branch(uncond_start, uncond_end, l); - } - let nt_disp = not_taken.as_offset32_or_zero(); - let nt_disp = nt_disp as u32; + sink.use_label_at_offset(uncond_disp_off, *not_taken, LabelUse::JmpRel32); + sink.add_uncond_branch(uncond_start, uncond_end, *not_taken); + sink.put1(0xE9); - sink.put4(nt_disp); + // Placeholder for the label value. + sink.put4(0x0); } Inst::JmpUnknown { target } => { @@ -1625,11 +1606,7 @@ pub(crate) fn emit( // j *%tmp1 // $start_of_jump_table: // -- jump table entries - let default_label = match default_target { - BranchTarget::Label(label) => label, - _ => unreachable!(), - }; - one_way_jmp(sink, CC::NB, *default_label); // idx unsigned >= jmp table size + one_way_jmp(sink, CC::NB, *default_target); // idx unsigned >= jmp table size // Copy the index (and make sure to clear the high 32-bits lane of tmp2). let inst = Inst::movzx_rm_r(ExtMode::LQ, RegMem::reg(*idx), *tmp2, None); @@ -1637,10 +1614,7 @@ pub(crate) fn emit( // Load base address of jump table. let start_of_jumptable = sink.get_label(); - let inst = Inst::lea( - Amode::rip_relative(BranchTarget::Label(start_of_jumptable)), - *tmp1, - ); + let inst = Inst::lea(Amode::rip_relative(start_of_jumptable), *tmp1); inst.emit(sink, info, state); // Load value out of the jump table. It's a relative offset to the target block, so it @@ -1676,7 +1650,7 @@ pub(crate) fn emit( // with the extra addend, it'll be relative to the jump table's start, after // patching. let off_into_table = word_off - jt_off; - sink.use_label_at_offset(word_off, target.as_label().unwrap(), LabelUse::PCRel32); + sink.use_label_at_offset(word_off, target, LabelUse::PCRel32); sink.put4(off_into_table); } } @@ -1912,7 +1886,7 @@ pub(crate) fn emit( let inst = Inst::xmm_rm_r(op, RegMem::reg(*lhs), *rhs_dst); inst.emit(sink, info, state); - let inst = Inst::jmp_known(BranchTarget::Label(done)); + let inst = Inst::jmp_known(done); inst.emit(sink, info, state); // x86's min/max are not symmetric; if either operand is a NaN, they return the @@ -1994,14 +1968,13 @@ pub(crate) fn emit( // Load the inline constant. let constant_start_label = sink.get_label(); - let load_offset = Amode::rip_relative(BranchTarget::Label(constant_start_label)); + let load_offset = Amode::rip_relative(constant_start_label); let load = Inst::load(*ty, load_offset, *dst, ExtKind::None, None); load.emit(sink, info, state); // Jump over the constant. let constant_end_label = sink.get_label(); - let continue_at_offset = BranchTarget::Label(constant_end_label); - let jump = Inst::jmp_known(continue_at_offset); + let jump = Inst::jmp_known(constant_end_label); jump.emit(sink, info, state); // Emit the constant. @@ -2169,7 +2142,7 @@ pub(crate) fn emit( // right thing. emit_signed_cvt(sink, info, state, src.to_reg(), *dst, *to_f64); - let inst = Inst::jmp_known(BranchTarget::Label(done)); + let inst = Inst::jmp_known(done); inst.emit(sink, info, state); sink.bind_label(handle_negative); @@ -2308,7 +2281,7 @@ pub(crate) fn emit( ); inst.emit(sink, info, state); - let inst = Inst::jmp_known(BranchTarget::Label(done)); + let inst = Inst::jmp_known(done); inst.emit(sink, info, state); sink.bind_label(not_nan); @@ -2499,7 +2472,7 @@ pub(crate) fn emit( ); inst.emit(sink, info, state); - let inst = Inst::jmp_known(BranchTarget::Label(done)); + let inst = Inst::jmp_known(done); inst.emit(sink, info, state); } else { // Trap. @@ -2531,7 +2504,7 @@ pub(crate) fn emit( ); inst.emit(sink, info, state); - let inst = Inst::jmp_known(BranchTarget::Label(done)); + let inst = Inst::jmp_known(done); inst.emit(sink, info, state); } else { // Trap. @@ -2569,7 +2542,7 @@ pub(crate) fn emit( ); inst.emit(sink, info, state); - let inst = Inst::jmp_known(BranchTarget::Label(done)); + let inst = Inst::jmp_known(done); inst.emit(sink, info, state); } else { let inst = Inst::trap(*srcloc, TrapCode::IntegerOverflow); @@ -2746,7 +2719,15 @@ pub(crate) fn emit( state.virtual_sp_offset += offset; } - Inst::Nop { .. } | Inst::EpiloguePlaceholder => { + Inst::Nop { len } => { + if *len == 0 { + // Nothing to emit. + } else { + unimplemented!("non-zero nop opcodes."); + } + } + + Inst::EpiloguePlaceholder => { // Generate no code. } } diff --git a/cranelift/codegen/src/isa/x64/inst/emit_tests.rs b/cranelift/codegen/src/isa/x64/inst/emit_tests.rs index 4bcf1d52f0..ce144f6263 100644 --- a/cranelift/codegen/src/isa/x64/inst/emit_tests.rs +++ b/cranelift/codegen/src/isa/x64/inst/emit_tests.rs @@ -1766,17 +1766,9 @@ fn test_x64_emit() { "lea 179(%r10,%r9,1), %r8", )); insns.push(( - Inst::lea(Amode::rip_relative(BranchTarget::ResolvedOffset(0)), w_rdi), + Inst::lea(Amode::rip_relative(MachLabel::from_block(0)), w_rdi), "488D3D00000000", - "lea 0(%rip), %rdi", - )); - insns.push(( - Inst::lea( - Amode::rip_relative(BranchTarget::ResolvedOffset(1337)), - w_r15, - ), - "4C8D3D39050000", - "lea 1337(%rip), %r15", + "lea label0(%rip), %rdi", )); // ======================================================== @@ -3676,7 +3668,13 @@ fn test_x64_emit() { assert_eq!(expected_printing, actual_printing); let mut sink = test_utils::TestCodeSink::new(); let mut buffer = MachBuffer::new(); + insn.emit(&mut buffer, &emit_info, &mut Default::default()); + + // Allow one label just after the instruction (so the offset is 0). + let label = buffer.get_label(); + buffer.bind_label(label); + let buffer = buffer.finish(); buffer.emit(&mut sink); let actual_encoding = &sink.stringify(); diff --git a/cranelift/codegen/src/isa/x64/inst/mod.rs b/cranelift/codegen/src/isa/x64/inst/mod.rs index 02153ed11c..58c2cb66cc 100644 --- a/cranelift/codegen/src/isa/x64/inst/mod.rs +++ b/cranelift/codegen/src/isa/x64/inst/mod.rs @@ -1,5 +1,4 @@ //! This module defines x86_64-specific machine instruction types. -#![allow(dead_code)] use crate::binemit::{CodeOffset, StackMap}; use crate::ir::{types, ExternalName, Opcode, SourceLoc, TrapCode, Type}; @@ -369,7 +368,7 @@ pub enum Inst { EpiloguePlaceholder, /// Jump to a known target: jmp simm32. - JmpKnown { dst: BranchTarget }, + JmpKnown { dst: MachLabel }, /// One-way conditional branch: jcond cond target. /// @@ -379,14 +378,14 @@ pub enum Inst { /// A note of caution: in contexts where the branch target is another block, this has to be the /// same successor as the one specified in the terminator branch of the current block. /// Otherwise, this might confuse register allocation by creating new invisible edges. - JmpIf { cc: CC, taken: BranchTarget }, + JmpIf { cc: CC, taken: MachLabel }, /// Two-way conditional branch: jcond cond target target. /// Emitted as a compound sequence; the MachBuffer will shrink it as appropriate. JmpCond { cc: CC, - taken: BranchTarget, - not_taken: BranchTarget, + taken: MachLabel, + not_taken: MachLabel, }, /// Jump-table sequence, as one compound instruction (see note in lower.rs for rationale). @@ -397,8 +396,8 @@ pub enum Inst { idx: Reg, tmp1: Writable, tmp2: Writable, - default_target: BranchTarget, - targets: Vec, + default_target: MachLabel, + targets: Vec, targets_for_term: Vec, }, @@ -1077,15 +1076,15 @@ impl Inst { Inst::EpiloguePlaceholder } - pub(crate) fn jmp_known(dst: BranchTarget) -> Inst { + pub(crate) fn jmp_known(dst: MachLabel) -> Inst { Inst::JmpKnown { dst } } - pub(crate) fn jmp_if(cc: CC, taken: BranchTarget) -> Inst { + pub(crate) fn jmp_if(cc: CC, taken: MachLabel) -> Inst { Inst::JmpIf { cc, taken } } - pub(crate) fn jmp_cond(cc: CC, taken: BranchTarget, not_taken: BranchTarget) -> Inst { + pub(crate) fn jmp_cond(cc: CC, taken: MachLabel, not_taken: MachLabel) -> Inst { Inst::JmpCond { cc, taken, @@ -1679,13 +1678,13 @@ impl PrettyPrint for Inst { Inst::EpiloguePlaceholder => "epilogue placeholder".to_string(), Inst::JmpKnown { dst } => { - format!("{} {}", ljustify("jmp".to_string()), dst.show_rru(mb_rru)) + format!("{} {}", ljustify("jmp".to_string()), dst.to_string()) } Inst::JmpIf { cc, taken } => format!( "{} {}", ljustify2("j".to_string(), cc.to_string()), - taken.show_rru(mb_rru), + taken.to_string(), ), Inst::JmpCond { @@ -1695,8 +1694,8 @@ impl PrettyPrint for Inst { } => format!( "{} {}; j {}", ljustify2("j".to_string(), cc.to_string()), - taken.show_rru(mb_rru), - not_taken.show_rru(mb_rru) + taken.to_string(), + not_taken.to_string() ), Inst::JmpTableSeq { idx, .. } => { @@ -2446,10 +2445,10 @@ impl MachInst for Inst { match self { // Interesting cases. &Self::Ret | &Self::EpiloguePlaceholder => MachTerminator::Ret, - &Self::JmpKnown { dst } => MachTerminator::Uncond(dst.as_label().unwrap()), + &Self::JmpKnown { dst } => MachTerminator::Uncond(dst), &Self::JmpCond { taken, not_taken, .. - } => MachTerminator::Cond(taken.as_label().unwrap(), not_taken.as_label().unwrap()), + } => MachTerminator::Cond(taken, not_taken), &Self::JmpTableSeq { ref targets_for_term, .. @@ -2487,8 +2486,8 @@ impl MachInst for Inst { Inst::Nop { len: 0 } } - fn gen_nop(_preferred_size: usize) -> Inst { - unimplemented!() + fn gen_nop(preferred_size: usize) -> Inst { + Inst::nop(preferred_size as u8) } fn maybe_direct_reload(&self, _reg: VirtualReg, _slot: SpillSlot) -> Option { @@ -2519,7 +2518,7 @@ impl MachInst for Inst { } fn gen_jump(label: MachLabel) -> Inst { - Inst::jmp_known(BranchTarget::Label(label)) + Inst::jmp_known(label) } fn gen_constant Writable>( diff --git a/cranelift/codegen/src/isa/x64/lower.rs b/cranelift/codegen/src/isa/x64/lower.rs index 0fabdf0717..f399e022dc 100644 --- a/cranelift/codegen/src/isa/x64/lower.rs +++ b/cranelift/codegen/src/isa/x64/lower.rs @@ -3315,10 +3315,10 @@ impl LowerBackend for X64Backend { ); assert!(op1 == Opcode::Jump || op1 == Opcode::Fallthrough); - let taken = BranchTarget::Label(targets[0]); + let taken = targets[0]; let not_taken = match op1 { - Opcode::Jump => BranchTarget::Label(targets[1]), - Opcode::Fallthrough => BranchTarget::Label(fallthrough.unwrap()), + Opcode::Jump => targets[1], + Opcode::Fallthrough => fallthrough.unwrap(), _ => unreachable!(), // assert above. }; @@ -3422,7 +3422,7 @@ impl LowerBackend for X64Backend { let op = ctx.data(branches[0]).opcode(); match op { Opcode::Jump | Opcode::Fallthrough => { - ctx.emit(Inst::jmp_known(BranchTarget::Label(targets[0]))); + ctx.emit(Inst::jmp_known(targets[0])); } Opcode::BrTable => { @@ -3465,13 +3465,9 @@ impl LowerBackend for X64Backend { let tmp2 = ctx.alloc_tmp(RegClass::I64, types::I64); let targets_for_term: Vec = targets.to_vec(); - let default_target = BranchTarget::Label(targets[0]); + let default_target = targets[0]; - let jt_targets: Vec = targets - .iter() - .skip(1) - .map(|bix| BranchTarget::Label(*bix)) - .collect(); + let jt_targets: Vec = targets.iter().skip(1).cloned().collect(); ctx.emit(Inst::JmpTableSeq { idx, diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs index d0189bfa3a..ddf0515768 100644 --- a/cranelift/codegen/src/machinst/buffer.rs +++ b/cranelift/codegen/src/machinst/buffer.rs @@ -148,6 +148,7 @@ use crate::timing; use log::trace; use smallvec::SmallVec; use std::mem; +use std::string::String; /// A buffer of output to be produced, fixed up, and then emitted to a CodeSink /// in bulk. @@ -259,6 +260,11 @@ impl MachLabel { pub fn get(self) -> u32 { self.0 } + + /// Creates a string representing this label, for convenience. + pub fn to_string(&self) -> String { + format!("label{}", self.0) + } } /// A stack map extent, when creating a stack map.