From d9310e8d9092b39a9f1c7451c837f6283985ee5c Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Mon, 6 Jul 2020 18:45:07 +0200 Subject: [PATCH] machinst x64: fix checked div sequence - it should mark as clobbering (def) rdx, not modifying it - the signed-div check requires a temporary to compare against int64_min --- cranelift/codegen/src/isa/x64/inst/args.rs | 2 +- cranelift/codegen/src/isa/x64/inst/emit.rs | 15 +++++++++++++-- cranelift/codegen/src/isa/x64/inst/mod.rs | 21 +++++++++++++++++---- cranelift/codegen/src/isa/x64/lower.rs | 7 +++++++ 4 files changed, 38 insertions(+), 7 deletions(-) diff --git a/cranelift/codegen/src/isa/x64/inst/args.rs b/cranelift/codegen/src/isa/x64/inst/args.rs index 9c6adef11d..cf387e07c8 100644 --- a/cranelift/codegen/src/isa/x64/inst/args.rs +++ b/cranelift/codegen/src/isa/x64/inst/args.rs @@ -199,7 +199,7 @@ impl RegMemImm { match self { Self::Reg { reg } => collector.add_use(*reg), Self::Mem { addr } => addr.get_regs_as_uses(collector), - Self::Imm { simm32: _ } => {} + Self::Imm { .. } => {} } } } diff --git a/cranelift/codegen/src/isa/x64/inst/emit.rs b/cranelift/codegen/src/isa/x64/inst/emit.rs index 4497d26c99..04fcf9c34a 100644 --- a/cranelift/codegen/src/isa/x64/inst/emit.rs +++ b/cranelift/codegen/src/isa/x64/inst/emit.rs @@ -666,6 +666,7 @@ pub(crate) fn emit( size, divisor, loc, + tmp, } => { // Generates the following code sequence: // @@ -727,8 +728,18 @@ pub(crate) fn emit( (Some(do_op), Some(done_label)) } else { // Check for integer overflow. - let inst = Inst::cmp_rmi_r(*size, RegMemImm::imm(0x80000000), regs::rax()); - inst.emit(sink, flags, state); + if *size == 8 { + let tmp = tmp.expect("temporary for i64 sdiv"); + + let inst = Inst::imm_r(true, 0x8000000000000000, tmp); + inst.emit(sink, flags, state); + + let inst = Inst::cmp_rmi_r(8, RegMemImm::reg(tmp.to_reg()), regs::rax()); + inst.emit(sink, flags, state); + } else { + let inst = Inst::cmp_rmi_r(*size, RegMemImm::imm(0x80000000), regs::rax()); + inst.emit(sink, flags, state); + } // If not equal, jump over the trap. let inst = Inst::trap_if(CC::Z, TrapCode::IntegerOverflow, *loc); diff --git a/cranelift/codegen/src/isa/x64/inst/mod.rs b/cranelift/codegen/src/isa/x64/inst/mod.rs index fc01c3e832..d3ac5ce66c 100644 --- a/cranelift/codegen/src/isa/x64/inst/mod.rs +++ b/cranelift/codegen/src/isa/x64/inst/mod.rs @@ -69,16 +69,20 @@ pub enum Inst { MulHi { size: u8, signed: bool, rhs: RegMem }, /// A synthetic sequence to implement the right inline checks for remainder and division, - /// assuming the dividend is in $rax. - /// Puts the result back into $rax if is_div, $rdx if !is_div, to mimic what the div + /// assuming the dividend is in %rax. + /// Puts the result back into %rax if is_div, %rdx if !is_div, to mimic what the div /// instruction does. /// The generated code sequence is described in the emit's function match arm for this /// instruction. + /// + /// Note: %rdx is marked as modified by this instruction, to avoid an early clobber problem + /// with the temporary and divisor. Make sure to zero %rdx right before this instruction! CheckedDivOrRemSeq { is_div: bool, is_signed: bool, size: u8, divisor: Reg, + tmp: Option>, loc: SourceLoc, }, @@ -846,10 +850,16 @@ fn x64_get_regs(inst: &Inst, collector: &mut RegUsageCollector) { collector.add_def(Writable::from_reg(regs::rdx())); rhs.get_regs_as_uses(collector); } - Inst::CheckedDivOrRemSeq { divisor, .. } => { + Inst::CheckedDivOrRemSeq { divisor, tmp, .. } => { + // Mark both fixed registers as mods, to avoid an early clobber problem in codegen + // (i.e. the temporary is allocated one of the fixed registers). This requires writing + // the rdx register *before* the instruction, which is not too bad. collector.add_mod(Writable::from_reg(regs::rax())); collector.add_mod(Writable::from_reg(regs::rdx())); collector.add_use(*divisor); + if let Some(tmp) = tmp { + collector.add_def(*tmp); + } } Inst::SignExtendRaxRdx { .. } => { collector.add_use(regs::rax()); @@ -1038,8 +1048,11 @@ fn x64_map_regs(inst: &mut Inst, mapper: &RUM) { } Inst::Div { divisor, .. } => divisor.map_uses(mapper), Inst::MulHi { rhs, .. } => rhs.map_uses(mapper), - Inst::CheckedDivOrRemSeq { divisor, .. } => { + Inst::CheckedDivOrRemSeq { divisor, tmp, .. } => { map_use(mapper, divisor); + if let Some(tmp) = tmp { + map_def(mapper, tmp) + } } Inst::SignExtendRaxRdx { .. } => {} Inst::XMM_Mov_RM_R { diff --git a/cranelift/codegen/src/isa/x64/lower.rs b/cranelift/codegen/src/isa/x64/lower.rs index cde9054619..972042eff8 100644 --- a/cranelift/codegen/src/isa/x64/lower.rs +++ b/cranelift/codegen/src/isa/x64/lower.rs @@ -825,11 +825,18 @@ fn lower_insn_to_regs>( // regalloc is aware of the coalescing opportunity between rax/rdx and the // destination register. let divisor = input_to_reg(ctx, inputs[1]); + let tmp = if op == Opcode::Sdiv && size == 8 { + Some(ctx.alloc_tmp(RegClass::I64, I64)) + } else { + None + }; + ctx.emit(Inst::imm_r(true, 0, Writable::from_reg(regs::rdx()))); ctx.emit(Inst::CheckedDivOrRemSeq { is_div, is_signed, size, divisor, + tmp, loc: srcloc, }); } else {