majit dynasm: partition the parallel-move keys, fault outside the operand contract, and fix the field-width witness

majit/majit-backend-dynasm/src/aarch64/assembler.rs

@@ -9,6 +9,7 @@
 ///   _assemble — assembler.py:779 (walk ops + emit code)
 ///   patch_jump_for_descr — assembler.py:965
 ///   redirect_call_assembler — assembler.py:1138
+use crate::regloc::ebp_loc_pat;
 use indexmap::IndexMap;
 use majit_ir::IndexMapExt;
 use std::sync::Arc;
@@ -7945,16 +7946,16 @@ impl<'a> crate::jump::RegallocMoves for AssemblerARM64<'a> {
                     dynasm!(self.mc ; .arch aarch64 ; fmov D(d.value), X(s.value));
                 }
             }
-            (Loc::Reg(s), Loc::Frame(f)) => {
-                let ofs = f.ebp_loc.value;
+            (Loc::Reg(s), ebp_loc_pat!(e)) => {
+                let ofs = e.value;
                 if s.is_xmm {
                     self.emit_str_fp_d(s.value, ofs);
                 } else {
                     self.emit_str_fp(s.value, ofs);
                 }
             }
-            (Loc::Frame(f), Loc::Reg(d)) => {
-                let ofs = f.ebp_loc.value;
+            (ebp_loc_pat!(e), Loc::Reg(d)) => {
+                let ofs = e.value;
                 if d.is_xmm {
                     self.emit_ldr_fp_d(d.value, ofs);
                 } else {
@@ -7969,19 +7970,23 @@ impl<'a> crate::jump::RegallocMoves for AssemblerARM64<'a> {
                     self.emit_mov_imm64(d.value as u32, i.value);
                 }
             }
-            (Loc::Immed(i), Loc::Frame(f)) => {
-                let ofs = f.ebp_loc.value;
+            (Loc::Immed(i), ebp_loc_pat!(e)) => {
+                let ofs = e.value;
                 self.emit_mov_imm64(16, i.value);
                 self.emit_str_fp(16, ofs);
             }
-            (Loc::Frame(f1), Loc::Frame(f2)) if f1.position == f2.position => {}
-            (Loc::Frame(f1), Loc::Frame(f2)) => {
-                let o1 = f1.ebp_loc.value;
-                let o2 = f2.ebp_loc.value;
+            (ebp_loc_pat!(e1), ebp_loc_pat!(e2)) if e1.value == e2.value => {}
+            (ebp_loc_pat!(e1), ebp_loc_pat!(e2)) => {
+                let o1 = e1.value;
+                let o2 = e2.value;
                 self.emit_ldr_fp(16, o1);
                 self.emit_str_fp(16, o2);
             }
-            _ => {}
+            _ => panic!(
+                "parallel move {src:?} -> {dst:?} is outside the RegallocMoves \
+                 operand contract; emitting nothing here would leave the \
+                 destination stale",
+            ),
         }
     }
 
@@ -7993,15 +7998,18 @@ impl<'a> crate::jump::RegallocMoves for AssemblerARM64<'a> {
             Loc::Reg(r) => {
                 dynasm!(self.mc ; .arch aarch64 ; str X(r.value), [sp, #-16]!);
             }
-            Loc::Frame(f) if f.ebp_loc.is_float => {
-                self.emit_ldr_fp_d(15, f.ebp_loc.value);
+            ebp_loc_pat!(e) if e.is_float => {
+                self.emit_ldr_fp_d(15, e.value);
                 dynasm!(self.mc ; .arch aarch64 ; str D(15), [sp, #-16]!);
             }
-            Loc::Frame(f) => {
-                self.emit_ldr_fp(16, f.ebp_loc.value);
+            ebp_loc_pat!(e) => {
+                self.emit_ldr_fp(16, e.value);
                 dynasm!(self.mc ; .arch aarch64 ; str x16, [sp, #-16]!);
             }
-            _ => {}
+            _ => panic!(
+                "parallel move cannot park {loc:?} on the stack; emitting nothing \
+                 here would leave the matching pop unbalanced",
+            ),
         }
     }
 
@@ -8013,15 +8021,18 @@ impl<'a> crate::jump::RegallocMoves for AssemblerARM64<'a> {
             Loc::Reg(r) => {
                 dynasm!(self.mc ; .arch aarch64 ; ldr X(r.value), [sp], #16);
             }
-            Loc::Frame(f) if f.ebp_loc.is_float => {
+            ebp_loc_pat!(e) if e.is_float => {
                 dynasm!(self.mc ; .arch aarch64 ; ldr D(15), [sp], #16);
-                self.emit_str_fp_d(15, f.ebp_loc.value);
+                self.emit_str_fp_d(15, e.value);
             }
-            Loc::Frame(f) => {
+            ebp_loc_pat!(e) => {
                 dynasm!(self.mc ; .arch aarch64 ; ldr x16, [sp], #16);
-                self.emit_str_fp(16, f.ebp_loc.value);
+                self.emit_str_fp(16, e.value);
             }
-            _ => {}
+            _ => panic!(
+                "parallel move cannot restore {loc:?} from the stack; emitting \
+                 nothing here would leave the stack pointer shifted",
+            ),
         }
     }
 }

majit/majit-backend-dynasm/src/jump.rs

@@ -25,9 +25,20 @@ use indexmap::IndexMap;
 /// The three emitters `remap_frame_layout` drives, named as `jump.py` calls
 /// them on the `assembler` it is handed.
 ///
+/// **Operand contract.** A destination is a register or a frame-pointer
+/// location in either spelling — `Loc::Frame`, which knows its stack position,
+/// or the bare `Loc::Ebp`, which does not (`regloc.py:113 class
+/// FrameLoc(RawEbpLoc)`). A source is one of those or an immediate. The one
+/// location left out is `Loc::Addr`, which upstream cannot key either. An
+/// implementation must fault on anything outside this rather than emit
+/// nothing: a move that is counted in `pending_dests` and then silently
+/// dropped leaves the destination holding a stale value, and a dropped
+/// `regalloc_pop` leaves the machine stack pointer shifted as well.
+///
 /// `regalloc_push`/`regalloc_pop` exist only to serve the cycle-breaking arm
 /// below — parking one value of a cycle on the machine stack is the whole
-/// reason a cycle can be resolved at all.
+/// reason a cycle can be resolved at all. They are only ever handed a
+/// destination, so an immediate never reaches them.
 pub(crate) trait RegallocMoves {
     /// `assembler.py:1145 regalloc_mov(from_loc, to_loc)`.
     fn regalloc_mov(&mut self, src: &Loc, dst: &Loc);
@@ -37,20 +48,70 @@ pub(crate) trait RegallocMoves {
 
 /// A location's identity for the dependency bookkeeping.
 ///
-/// Registers and frame slots share one number space, so the constants keep the
-/// classes apart; an immediate has no identity because it is never anyone's
-/// destination and can be re-materialised at will.
+/// Two locations must get the same key exactly when they are the same storage.
+/// Handing two distinct destinations one key collapses them to a single
+/// `IndexMap` entry, and the move for whichever one loses is never emitted.
+///
+/// Registers and stack slots are told apart **by sign**: a register key is
+/// positive, a stack key is `!offset` and so negative. Upstream instead keys a
+/// register on its bare number and a frame slot on its byte offset, and argues
+/// the two cannot meet because offsets start above the register file —
+/// `regloc.py:117-120` says so in as many words and asserts `ebp_offset >= 8 +
+/// 8 * IS_X86_64` rather than trusting it. That argument does not survive the
+/// per-class bias below: an offset of 4096 is an ordinary frame for a large
+/// trace and is exactly the general-register base.
+///
+/// The bias is still worth keeping. Upstream gives `r0` and `xmm0` the same key
+/// — harmless there because the two files are remapped in separate calls, but
+/// `remap_frame_layout_mixed` compares one call's destination keys against the
+/// other's sources, which is precisely across the two files.
 pub(crate) fn loc_as_key(loc: &Loc) -> i32 {
+    /// Keeps the two register files apart; both stay positive.
+    const XMM_KEY_BASE: i32 = 0x2000;
+    const GPR_KEY_BASE: i32 = 0x1000;
+
     match loc {
-        Loc::Reg(r) if r.is_xmm => 0x2000 + i32::from(r.value),
-        Loc::Reg(r) => 0x1000 + i32::from(r.value),
-        Loc::Frame(f) => f.ebp_loc.value,
-        Loc::Ebp(e) => e.value,
+        Loc::Reg(r) if r.is_xmm => XMM_KEY_BASE + i32::from(r.value),
+        Loc::Reg(r) => GPR_KEY_BASE + i32::from(r.value),
+        // `!offset` for a non-negative offset is negative, so no stack slot can
+        // ever land on a register key however deep the frame gets.
+        Loc::Frame(f) => stack_key(f.ebp_loc.value),
+        Loc::Ebp(e) => stack_key(e.value),
+        // Never a destination and re-materialisable at will, so it needs no
+        // identity — only a value no real location can take.
         Loc::Immed(_) => i32::MIN,
-        Loc::Addr(a) => a.offset,
+        // `AddressLoc` is the one location class upstream leaves without a key:
+        // it overrides neither `_getregkey` nor, for its `'a'`/`'m'` codes, the
+        // `value` the inherited one reads (`regloc.py:207-250`), so a parallel
+        // move handed one raises there as well. Minting a key here instead
+        // would put an entry in `pending_dests` that no emitter can retire.
+        Loc::Addr(a) => panic!(
+            "parallel move over an address location (offset {}), which no \
+             regalloc_mov can emit",
+            a.offset,
+        ),
     }
 }
 
+/// The key for a stack slot at `offset` bytes from the frame pointer.
+fn stack_key(offset: i32) -> i32 {
+    debug_assert!(
+        offset >= 0,
+        "a negative frame offset ({offset}) inverts back into the positive \
+         register key space",
+    );
+    !offset
+}
+
+/// The key of the slot one machine word past `key`.
+///
+/// Stack keys run backwards against offsets — `!(offset + WORD)` is
+/// `!offset - WORD` — so the neighbour is found by subtracting, and writing
+/// `key + WORD` here would silently read the slot on the wrong side.
+fn stack_key_next_word(key: i32) -> i32 {
+    key - WORD as i32
+}
+
 pub(crate) fn loc_width(loc: &Loc) -> usize {
     match loc {
         Loc::Reg(r) => r.get_width(),
@@ -74,7 +135,16 @@ pub(crate) fn remap_frame_layout<A: RegallocMoves + ?Sized>(
     let mut pending_dests = dst_locations.len() as i32;
     let mut srccount: IndexMap<i32, i32> = IndexMap::new();
     for dst in dst_locations {
-        srccount.insert(loc_as_key(dst), 0);
+        // `jump.py:7 assert key not in srccount`. A repeated destination shares
+        // one entry while `pending_dests` counts both, so the second one can
+        // never be retired: every key reaches -1, the loop stops making
+        // progress, and the cycle-breaking arm finds no key left at or above
+        // zero to park — the whole call spins. `insert` returns the displaced
+        // value, so the check costs nothing beyond the store already made.
+        assert!(
+            srccount.insert(loc_as_key(dst), 0).is_none(),
+            "duplicate value in dst_locations!",
+        );
     }
     for i in 0..dst_locations.len() {
         let src = src_locations[i];
@@ -178,7 +248,7 @@ pub(crate) fn remap_frame_layout_mixed<A: RegallocMoves + ?Sized>(
         if loc.is_stack() {
             let key = loc_as_key(&loc);
             if dst_keys.contains_key(&key)
-                || (loc_width(&loc) > WORD && dst_keys.contains_key(&(key + WORD as i32)))
+                || (loc_width(&loc) > WORD && dst_keys.contains_key(&stack_key_next_word(key)))
             {
                 asm.regalloc_push(&loc);
                 extrapushes.push(dstloc);

majit/majit-backend-dynasm/src/regloc.rs

@@ -159,12 +159,35 @@ pub enum Loc {
     Addr(AddressLoc),
 }
 
+/// Matches either spelling of a frame-pointer location, binding its
+/// `RawEbpLoc`.
+///
+/// `regloc.py:113 class FrameLoc(RawEbpLoc)` — the two are one type upstream,
+/// separated here only because Rust has no inheritance. Every operand that
+/// addresses through the frame pointer accepts both (`mov`'s `'b'` code), so a
+/// match naming just one spelling silently excludes the other.
+macro_rules! ebp_loc_pat {
+    ($e:ident) => {
+        $crate::regloc::Loc::Frame($crate::regloc::FrameLoc { ebp_loc: $e, .. })
+            | $crate::regloc::Loc::Ebp($e)
+    };
+}
+
+pub(crate) use ebp_loc_pat;
+
 impl Loc {
     pub fn is_reg(&self) -> bool {
         matches!(self, Loc::Reg(_))
     }
+    /// `regloc.py:82 RawEbpLoc.is_stack` returns True, and `FrameLoc` inherits
+    /// it — a frame slot is a raw ebp location that also knows its stack
+    /// position, so both spellings are stack locations.
+    ///
+    /// Naming only `Frame` here makes an `Ebp` operand look like a register to
+    /// the parallel move, which then skips the scratch register and hands the
+    /// backend a memory-to-memory move no machine encodes.
     pub fn is_stack(&self) -> bool {
-        matches!(self, Loc::Frame(_))
+        matches!(self, Loc::Frame(_) | Loc::Ebp(_))
     }
     pub fn is_immed(&self) -> bool {
         matches!(self, Loc::Immed(_))

majit/majit-backend-dynasm/src/x86/assembler.rs

@@ -9,6 +9,7 @@
 ///   _assemble — assembler.py:779 (walk ops + emit code)
 ///   patch_jump_for_descr — assembler.py:965
 ///   redirect_call_assembler — assembler.py:1138
+use crate::regloc::ebp_loc_pat;
 use indexmap::IndexMap;
 use std::sync::Arc;
 
@@ -8827,16 +8828,16 @@ impl<'a> crate::jump::RegallocMoves for Assembler386<'a> {
                     dynasm!(self.mc ; .arch x64 ; movq Rx(d.value), Rq(s.value));
                 }
             }
-            (Loc::Reg(s), Loc::Frame(f)) => {
-                let ofs = f.ebp_loc.value;
+            (Loc::Reg(s), ebp_loc_pat!(e)) => {
+                let ofs = e.value;
                 if s.is_xmm {
                     dynasm!(self.mc ; .arch x64 ; movsd [rbp + ofs], Rx(s.value));
                 } else {
                     dynasm!(self.mc ; .arch x64 ; mov [rbp + ofs], Rq(s.value));
                 }
             }
-            (Loc::Frame(f), Loc::Reg(d)) => {
-                let ofs = f.ebp_loc.value;
+            (ebp_loc_pat!(e), Loc::Reg(d)) => {
+                let ofs = e.value;
                 if d.is_xmm {
                     dynasm!(self.mc ; .arch x64 ; movsd Rx(d.value), [rbp + ofs]);
                 } else {
@@ -8854,25 +8855,29 @@ impl<'a> crate::jump::RegallocMoves for Assembler386<'a> {
                     dynasm!(self.mc ; .arch x64 ; mov Rq(d.value), QWORD i.value);
                 }
             }
-            (Loc::Immed(i), Loc::Frame(f)) => {
-                let ofs = f.ebp_loc.value;
+            (Loc::Immed(i), ebp_loc_pat!(e)) => {
+                let ofs = e.value;
                 let scratch = crate::regloc::X86_64_SCRATCH_REG.value;
                 dynasm!(self.mc ; .arch x64
                     ; mov Rq(scratch), QWORD i.value
                     ; mov [rbp + ofs], Rq(scratch)
                 );
             }
-            (Loc::Frame(f1), Loc::Frame(f2)) if f1.position == f2.position => {}
-            (Loc::Frame(f1), Loc::Frame(f2)) => {
-                let o1 = f1.ebp_loc.value;
-                let o2 = f2.ebp_loc.value;
+            (ebp_loc_pat!(e1), ebp_loc_pat!(e2)) if e1.value == e2.value => {}
+            (ebp_loc_pat!(e1), ebp_loc_pat!(e2)) => {
+                let o1 = e1.value;
+                let o2 = e2.value;
                 let scratch = crate::regloc::X86_64_SCRATCH_REG.value;
                 dynasm!(self.mc ; .arch x64
                     ; mov Rq(scratch), [rbp + o1]
                     ; mov [rbp + o2], Rq(scratch)
                 );
             }
-            _ => {}
+            _ => panic!(
+                "parallel move {src:?} -> {dst:?} is outside the RegallocMoves \
+                 operand contract; emitting nothing here would leave the \
+                 destination stale",
+            ),
         }
     }
 
@@ -8884,13 +8889,16 @@ impl<'a> crate::jump::RegallocMoves for Assembler386<'a> {
             Loc::Reg(r) => {
                 dynasm!(self.mc ; .arch x64 ; push Rq(r.value));
             }
-            Loc::Frame(f) if f.ebp_loc.is_float => {
-                dynasm!(self.mc ; .arch x64 ; sub rsp, 8 ; movsd xmm15, [rbp + f.ebp_loc.value] ; movsd [rsp], xmm15);
+            ebp_loc_pat!(e) if e.is_float => {
+                dynasm!(self.mc ; .arch x64 ; sub rsp, 8 ; movsd xmm15, [rbp + e.value] ; movsd [rsp], xmm15);
             }
-            Loc::Frame(f) => {
-                dynasm!(self.mc ; .arch x64 ; push QWORD [rbp + f.ebp_loc.value]);
+            ebp_loc_pat!(e) => {
+                dynasm!(self.mc ; .arch x64 ; push QWORD [rbp + e.value]);
             }
-            _ => {}
+            _ => panic!(
+                "parallel move cannot park {loc:?} on the stack; emitting nothing \
+                 here would leave the matching pop unbalanced",
+            ),
         }
     }
 
@@ -8902,13 +8910,16 @@ impl<'a> crate::jump::RegallocMoves for Assembler386<'a> {
             Loc::Reg(r) => {
                 dynasm!(self.mc ; .arch x64 ; pop Rq(r.value));
             }
-            Loc::Frame(f) if f.ebp_loc.is_float => {
-                dynasm!(self.mc ; .arch x64 ; movsd xmm15, [rsp] ; add rsp, 8 ; movsd [rbp + f.ebp_loc.value], xmm15);
+            ebp_loc_pat!(e) if e.is_float => {
+                dynasm!(self.mc ; .arch x64 ; movsd xmm15, [rsp] ; add rsp, 8 ; movsd [rbp + e.value], xmm15);
             }
-            Loc::Frame(f) => {
-                dynasm!(self.mc ; .arch x64 ; pop QWORD [rbp + f.ebp_loc.value]);
+            ebp_loc_pat!(e) => {
+                dynasm!(self.mc ; .arch x64 ; pop QWORD [rbp + e.value]);
             }
-            _ => {}
+            _ => panic!(
+                "parallel move cannot restore {loc:?} from the stack; emitting \
+                 nothing here would leave the stack pointer shifted",
+            ),
         }
     }
 }