Repository: bytecodealliance/wasmtime
Pull request #9398 (2024-10-08 16:42:17)

s390x cleanup: Remove unused memcpy infrastructure
After commit c711fcfd92a5d8e2591f3ba74fb7fd7af53c11f8, there is no longer any user of the memcpy clause in the s390x ISLE files.

Removing that allows a bunch of further cleanups, as this was the sole remaining user of the libcall infrastructure, as well as the sole remaining instruction using two memory operands.

<!--
Please make sure you include the following information:

- If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

- Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->



Patch:
diff --git a/cranelift/codegen/src/isa/s390x/inst.isle b/cranelift/codegen/src/isa/s390x/inst.isle
index 74022b0e5..451dda7a5 100644
--- a/cranelift/codegen/src/isa/s390x/inst.isle
+++ b/cranelift/codegen/src/isa/s390x/inst.isle
@@ -372,12 +372,6 @@
       (rd Reg)
       (mem MemArg))
 
-    ;; A memory copy of 1-256 bytes.
-    (Mvc
-      (dst MemArgPair)
-      (src MemArgPair)
-      (len_minus_one u8))
-
     ;; A load-multiple instruction.
     (LoadMultiple64
       (rt WritableReg)
@@ -1860,25 +1854,6 @@
       inst)
 
 
-;; Accessors for `MemArgPair`.
-
-(type MemArgPair extern (enum))
-
-;; Convert a MemArg to a MemArgPair, reloading the address if necessary.
-(decl memarg_pair (MemArg) MemArgPair)
-(rule 1 (memarg_pair (memarg_pair_from_memarg mem)) mem)
-(rule (memarg_pair mem) (memarg_pair_from_reg
-                          (load_addr mem) (memarg_flags mem)))
-
-;; Convert a MemArg to a MemArgPair if no reloading is necessary.
-(decl memarg_pair_from_memarg (MemArgPair) MemArg)
-(extern extractor memarg_pair_from_memarg memarg_pair_from_memarg)
-
-;; Create a MemArgPair from a single base register.
-(decl memarg_pair_from_reg (Reg MemFlags) MemArgPair)
-(extern constructor memarg_pair_from_reg memarg_pair_from_reg)
-
-
 ;; Helpers for stack-slot addresses ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (decl stack_addr_impl (Type StackSlot Offset32) Reg)
@@ -2320,11 +2295,6 @@
 (rule (storerev64 src addr)
       (SideEffectNoResult.Inst (MInst.StoreRev64 src addr)))
 
-;; Helper for emitting `MInst.Mvc` instructions.
-(decl mvc (MemArgPair MemArgPair u8) SideEffectNoResult)
-(rule (mvc dst src len_minus_one)
-      (SideEffectNoResult.Inst (MInst.Mvc dst src len_minus_one)))
-
 ;; Helper for emitting `MInst.LoadAR` instructions.
 (decl load_ar (u8) Reg)
 (rule (load_ar ar)
@@ -2916,14 +2886,6 @@
       (if-let $false (lane_order_equal callee_lane_order (lane_order)))
       (vec_elt_rev ty reg))
 
-;; Helpers to emit a memory copy (MVC or memcpy libcall).
-(decl memcpy (MemArg MemArg u64) SideEffectNoResult)
-(rule 1 (memcpy dst src (len_minus_one len))
-      (mvc (memarg_pair dst) (memarg_pair src) len))
-(rule (memcpy dst src len)
-      (lib_call
-        (lib_call_info_memcpy (load_addr dst) (load_addr src) (imm $I64 len))))
-
 ;; Prepare a stack copy of a single (oversized) argument.
 (decl copy_to_buffer (MemArg ABIArg Value) InstOutput)
 (rule 2 (copy_to_buffer base (abi_arg_only_slot slot) _) (output_none))
@@ -3604,18 +3566,6 @@
 (extern constructor abi_lane_order abi_lane_order)
 
 
-;; Helpers for generating calls to library routines ;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-
-(type LibCallInfo extern (enum))
-
-(decl lib_call_info_memcpy (Reg Reg Reg) BoxCallInfo)
-(extern constructor lib_call_info_memcpy lib_call_info_memcpy)
-
-(decl lib_call (BoxCallInfo) SideEffectNoResult)
-(rule (lib_call info)
-      (call_impl (writable_link_reg) info))
-
-
 ;; Helpers for generating vector pack and unpack instructions ;;;;;;;;;;;;;;;;;;
 
 (decl vec_widen_type (Type) Type)
