From 0367fbc2d411dab09eb7759077c052fd7b5d7799 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Thu, 10 Nov 2022 16:19:25 -0800 Subject: [PATCH] cranelift: Rework pinned register lowering (#5249) Rework pinned register lowering to avoid the use of pinned virtual registers, instead using the MovFromPReg and MovToPReg pseudo instructions. --- cranelift/codegen/src/isa/aarch64/inst.isle | 36 ++++++++++-------- .../codegen/src/isa/aarch64/inst/emit.rs | 25 ++++++++++++- cranelift/codegen/src/isa/aarch64/inst/mod.rs | 30 ++++++++++++--- .../codegen/src/isa/aarch64/inst/regs.rs | 5 +++ cranelift/codegen/src/isa/aarch64/lower.isle | 2 +- cranelift/codegen/src/isa/aarch64/lower.rs | 2 +- .../codegen/src/isa/aarch64/lower/isle.rs | 8 ++-- cranelift/codegen/src/isa/x64/inst.isle | 37 +++++++++++-------- cranelift/codegen/src/isa/x64/inst/emit.rs | 14 ++++++- cranelift/codegen/src/isa/x64/inst/mod.rs | 21 +++++++++-- cranelift/codegen/src/isa/x64/lower/isle.rs | 10 ++--- cranelift/codegen/src/machinst/lower.rs | 15 +------- .../filetests/isa/aarch64/pinned-reg.clif | 4 +- .../filetests/isa/x64/pinned-reg.clif | 12 +++++- 14 files changed, 150 insertions(+), 71 deletions(-) diff --git a/cranelift/codegen/src/isa/aarch64/inst.isle b/cranelift/codegen/src/isa/aarch64/inst.isle index 5f439329e4..3debb844ac 100644 --- a/cranelift/codegen/src/isa/aarch64/inst.isle +++ b/cranelift/codegen/src/isa/aarch64/inst.isle @@ -167,10 +167,16 @@ ;; Like `Move` but with a particular `PReg` source (for implementing CLIF ;; instructions like `get_stack_pointer`). - (MovPReg + (MovFromPReg (rd WritableReg) (rm PReg)) + ;; Like `Move` but with a particular `PReg` destination (for + ;; implementing CLIF instructions like `set_pinned_reg`). + (MovToPReg + (rd PReg) + (rm Reg)) + ;; A MOV[Z,N] with a 16-bit immediate. (MovWide (op MoveWideOp) @@ -3061,12 +3067,16 @@ dst)) ;; Helper for emitting `MInst.MovPReg` instructions. -(decl mov_preg (PReg) Reg) -(rule (mov_preg src) +(decl mov_from_preg (PReg) Reg) +(rule (mov_from_preg src) (let ((dst WritableReg (temp_writable_reg $I64)) - (_ Unit (emit (MInst.MovPReg dst src)))) + (_ Unit (emit (MInst.MovFromPReg dst src)))) dst)) +(decl mov_to_preg (PReg Reg) SideEffectNoResult) +(rule (mov_to_preg dst src) + (SideEffectNoResult.Inst (MInst.MovToPReg dst src))) + (decl preg_sp () PReg) (extern constructor preg_sp preg_sp) @@ -3076,13 +3086,16 @@ (decl preg_link () PReg) (extern constructor preg_link preg_link) +(decl preg_pinned () PReg) +(extern constructor preg_pinned preg_pinned) + (decl aarch64_sp () Reg) (rule (aarch64_sp) - (mov_preg (preg_sp))) + (mov_from_preg (preg_sp))) (decl aarch64_fp () Reg) (rule (aarch64_fp) - (mov_preg (preg_fp))) + (mov_from_preg (preg_fp))) (decl aarch64_link () Reg) (rule 1 (aarch64_link) @@ -3111,7 +3124,7 @@ (lr WritableReg (writable_link_reg)) (_ Unit (emit (MInst.ULoad64 lr addr (mem_flags_trusted)))) (_ Unit (emit (MInst.Xpaclri)))) - (mov_preg (preg_link)))) + (mov_from_preg (preg_link)))) ;; Helper for getting the maximum shift amount for a type. @@ -3270,16 +3283,9 @@ ;; Helpers for pinned register manipulation. -(decl writable_pinned_reg () WritableReg) -(extern constructor writable_pinned_reg writable_pinned_reg) - -(decl pinned_reg () Reg) -(rule (pinned_reg) (writable_pinned_reg)) - (decl write_pinned_reg (Reg) SideEffectNoResult) (rule (write_pinned_reg val) - (let ((dst WritableReg (writable_pinned_reg))) - (SideEffectNoResult.Inst (gen_move $I64 dst val)))) + (mov_to_preg (preg_pinned) val)) ;; Helpers for stackslot effective address generation. diff --git a/cranelift/codegen/src/isa/aarch64/inst/emit.rs b/cranelift/codegen/src/isa/aarch64/inst/emit.rs index fa1dd5618c..0784cdeb01 100644 --- a/cranelift/codegen/src/isa/aarch64/inst/emit.rs +++ b/cranelift/codegen/src/isa/aarch64/inst/emit.rs @@ -1367,15 +1367,36 @@ impl MachInstEmit for Inst { } } } - &Inst::MovPReg { rd, rm } => { + &Inst::MovFromPReg { rd, rm } => { let rd = allocs.next_writable(rd); let rm: Reg = rm.into(); - debug_assert!([regs::fp_reg(), regs::stack_reg(), regs::link_reg()].contains(&rm)); + debug_assert!([ + regs::fp_reg(), + regs::stack_reg(), + regs::link_reg(), + regs::pinned_reg() + ] + .contains(&rm)); assert!(rm.class() == RegClass::Int); assert!(rd.to_reg().class() == rm.class()); let size = OperandSize::Size64; Inst::Mov { size, rd, rm }.emit(&[], sink, emit_info, state); } + &Inst::MovToPReg { rd, rm } => { + let rd: Writable = Writable::from_reg(rd.into()); + let rm = allocs.next(rm); + debug_assert!([ + regs::fp_reg(), + regs::stack_reg(), + regs::link_reg(), + regs::pinned_reg() + ] + .contains(&rd.to_reg())); + assert!(rd.to_reg().class() == RegClass::Int); + assert!(rm.class() == rd.to_reg().class()); + let size = OperandSize::Size64; + Inst::Mov { size, rd, rm }.emit(&[], sink, emit_info, state); + } &Inst::MovWide { op, rd, imm, size } => { let rd = allocs.next_writable(rd); sink.put4(enc_move_wide(op, rd, imm, size)); diff --git a/cranelift/codegen/src/isa/aarch64/inst/mod.rs b/cranelift/codegen/src/isa/aarch64/inst/mod.rs index dc52b84daf..29aeafc0ad 100644 --- a/cranelift/codegen/src/isa/aarch64/inst/mod.rs +++ b/cranelift/codegen/src/isa/aarch64/inst/mod.rs @@ -654,13 +654,28 @@ fn aarch64_get_operands VReg>(inst: &Inst, collector: &mut Operan collector.reg_def(rd); collector.reg_use(rm); } - &Inst::MovPReg { rd, rm } => { - debug_assert!( - [regs::fp_reg(), regs::stack_reg(), regs::link_reg()].contains(&rm.into()) - ); + &Inst::MovFromPReg { rd, rm } => { + debug_assert!([ + regs::fp_reg(), + regs::stack_reg(), + regs::link_reg(), + regs::pinned_reg() + ] + .contains(&rm.into())); debug_assert!(rd.to_reg().is_virtual()); collector.reg_def(rd); } + &Inst::MovToPReg { rd, rm } => { + debug_assert!([ + regs::fp_reg(), + regs::stack_reg(), + regs::link_reg(), + regs::pinned_reg() + ] + .contains(&rd.into())); + debug_assert!(rm.is_virtual()); + collector.reg_use(rm); + } &Inst::MovK { rd, rn, .. } => { collector.reg_use(rn); collector.reg_reuse_def(rd, 0); // `rn` == `rd`. @@ -1551,11 +1566,16 @@ impl Inst { let rm = pretty_print_ireg(rm, size, allocs); format!("mov {}, {}", rd, rm) } - &Inst::MovPReg { rd, rm } => { + &Inst::MovFromPReg { rd, rm } => { let rd = pretty_print_ireg(rd.to_reg(), OperandSize::Size64, allocs); let rm = show_ireg_sized(rm.into(), OperandSize::Size64); format!("mov {}, {}", rd, rm) } + &Inst::MovToPReg { rd, rm } => { + let rd = show_ireg_sized(rd.into(), OperandSize::Size64); + let rm = pretty_print_ireg(rm, OperandSize::Size64, allocs); + format!("mov {}, {}", rd, rm) + } &Inst::MovWide { op, rd, diff --git a/cranelift/codegen/src/isa/aarch64/inst/regs.rs b/cranelift/codegen/src/isa/aarch64/inst/regs.rs index eacd0b4330..7cfe46d74f 100644 --- a/cranelift/codegen/src/isa/aarch64/inst/regs.rs +++ b/cranelift/codegen/src/isa/aarch64/inst/regs.rs @@ -93,6 +93,11 @@ pub fn link_reg() -> Reg { xreg(30) } +/// Get a reference to the pinned register (x21). +pub fn pinned_reg() -> Reg { + xreg(PINNED_REG) +} + /// Get a writable reference to the link register. pub fn writable_link_reg() -> Writable { Writable::from_reg(link_reg()) diff --git a/cranelift/codegen/src/isa/aarch64/lower.isle b/cranelift/codegen/src/isa/aarch64/lower.isle index a9cc3846a9..ad14361376 100644 --- a/cranelift/codegen/src/isa/aarch64/lower.isle +++ b/cranelift/codegen/src/isa/aarch64/lower.isle @@ -2189,7 +2189,7 @@ ;;; Rules for `{get,set}_pinned_reg` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; (rule (lower (get_pinned_reg)) - (pinned_reg)) + (mov_from_preg (preg_pinned))) (rule (lower (set_pinned_reg val)) (side_effect (write_pinned_reg val))) diff --git a/cranelift/codegen/src/isa/aarch64/lower.rs b/cranelift/codegen/src/isa/aarch64/lower.rs index 66c0a21f39..7ba0249e77 100644 --- a/cranelift/codegen/src/isa/aarch64/lower.rs +++ b/cranelift/codegen/src/isa/aarch64/lower.rs @@ -794,6 +794,6 @@ impl LowerBackend for AArch64Backend { } fn maybe_pinned_reg(&self) -> Option { - Some(xreg(PINNED_REG)) + Some(regs::pinned_reg()) } } diff --git a/cranelift/codegen/src/isa/aarch64/lower/isle.rs b/cranelift/codegen/src/isa/aarch64/lower/isle.rs index 1ceb17007b..e47e3fc6b1 100644 --- a/cranelift/codegen/src/isa/aarch64/lower/isle.rs +++ b/cranelift/codegen/src/isa/aarch64/lower/isle.rs @@ -559,6 +559,10 @@ impl Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6> { super::regs::link_reg().to_real_reg().unwrap().into() } + fn preg_pinned(&mut self) -> PReg { + super::regs::pinned_reg().to_real_reg().unwrap().into() + } + fn branch_target(&mut self, elements: &VecMachLabel, idx: u8) -> BranchTarget { BranchTarget::Label(elements[idx as usize]) } @@ -718,8 +722,4 @@ impl Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6> { ); } } - - fn writable_pinned_reg(&mut self) -> WritableReg { - super::regs::writable_xreg(super::regs::PINNED_REG) - } } diff --git a/cranelift/codegen/src/isa/x64/inst.isle b/cranelift/codegen/src/isa/x64/inst.isle index 793be99de0..5a6bc40897 100644 --- a/cranelift/codegen/src/isa/x64/inst.isle +++ b/cranelift/codegen/src/isa/x64/inst.isle @@ -95,8 +95,13 @@ ;; Like `MovRR` but with a physical register source (for implementing ;; CLIF instructions like `get_stack_pointer`). - (MovPReg (src PReg) - (dst WritableGpr)) + (MovFromPReg (src PReg) + (dst WritableGpr)) + + ;; Like `MovRR` but with a physical register destination (for + ;; implementing CLIF instructions like `set_pinned_reg`). + (MovToPReg (src Gpr) + (dst PReg)) ;; Zero-extended loads, except for 64 bits: movz (bl bq wl wq lq) addr ;; reg. @@ -1200,10 +1205,6 @@ (decl temp_writable_xmm () WritableXmm) (extern constructor temp_writable_xmm temp_writable_xmm) -;; Fetch the special pinned register. -(decl pinned_writable_gpr () WritableGpr) -(extern constructor pinned_writable_gpr pinned_writable_gpr) - ;; Construct a new `XmmMem` from the given `RegMem`. ;; ;; Asserts that the `RegMem`'s register, if any, is an XMM register. @@ -3701,12 +3702,11 @@ (decl read_pinned_gpr () Gpr) (rule (read_pinned_gpr) - (pinned_writable_gpr)) + (mov_from_preg (preg_pinned))) (decl write_pinned_gpr (Gpr) SideEffectNoResult) (rule (write_pinned_gpr val) - (let ((dst WritableGpr (pinned_writable_gpr))) - (SideEffectNoResult.Inst (gen_move $I64 dst val)))) + (mov_to_preg (preg_pinned) val)) ;;;; Shuffle ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; @@ -3882,26 +3882,33 @@ (decl const_to_synthetic_amode (VCodeConstant) SyntheticAmode) (extern constructor const_to_synthetic_amode const_to_synthetic_amode) -;; Helper for creating `MovPReg` instructions. -(decl mov_preg (PReg) Reg) -(rule (mov_preg preg) +;; Helper for creating `MovFromPReg` instructions. +(decl mov_from_preg (PReg) Reg) +(rule (mov_from_preg preg) (let ((dst WritableGpr (temp_writable_gpr)) - (_ Unit (emit (MInst.MovPReg preg dst)))) + (_ Unit (emit (MInst.MovFromPReg preg dst)))) dst)) +(decl mov_to_preg (PReg Gpr) SideEffectNoResult) +(rule (mov_to_preg dst src) + (SideEffectNoResult.Inst (MInst.MovToPReg src dst))) + (decl preg_rbp () PReg) (extern constructor preg_rbp preg_rbp) (decl preg_rsp () PReg) (extern constructor preg_rsp preg_rsp) +(decl preg_pinned () PReg) +(extern constructor preg_pinned preg_pinned) + (decl x64_rbp () Reg) (rule (x64_rbp) - (mov_preg (preg_rbp))) + (mov_from_preg (preg_rbp))) (decl x64_rsp () Reg) (rule (x64_rsp) - (mov_preg (preg_rsp))) + (mov_from_preg (preg_rsp))) ;;;; Helpers for Emitting LibCalls ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/cranelift/codegen/src/isa/x64/inst/emit.rs b/cranelift/codegen/src/isa/x64/inst/emit.rs index 9291712c88..7d75a0ddec 100644 --- a/cranelift/codegen/src/isa/x64/inst/emit.rs +++ b/cranelift/codegen/src/isa/x64/inst/emit.rs @@ -698,9 +698,9 @@ pub(crate) fn emit( ); } - Inst::MovPReg { src, dst } => { + Inst::MovFromPReg { src, dst } => { let src: Reg = (*src).into(); - debug_assert!([regs::rsp(), regs::rbp()].contains(&src)); + debug_assert!([regs::rsp(), regs::rbp(), regs::pinned_reg()].contains(&src)); let src = Gpr::new(src).unwrap(); let size = OperandSize::Size64; let dst = allocs.next(dst.to_reg().to_reg()); @@ -708,6 +708,16 @@ pub(crate) fn emit( Inst::MovRR { size, src, dst }.emit(&[], sink, info, state); } + Inst::MovToPReg { src, dst } => { + let src = allocs.next(src.to_reg()); + let src = Gpr::new(src).unwrap(); + let dst: Reg = (*dst).into(); + debug_assert!([regs::rsp(), regs::rbp(), regs::pinned_reg()].contains(&dst)); + let dst = WritableGpr::from_writable_reg(Writable::from_reg(dst)).unwrap(); + let size = OperandSize::Size64; + Inst::MovRR { size, src, dst }.emit(&[], sink, info, state); + } + Inst::MovzxRmR { ext_mode, src, dst } => { let dst = allocs.next(dst.to_reg().to_reg()); let (opcodes, num_opcodes, mut rex_flags) = match ext_mode { diff --git a/cranelift/codegen/src/isa/x64/inst/mod.rs b/cranelift/codegen/src/isa/x64/inst/mod.rs index aaa698b5fe..7ea05103e7 100644 --- a/cranelift/codegen/src/isa/x64/inst/mod.rs +++ b/cranelift/codegen/src/isa/x64/inst/mod.rs @@ -92,7 +92,8 @@ impl Inst { | Inst::Mov64MR { .. } | Inst::MovRM { .. } | Inst::MovRR { .. } - | Inst::MovPReg { .. } + | Inst::MovFromPReg { .. } + | Inst::MovToPReg { .. } | Inst::MovsxRmR { .. } | Inst::MovzxRmR { .. } | Inst::MulHi { .. } @@ -1233,13 +1234,20 @@ impl PrettyPrint for Inst { ) } - Inst::MovPReg { src, dst } => { + Inst::MovFromPReg { src, dst } => { let src: Reg = (*src).into(); let src = regs::show_ireg_sized(src, 8); let dst = pretty_print_reg(dst.to_reg().to_reg(), 8, allocs); format!("{} {}, {}", ljustify("movq".to_string()), src, dst) } + Inst::MovToPReg { src, dst } => { + let src = pretty_print_reg(src.to_reg(), 8, allocs); + let dst: Reg = (*dst).into(); + let dst = regs::show_ireg_sized(dst, 8); + format!("{} {}, {}", ljustify("movq".to_string()), src, dst) + } + Inst::MovzxRmR { ext_mode, src, dst, .. } => { @@ -1877,11 +1885,16 @@ fn x64_get_operands VReg>(inst: &Inst, collector: &mut OperandCol collector.reg_use(src.to_reg()); collector.reg_def(dst.to_writable_reg()); } - Inst::MovPReg { dst, src } => { - debug_assert!([regs::rsp(), regs::rbp()].contains(&(*src).into())); + Inst::MovFromPReg { dst, src } => { + debug_assert!([regs::rsp(), regs::rbp(), regs::pinned_reg()].contains(&(*src).into())); debug_assert!(dst.to_reg().to_reg().is_virtual()); collector.reg_def(dst.to_writable_reg()); } + Inst::MovToPReg { dst, src } => { + debug_assert!(src.to_reg().is_virtual()); + debug_assert!([regs::rsp(), regs::rbp(), regs::pinned_reg()].contains(&(*dst).into())); + collector.reg_use(src.to_reg()); + } Inst::XmmToGpr { src, dst, .. } => { collector.reg_use(src.to_reg()); collector.reg_def(dst.to_writable_reg()); diff --git a/cranelift/codegen/src/isa/x64/lower/isle.rs b/cranelift/codegen/src/isa/x64/lower/isle.rs index 17776d289e..71c519a148 100644 --- a/cranelift/codegen/src/isa/x64/lower/isle.rs +++ b/cranelift/codegen/src/isa/x64/lower/isle.rs @@ -640,6 +640,11 @@ impl Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6> { regs::rsp().to_real_reg().unwrap().into() } + #[inline] + fn preg_pinned(&mut self) -> PReg { + regs::pinned_reg().to_real_reg().unwrap().into() + } + fn libcall_1(&mut self, libcall: &LibCall, a: Reg) -> Reg { let call_conv = self.lower_ctx.abi().call_conv(self.lower_ctx.sigs()); let ret_ty = libcall.signature(call_conv).returns[0].value_type; @@ -769,11 +774,6 @@ impl Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6> { .use_constant(VCodeConstantData::WellKnown(&UMAX_MASK)) } - #[inline] - fn pinned_writable_gpr(&mut self) -> WritableGpr { - Writable::from_reg(Gpr::new(regs::pinned_reg()).unwrap()) - } - #[inline] fn shuffle_0_31_mask(&mut self, mask: &VecMask) -> VCodeConstant { let mask = mask diff --git a/cranelift/codegen/src/machinst/lower.rs b/cranelift/codegen/src/machinst/lower.rs index 8e7dc6a02c..e09b5585c8 100644 --- a/cranelift/codegen/src/machinst/lower.rs +++ b/cranelift/codegen/src/machinst/lower.rs @@ -1276,25 +1276,12 @@ impl<'func, I: VCodeInst> Lower<'func, I> { } } - let mut regs = self.value_regs[val]; + let regs = self.value_regs[val]; trace!(" -> regs {:?}", regs); assert!(regs.is_valid()); self.value_lowered_uses[val] += 1; - // Pinned-reg hack: if backend specifies a fixed pinned register, use it - // directly when we encounter a GetPinnedReg op, rather than lowering - // the actual op, and do not return the source inst to the caller; the - // value comes "out of the ether" and we will not force generation of - // the superfluous move. - if let ValueDef::Result(i, 0) = self.f.dfg.value_def(val) { - if self.f.dfg[i].opcode() == Opcode::GetPinnedReg { - if let Some(pr) = self.pinned_reg { - regs = ValueRegs::one(pr); - } - } - } - regs } diff --git a/cranelift/filetests/filetests/isa/aarch64/pinned-reg.clif b/cranelift/filetests/filetests/isa/aarch64/pinned-reg.clif index caa60dc73f..1e2669d314 100644 --- a/cranelift/filetests/filetests/isa/aarch64/pinned-reg.clif +++ b/cranelift/filetests/filetests/isa/aarch64/pinned-reg.clif @@ -11,6 +11,8 @@ block0: } ; block0: -; add x21, x21, #1 +; mov x1, x21 +; add x1, x1, #1 +; mov x21, x1 ; ret diff --git a/cranelift/filetests/filetests/isa/x64/pinned-reg.clif b/cranelift/filetests/filetests/isa/x64/pinned-reg.clif index a11568d04d..6588f16261 100644 --- a/cranelift/filetests/filetests/isa/x64/pinned-reg.clif +++ b/cranelift/filetests/filetests/isa/x64/pinned-reg.clif @@ -13,7 +13,9 @@ block0: ; pushq %rbp ; movq %rsp, %rbp ; block0: -; addq %r15, $1, %r15 +; movq %r15, %rsi +; addq %rsi, $1, %rsi +; movq %rsi, %r15 ; movq %rbp, %rsp ; popq %rbp ; ret @@ -28,8 +30,14 @@ block0: ; pushq %rbp ; movq %rsp, %rbp +; subq %rsp, $16, %rsp +; movq %rsi, 0(%rsp) ; block0: -; addq %r15, $1, %r15 +; movq %r15, %rsi +; addq %rsi, $1, %rsi +; movq %rsi, %r15 +; movq 0(%rsp), %rsi +; addq %rsp, $16, %rsp ; movq %rbp, %rsp ; popq %rbp ; ret