diff --git a/cranelift/codegen/src/isa/s390x/inst/args.rs b/cranelift/codegen/src/isa/s390x/inst/args.rs
index 0aa96bf58..5134f9bd3 100644
--- a/cranelift/codegen/src/isa/s390x/inst/args.rs
+++ b/cranelift/codegen/src/isa/s390x/inst/args.rs
@@ -124,53 +124,6 @@ impl MemArg {
     }
 }
 
-/// A memory argument for an instruction with two memory operands.
-/// We cannot use two instances of MemArg, because we do not have
-/// two free temp registers that would be needed to reload two
-/// addresses in the general case.  Also, two copies of MemArg would
-/// increase the size of Inst beyond its current limit.  Use this
-/// simplified form instead that never needs any reloads, and suffices
-/// for all current users.
-#[derive(Clone, Debug)]
-pub struct MemArgPair {
-    pub base: Reg,
-    pub disp: UImm12,
-    pub flags: MemFlags,
-}
-
-impl MemArgPair {
-    /// Convert a MemArg to a MemArgPair if possible.
-    pub fn maybe_from_memarg(mem: &MemArg) -> Option<MemArgPair> {
-        match mem {
-            &MemArg::BXD12 {
-                base,
-                index,
-                disp,
-                flags,
-            } => {
-                if index != zero_reg() {
-                    None
-                } else {
-                    Some(MemArgPair { base, disp, flags })
-                }
-            }
-            &MemArg::RegOffset { reg, off, flags } => {
-                if off < 0 {
-                    None
-                } else {
-                    let disp = UImm12::maybe_from_u64(off as u64)?;
-                    Some(MemArgPair {
-                        base: reg,
-                        disp,
-                        flags,
-                    })
-                }
-            }
-            _ => None,
-        }
-    }
-}
-
 //=============================================================================
 // Instruction sub-components (conditions, branches and branch targets):
 // definitions
diff --git a/cranelift/codegen/src/isa/s390x/inst/emit.rs b/cranelift/codegen/src/isa/s390x/inst/emit.rs
index acc63cad6..3947d1b09 100644
--- a/cranelift/codegen/src/isa/s390x/inst/emit.rs
+++ b/cranelift/codegen/src/isa/s390x/inst/emit.rs
@@ -368,34 +368,6 @@ pub fn mem_imm16_emit(
     }
 }
 
-pub fn mem_mem_emit(
-    dst: &MemArgPair,
-    src: &MemArgPair,
-    len_minus_one: u8,
-    opcode_ss: u8,
-    add_trap: bool,
-    sink: &mut MachBuffer<Inst>,
-    _state: &mut EmitState,
-) {
-    if add_trap {
-        if let Some(trap_code) = dst.flags.trap_code().or(src.flags.trap_code()) {
-            sink.add_trap(trap_code);
-        }
-    }
-
-    put(
-        sink,
-        &enc_ss_a(
-            opcode_ss,
-            dst.base,
-            dst.disp.bits(),
-            src.base,
-            src.disp.bits(),
-            len_minus_one,
-        ),
-    );
-}
-
 pub fn mem_vrx_emit(
     rd: Reg,
     mem: &MemArg,
@@ -963,31 +935,6 @@ fn enc_siy(opcode: u16, b1: Reg, d1: u32, i2: u8) -> [u8; 6] {
     enc
 }
 
-/// SSa-type instructions.
-///
-///   47     39 31 27 15 11
-///   opcode  l b1 d1 b2 d2
-///       40 32 28 16 12  0
-///
-///
-fn enc_ss_a(opcode: u8, b1: Reg, d1: u32, b2: Reg, d2: u32, l: u8) -> [u8; 6] {
-    let b1 = machreg_to_gpr(b1) & 0x0f;
-    let d1_lo = (d1 & 0xff) as u8;
-    let d1_hi = ((d1 >> 8) & 0x0f) as u8;
-    let b2 = machreg_to_gpr(b2) & 0x0f;
-    let d2_lo = (d2 & 0xff) as u8;
-    let d2_hi = ((d2 >> 8) & 0x0f) as u8;
-
-    let mut enc: [u8; 6] = [0; 6];
-    enc[0] = opcode;
-    enc[1] = l;
-    enc[2] = b1 << 4 | d1_hi;
-    enc[3] = d1_lo;
-    enc[4] = b2 << 4 | d2_hi;
-    enc[5] = d2_lo;
-    enc
-}
-
 /// VRIa-type instructions.
 ///
 ///   47      39 35 31 15 11  7
@@ -2185,16 +2132,6 @@ impl Inst {
                 };
                 mem_imm16_emit(imm, &mem, opcode, true, sink, emit_info, state);
             }
-            &Inst::Mvc {
-                ref dst,
-                ref src,
-                len_minus_one,
-            } => {
-                let dst = dst.clone();
-                let src = src.clone();
-                let opcode = 0xd2; // MVC
-                mem_mem_emit(&dst, &src, len_minus_one, opcode, true, sink, state);
-            }
 
             &Inst::LoadMultiple64 { rt, rt2, ref mem } => {
                 let mem = mem.clone();
@@ -2248,9 +2185,7 @@ impl Inst {
                 put(sink, &enc_rre(opcode, rd.to_reg(), rm));
             }
             &Inst::MovPReg { rd, rm } => {
-                let rm: Reg = rm.into();
-                debug_assert!([regs::gpr(0), regs::gpr(14), regs::gpr(15)].contains(&rm));
-                Inst::Mov64 { rd, rm }.emit(sink, emit_info, state);
+                Inst::Mov64 { rd, rm: rm.into() }.emit(sink, emit_info, state);
             }
             &Inst::Mov32 { rd, rm } => {
                 let opcode = 0x18; // LR
@@ -3258,8 +3193,6 @@ impl Inst {
                 state.nominal_sp_offset += size;
             }
             &Inst::Call { link, ref info } => {
-                debug_assert_eq!(link.to_reg(), gpr(14));
-
                 let opcode = 0xc05; // BRASL
 
                 // Add relocation for target function.  This has to be done *before*
@@ -3278,8 +3211,6 @@ impl Inst {
                 state.nominal_sp_offset -= info.callee_pop_size;
             }
             &Inst::CallInd { link, ref info } => {
-                debug_assert_eq!(link.to_reg(), gpr(14));
-
                 if let Some(s) = state.take_stack_map() {
                     let offset = sink.cur_offset() + 2;
                     sink.push_user_stack_map(state, offset, s);
@@ -3322,8 +3253,6 @@ impl Inst {
             &Inst::ElfTlsGetOffset {
                 ref symbol, link, ..
             } => {
-                debug_assert_eq!(link.to_reg(), gpr(14));
-
                 let opcode = 0xc05; // BRASL
 
                 // Add relocation for target function. This has to be done
@@ -3336,13 +3265,12 @@ impl Inst {
                     _ => unreachable!(),
                 }
 
-                put(sink, &enc_ril_b(opcode, gpr(14), 0));
+                put(sink, &enc_ril_b(opcode, link.to_reg(), 0));
                 sink.add_call_site();
             }
             &Inst::Args { .. } => {}
             &Inst::Rets { .. } => {}
             &Inst::Ret { link } => {
-                debug_assert_eq!(link, gpr(14));
                 let opcode = 0x07; // BCR
                 put(sink, &enc_rr(opcode, gpr(15), link));
             }
diff --git a/cranelift/codegen/src/isa/s390x/inst/mod.rs b/cranelift/codegen/src/isa/s390x/inst/mod.rs
index bccd0057b..9c3322143 100644
--- a/cranelift/codegen/src/isa/s390x/inst/mod.rs
+++ b/cranelift/codegen/src/isa/s390x/inst/mod.rs
@@ -148,7 +148,6 @@ impl Inst {
             | Inst::StoreRev16 { .. }
             | Inst::StoreRev32 { .. }
             | Inst::StoreRev64 { .. }
-            | Inst::Mvc { .. }
             | Inst::LoadMultiple64 { .. }
             | Inst::StoreMultiple64 { .. }
             | Inst::Mov32 { .. }
@@ -564,10 +563,6 @@ fn s390x_get_operands(inst: &mut Inst, collector: &mut DenyReuseVisitor<impl Ope
         | Inst::StoreImm64SExt16 { mem, .. } => {
             memarg_operands(mem, collector);
         }
-        Inst::Mvc { dst, src, .. } => {
-            collector.reg_use(&mut dst.base);
-            collector.reg_use(&mut src.base);
-        }
         Inst::LoadMultiple64 { rt, rt2, mem, .. } => {
             memarg_operands(mem, collector);
             let first_regnum = rt.to_reg().to_real_reg().unwrap().hw_enc();
@@ -589,9 +584,8 @@ fn s390x_get_operands(inst: &mut Inst, collector: &mut DenyReuseVisitor<impl Ope
             collector.reg_use(rm);
         }
         Inst::MovPReg { rd, rm } => {
-            debug_assert!([gpr_preg(0), gpr_preg(14), gpr_preg(15)].contains(rm));
-            debug_assert!(rd.to_reg().is_virtual());
             collector.reg_def(rd);
+            collector.reg_fixed_nonallocatable(*rm);
         }
         Inst::Mov32 { rd, rm } => {
             collector.reg_def(rd);
@@ -1937,22 +1931,6 @@ impl Inst {
 
                 format!("{mem_str}{op} {mem}, {imm}")
             }
-            &Inst::Mvc {
-                ref dst,
-                ref src,
-                len_minus_one,
-            } => {
-                let dst = dst.clone();
-                let src = src.clone();
-                format!(
-                    "mvc {}({},{}), {}({})",
-                    dst.disp.pretty_print_default(),
-                    len_minus_one,
-                    show_reg(dst.base),
-                    src.disp.pretty_print_default(),
-                    show_reg(src.base)
-                )
-            }
             &Inst::LoadMultiple64 { rt, rt2, ref mem } => {
                 let mem = mem.clone();
                 let (mem_str, mem) = mem_finalize_for_show(
@@ -3148,7 +3126,6 @@ impl Inst {
                 } else {
                     "".to_string()
                 };
-                debug_assert_eq!(link, gpr(14));
                 format!(
                     "brasl {}, {}{}",
                     show_reg(link),
@@ -3164,7 +3141,6 @@ impl Inst {
                 } else {
                     "".to_string()
                 };
-                debug_assert_eq!(link, gpr(14));
                 format!("basr {}, {}{}", show_reg(link), rn, callee_pop_size)
             }
             &Inst::ReturnCall { ref info } => {
@@ -3196,7 +3172,6 @@ impl Inst {
                     }
                     _ => unreachable!(),
                 };
-                debug_assert_eq!(link, gpr(14));
                 format!("brasl {}, {}", show_reg(link), dest)
             }
             &Inst::Args { ref args } => {
@@ -3218,7 +3193,6 @@ impl Inst {
                 s
             }
             &Inst::Ret { link } => {
-                debug_assert_eq!(link, gpr(14));
                 let link = show_reg(link);
                 format!("br {link}")
             }
diff --git a/cranelift/codegen/src/isa/s390x/lower/isle.rs b/cranelift/codegen/src/isa/s390x/lower/isle.rs
index 6fdb72aa9..c65aa9c21 100644
--- a/cranelift/codegen/src/isa/s390x/lower/isle.rs
+++ b/cranelift/codegen/src/isa/s390x/lower/isle.rs
@@ -7,8 +7,8 @@ use crate::ir::ExternalName;
 // Types that the generated ISLE code uses via `use super::*`.
 use crate::isa::s390x::abi::{S390xMachineDeps, REG_SAVE_AREA_SIZE};
 use crate::isa::s390x::inst::{
-    gpr, stack_reg, writable_gpr, zero_reg, Cond, Inst as MInst, LaneOrder, MemArg, MemArgPair,
-    RegPair, ReturnCallInfo, SymbolReloc, UImm12, UImm16Shifted, UImm32Shifted, WritableRegPair,
+    gpr, stack_reg, writable_gpr, zero_reg, Cond, Inst as MInst, LaneOrder, MemArg, RegPair,
+    ReturnCallInfo, SymbolReloc, UImm12, UImm16Shifted, UImm32Shifted, WritableRegPair,
 };
 use crate::isa::s390x::S390xBackend;
 use crate::machinst::isle::*;
@@ -16,8 +16,8 @@ use crate::machinst::{CallInfo, MachLabel, Reg};
 use crate::{
     ir::{
         condcodes::*, immediates::*, types::*, ArgumentExtension, ArgumentPurpose, AtomicRmwOp,
-        BlockCall, Endianness, Inst, InstructionData, KnownSymbol, LibCall, MemFlags, Opcode,
-        TrapCode, Value, ValueList,
+        BlockCall, Endianness, Inst, InstructionData, KnownSymbol, MemFlags, Opcode, TrapCode,
+        Value, ValueList,
     },
     isa::CallConv,
     machinst::abi::ABIMachineSpec,
@@ -309,47 +309,6 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, S390xBackend> {
         })
     }
 
-    fn lib_call_info_memcpy(
-        &mut self,
-        dst: Reg,
-        src: Reg,
-        len: Reg,
-    ) -> Box<CallInfo<ExternalName>> {
-        let caller_conv = self.lower_ctx.abi().call_conv(self.lower_ctx.sigs());
-        let callee_conv = CallConv::for_libcall(&self.backend.flags, caller_conv);
-
-        // Clobbers are defined by the calling convention. We will remove return value regs later.
-        let clobbers = S390xMachineDeps::get_regs_clobbered_by_call(callee_conv);
-
-        // Libcalls only require the register save area.
-        self.lower_ctx
-            .abi_mut()
-            .accumulate_outgoing_args_size(REG_SAVE_AREA_SIZE);
-
-        Box::new(CallInfo {
-            dest: ExternalName::LibCall(LibCall::Memcpy),
-            uses: smallvec![
-                CallArgPair {
-                    vreg: dst,
-                    preg: gpr(2),
-                },
-                CallArgPair {
-                    vreg: src,
-                    preg: gpr(3),
-                },
-                CallArgPair {
-                    vreg: len,
-                    preg: gpr(4),
-                },
-            ],
-            defs: smallvec![],
-            clobbers,
-            caller_conv,
-            callee_conv,
-            callee_pop_size: 0,
-        })
-    }
-
     fn abi_for_elf_tls_get_offset(&mut self) {
         self.lower_ctx
             .abi_mut()
@@ -916,20 +875,6 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, S390xBackend> {
         }
     }
 
-    #[inline]
-    fn memarg_pair_from_memarg(&mut self, mem: &MemArg) -> Option<MemArgPair> {
-        MemArgPair::maybe_from_memarg(mem)
-    }
-
-    #[inline]
-    fn memarg_pair_from_reg(&mut self, reg: Reg, flags: MemFlags) -> MemArgPair {
-        MemArgPair {
-            base: reg,
-            disp: UImm12::zero(),
-            flags,
-        }
-    }
-
     #[inline]
     fn memarg_frame_pointer_offset(&mut self) -> MemArg {
         // The frame pointer (back chain) is stored directly at SP.


Test patch:
diff --git a/cranelift/codegen/src/isa/s390x/inst/emit_tests.rs b/cranelift/codegen/src/isa/s390x/inst/emit_tests.rs
index ca59b0535..6c03b210b 100644
--- a/cranelift/codegen/src/isa/s390x/inst/emit_tests.rs
+++ b/cranelift/codegen/src/isa/s390x/inst/emit_tests.rs
@@ -6037,24 +6037,6 @@ fn test_s390x_binemit() {
         "stgrl %r1, label1",
     ));
 
-    insns.push((
-        Inst::Mvc {
-            dst: MemArgPair {
-                base: gpr(2),
-                disp: UImm12::maybe_from_u64(0x345).unwrap(),
-                flags: MemFlags::trusted(),
-            },
-            src: MemArgPair {
-                base: gpr(8),
-                disp: UImm12::maybe_from_u64(0x9ab).unwrap(),
-                flags: MemFlags::trusted(),
-            },
-            len_minus_one: 255,
-        },
-        "D2FF234589AB",
-        "mvc 837(255,%r2), 2475(%r8)",
-    ));
-
     insns.push((
         Inst::LoadMultiple64 {
             rt: writable_gpr(8),