From 860f0b04326b9e3897e198eb0aeec7c7bfb4d866 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 15 Aug 2026 16:11:29 +0900 Subject: [PATCH 1/6] Witness the undeclared field's width and describe an array field as a pointer `field_scalar_tokens`' undeclared arm registers a field as eight bytes and emits no check for it, so a `u32` field with no `int_fields` entry compiles clean and registers a descr four bytes too wide. It now emits a const witness comparing the field type's width against `size_of::()`, which admits a `#[repr(transparent)]` newtype over `i64` where a type-identity check would not. The write-set rebuild in `lower_stmt.rs` asked only `ref_fields` whether a member is a pointer. A field declared in `array_fields` is a pointer to a buffer, and the array base read registers it as one, so the two producers described the same member with opposite kinds; `get_field_descr` is cache-or-mint, so which description the shared `(struct, fieldname)` slot ends up holding depends on emit order. The write-set path now takes the array declaration's pointer shape and its element-type witness. Adds `jit_interp_array_field_write_kind.rs`, whose fixture reaches the field only through the write-set declaration so the cache slot has one producer. Assisted-by: Claude --- .../jit_interp/jitcode_lower/lower_stmt.rs | 33 +++- .../jit_interp/jitcode_lower/lower_vable.rs | 48 ++++- .../jit_interp_array_field_write_kind.rs | 179 ++++++++++++++++++ 3 files changed, 255 insertions(+), 5 deletions(-) create mode 100644 majit/majit-metainterp/tests/jit_interp_array_field_write_kind.rs diff --git a/majit/majit-macros/src/jit_interp/jitcode_lower/lower_stmt.rs b/majit/majit-macros/src/jit_interp/jitcode_lower/lower_stmt.rs index 2f6c4eb2a57..3ab3a1fa8c2 100644 --- a/majit/majit-macros/src/jit_interp/jitcode_lower/lower_stmt.rs +++ b/majit/majit-macros/src/jit_interp/jitcode_lower/lower_stmt.rs @@ -82,13 +82,40 @@ impl<'c> Lowerer<'c> { layouts.push((path, Vec::new())); &mut layouts.last_mut().unwrap().1 }; - let is_ref = config.ref_fields.contains_key(&key); // Same declared width the getfield/setfield lowering registers, so // this write-EI rebuild mints the field descr the reads resolve to // rather than a machine-word twin of it. + // + // A pointer field stays a pointer field whichever map declares it. + // `ref_fields` names the pointee of a single-object link; an array + // declaration names the element type of a buffer the field points + // at. Both read back through `getfield_gc_r`, and the array base's + // own layout registration describes the field as a pointer word. + // Asking only `ref_fields` here therefore describes the SAME member + // as an eight-byte signed integer at a second producer, and + // `get_field_descr` resolves that disagreement by keeping whichever + // producer reached its `(struct, fieldname)` slot first — so the + // read's descr and the write set's are the same object only by + // registration order. let member = syn::Member::Named(field.clone()); - let (__fsize, __fsigned, __fcheck) = - super::lower_vable::field_scalar_tokens(config, &key, path, &member); + let (is_ref, __fsize, __fsigned, __fcheck) = match config.array_fields.get(&key) { + Some((_, _, element_path)) => ( + true, + quote! { ::core::mem::size_of::() }, + quote! { false }, + // The array base's witness, for the same reason it gives: + // naming the pointee accepts `*const T` as well as `*mut T`. + quote! { + const _: fn(&#path) -> #element_path = + |__s| unsafe { *__s.#field }; + }, + ), + None => { + let (size, signed, check) = + super::lower_vable::field_scalar_tokens(config, &key, path, &member); + (config.ref_fields.contains_key(&key), size, signed, check) + } + }; fields.push(quote! { { #__fcheck diff --git a/majit/majit-macros/src/jit_interp/jitcode_lower/lower_vable.rs b/majit/majit-macros/src/jit_interp/jitcode_lower/lower_vable.rs index 359addcb85e..1b9d95eae37 100644 --- a/majit/majit-macros/src/jit_interp/jitcode_lower/lower_vable.rs +++ b/majit/majit-macros/src/jit_interp/jitcode_lower/lower_vable.rs @@ -6,7 +6,8 @@ use super::*; /// struct. `descr.py:218-239 get_field_descr` derives both from `FIELDTYPE`, /// but the macro sees only the field's name at the access site, so a sub-word /// integer field has to be named in `int_fields` to be registered as one. -/// Anything undeclared keeps the machine-word default. +/// Anything undeclared keeps the machine-word default — and has to be eight +/// bytes wide to keep it, which the returned witness enforces. pub(super) fn field_scalar_tokens( config: &LowererConfig, key: &str, @@ -42,10 +43,53 @@ pub(super) fn field_scalar_tokens( // `scalar_size`'s own default for a non-`Ref` field: the `i64` storage, // which is 8 on every target and NOT the machine word (a `usize` here // would report 4 on wasm32). + // + // That default is a CLAIM about the field — eight bytes — and it used to + // be the one claim here that carried no witness. The declared arm above + // cannot drift from the struct; the undeclared arm could say eight bytes + // about a `u32` and register a descr four bytes too wide, with the + // sub-word range a declaration exists to buy silently gone. Omission is + // the dangerous direction precisely because it is spelled as nothing at + // all, so the default witnesses itself too. + // + // WIDTH, not type identity, because width is what the claim is about. + // The storage this registers is reached as a raw eight-byte word, and a + // `#[repr(transparent)]` newtype over `i64` — a tagged value word is the + // usual one — is exactly that word. Demanding the field spell `i64` + // would reject those for no defect. What stays uncheckable either way + // is the sign, which no Rust type-level test can recover from a field + // the macro only knows by name. None => ( quote! { ::core::mem::size_of::() }, quote! { true }, - ref_field_witness_tokens(&config.ref_fields, key, struct_path, member), + match ref_field_witness_tokens(&config.ref_fields, key, struct_path, member) { + // A ref field is a pointer word and carries its own witness. + witness if !witness.is_empty() => witness, + _ => { + let message = format!( + "`{key}` is not the eight-byte scalar an undeclared field is \ + registered as. Name its Rust integer type in `int_fields`, or its \ + pointee in `ref_fields` / its element type in `array_fields`, so \ + the emitted descr reports the field's own width instead of this \ + default.", + ); + quote! { + const _: () = { + // Names the field's type without spelling it, which + // is the only handle available here: the macro sees + // the member, not its declaration. + const fn __field_width(_: fn(&#struct_path) -> T) -> usize { + ::core::mem::size_of::() + } + assert!( + __field_width(|__s: &#struct_path| __s.#member) + == ::core::mem::size_of::(), + #message, + ); + }; + } + } + }, ), } } diff --git a/majit/majit-metainterp/tests/jit_interp_array_field_write_kind.rs b/majit/majit-metainterp/tests/jit_interp_array_field_write_kind.rs new file mode 100644 index 00000000000..4baf8d10dd2 --- /dev/null +++ b/majit/majit-metainterp/tests/jit_interp_array_field_write_kind.rs @@ -0,0 +1,179 @@ +//! An array field is a POINTER field, and both producers that describe it have +//! to say so. +//! +//! Two places in the lowering register a description of the same member. The +//! array base read (`emit_array_field_base`) registers it as a pointer word: +//! the field holds a buffer base, and the read is a `getfield_gc_r`. A +//! `residual_writes = { . => [..] }` declaration — the FIELD form, +//! no `[]` — rebuilds the same layout to mint the write set's descr. +//! +//! `get_field_descr` caches by `(struct, fieldname)` and is cache-OR-mint: the +//! first producer to reach a slot wins, and a later one describing the field +//! differently gets the cached descr back with only a collision counter to show +//! for it. So a write-set producer that asked the wrong map about the field's +//! kind does not fail; it either loses silently, or wins and leaves every +//! `getfield_gc_r` of that base carrying an Int-typed field descr. Which of +//! those happens is decided by the order the two producers were emitted in. +//! +//! This fixture pins the write-set producer alone. Nothing here reads +//! `sel.data[i]`, so the array base producer never runs and the cache slot for +//! `PointerFieldStack::data` holds exactly what the write-set path decided — +//! order cannot mask the answer. + +use majit_metainterp::{Assembler, JitDriver}; + +/// Reached only through the write-set declaration below, so its cache slot has +/// exactly one producer. A struct any other fixture also touched would let the +/// other producer mint the slot and this test would pass without meaning it. +#[repr(C)] +struct PointerFieldStack { + data: *mut i64, + size: usize, +} + +/// An opaque in-place mutator. The body is irrelevant — the JIT never looks +/// inside a residual — but it has to exist for the concrete path. +extern "C" fn jit_scramble_pointer_field(stack: usize) { + let stack = stack as *mut PointerFieldStack; + if stack.is_null() { + return; + } + unsafe { + (*stack).size = (*stack).size; + } +} + +pub type Bytecode = [u8]; + +const OP_NOP: u8 = 0; +const OP_SCRAMBLE: u8 = 1; + +struct PointerFieldState { + a: i64, + sel: usize, +} + +#[majit_macros::jit_interp( + state = PointerFieldState, + env = Bytecode, + state_fields = { a: int, sel: ref(PointerFieldStack) }, + greens = [], + array_fields = { PointerFieldStack::data => i64 }, + calls = { jit_scramble_pointer_field => residual_void }, + residual_writes = { + sel.data => [jit_scramble_pointer_field], + }, +)] +#[allow(unused_assignments, unused_variables)] +fn pointer_field_only(program: &Bytecode, threshold: u32) -> i64 { + let mut driver: JitDriver = JitDriver::new(threshold); + let mut pc: usize = 0; + let state = PointerFieldState { a: 0, sel: 0 }; + { + use majit_metainterp::JitState as _; + state + .build_meta(0, program) + .install_canonical_liveness(&mut driver); + } + while pc < program.len() { + jit_merge_point!(); + let opcode = program[pc]; + pc += 1; + match opcode { + OP_NOP => {} + OP_SCRAMBLE => jit_scramble_pointer_field(state.sel), + _ => break, + } + } + state.a +} + +fn build() -> majit_metainterp::JitCode { + let mut asm = Assembler::new(); + asm.set_canonical_liveness_triple(vec![0], vec![0], vec![]); + __prebuild_jitcode_liveness_pointer_field_only(&mut asm); + let _ = asm.ensure_canonical_liveness_offset(); + __dispatch_jitcode_pointer_field_only(&mut asm, 0i64) + .expect("dispatch lower must succeed for the pointer-field fixture") +} + +/// `(field_type, field_size, is_signed)` the cache holds for `data`, or `None` +/// if nothing registered it. +fn cached_data_field() -> Option<(majit_ir::Type, usize, bool)> { + use majit_ir::descr::FieldDescr as _; + let type_id = majit_metainterp::__pyre_struct_type_id::(false); + let cache = majit_ir::descr::gc_cache().lock().unwrap(); + let descr = cache + ._cache_field + .get(&majit_ir::descr::LLType::Struct(type_id)) + .and_then(|fields| fields.get("data")) + .cloned()?; + Some(( + descr.field_type(), + descr.field_size(), + descr.is_field_signed(), + )) +} + +/// The subject: a write-set declaration naming an array field describes a +/// pointer. +/// +/// Before this was fixed the write-set path asked only `ref_fields` whether the +/// member is a pointer. An array field is declared in `array_fields`, so the +/// answer was no and the field went into the write set as an eight-byte SIGNED +/// INTEGER — the undeclared-scalar default — for a member holding `*mut i64`. +#[test] +fn a_write_set_declaration_over_an_array_field_describes_a_pointer() { + let _jc = build(); + let (field_type, field_size, is_signed) = cached_data_field() + .expect("the fixture's `residual_writes` declaration must register `data`"); + assert_eq!( + field_type, + majit_ir::Type::Ref, + "`sel.data` names the buffer BASE POINTER, so the write set's descr must \ + be a Ref; an Int here is the undeclared-scalar default leaking into a \ + pointer field, and `get_field_descr` will hand that descr to every \ + `getfield_gc_r` of the same base that reaches the slot after it", + ); + assert_eq!( + field_size, + std::mem::size_of::(), + "a pointer field is one target word wide, not the eight bytes the \ + integer default claims (they differ on wasm32)", + ); + assert!( + !is_signed, + "a pointer is not a signed integer; the sign flag is part of what \ + `describes_same_field` compares when a second producer arrives", + ); +} + +/// The control, without which the assertion above cannot fail for the reason it +/// names: a non-pointer field of the same struct must still be described as the +/// scalar it is. +/// +/// A fix that made every write-set field a Ref would satisfy the subject and be +/// exactly as wrong in the other direction. +#[test] +fn a_scalar_field_of_the_same_struct_is_still_a_scalar() { + use majit_ir::descr::FieldDescr as _; + let _jc = build(); + let type_id = majit_metainterp::__pyre_struct_type_id::(false); + let cache = majit_ir::descr::gc_cache().lock().unwrap(); + let size_field = cache + ._cache_field + .get(&majit_ir::descr::LLType::Struct(type_id)) + .and_then(|fields| fields.get("size")) + .cloned(); + // `size` is not named by any declaration or access here, so the control is + // only meaningful if something registered it. Skipping when nothing did is + // honest; asserting on an absent slot would pass for the wrong reason. + if let Some(descr) = size_field { + assert_eq!( + descr.field_type(), + majit_ir::Type::Int, + "`size` is a scalar; only the field a pointer declaration names may \ + become a Ref", + ); + } +} From 3ce19d61e2b11b0ec57845247b6d58b62bec6b55 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 15 Aug 2026 16:45:10 +0900 Subject: [PATCH 2/6] Note that the unconsulted-declaration gate covers one jit_interp machine `assert_no_unconsulted_field_declarations` is keyed on the dispatch-arm census, which `#[jit_inline]` helpers do not have. Each helper carries its own int_fields/ref_fields and records under its own name, so the population the gate cannot see is usually the larger one. Assisted-by: Claude --- majit/majit-metainterp/src/lib.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/majit/majit-metainterp/src/lib.rs b/majit/majit-metainterp/src/lib.rs index 8cc3857e39e..b8e6e79d9cf 100644 --- a/majit/majit-metainterp/src/lib.rs +++ b/majit/majit-metainterp/src/lib.rs @@ -672,6 +672,16 @@ pub fn unconsulted_field_declarations() -> Vec { /// Takes its denominator from the same census as /// [`assert_no_degraded_dispatch_arms`], for the same reason: an empty result /// is also what a portal that was never built produces. +/// +/// ⚠ Covers ONE `#[jit_interp]` machine, which is usually the smaller half of a +/// consumer's population. Each `#[jit_inline]` helper carries its own +/// `int_fields` / `ref_fields` and records under its own name, so a helper +/// repeating a declaration at every site turns one unconsulted key into dozens +/// — and none of them have a dispatch-arm census to key this assertion on. A +/// caller that wants the whole picture has to filter +/// [`unconsulted_field_declarations`] itself and supply its own denominator; +/// measured on one consumer, five of six survivors were on helpers, so a +/// portal-only assertion reported the sixth and called the crate clean. pub fn assert_no_unconsulted_field_declarations(interp: &str) { let census = dispatch_arm_census(); let Some(entry) = census.iter().find(|e| e.interp == interp) else { From 934057e899f1772c580769cc1af03f53b35fb1a5 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 15 Aug 2026 20:43:42 +0900 Subject: [PATCH 3/6] Name the actual cause when the unroll optimizer is skipped `compile_loop` suppresses unrolling on either of two conditions: the `PYRE_NO_UNROLL` env override, or `unroll` being absent from the jitdriver's `enable_opts`. Both emitted the same log line naming the env variable and raised `InvalidLoop("PYRE_NO_UNROLL")`. Add `unroll_skip_reason`, which returns which condition fired, and use its string for both the log line and the `InvalidLoop` payload. The suppressed case also logged "abort trace" while the trace was retried and compiled as a simple loop, leaving the log in disagreement with the `Traces aborted` counter; it now says the unroll was suppressed and the trace is being compiled as a simple loop. Assisted-by: Claude --- majit/majit-metainterp/src/lib.rs | 22 +++++ majit/majit-metainterp/src/pyjitpl.rs | 44 +++++---- .../tests/unroll_skip_reason.rs | 93 +++++++++++++++++++ 3 files changed, 141 insertions(+), 18 deletions(-) create mode 100644 majit/majit-metainterp/tests/unroll_skip_reason.rs diff --git a/majit/majit-metainterp/src/lib.rs b/majit/majit-metainterp/src/lib.rs index b8e6e79d9cf..1ae48eb2c7d 100644 --- a/majit/majit-metainterp/src/lib.rs +++ b/majit/majit-metainterp/src/lib.rs @@ -314,6 +314,28 @@ pub fn no_unroll_enabled() -> bool { *FLAG.get_or_init(|| std::env::var_os("PYRE_NO_UNROLL").is_some()) } +/// Why the unroll optimizer will be skipped, or `None` when it runs. +/// +/// Two disjoint conditions suppress unrolling, and which one fired is the whole +/// content of the answer for anyone reading a log. The env override is a +/// process-wide switch a reader can check from their shell; the `enable_opts` +/// omission is one jitdriver's own configuration, invisible from the +/// environment and typically a deliberate, documented choice by that frontend. +/// +/// Reporting the env override's name for both is worse than reporting nothing. +/// A frontend that left `unroll` out of its `enable_opts` produces a log naming +/// `PYRE_NO_UNROLL`; the reader checks their environment, finds it unset, and +/// has been sent to look for a cause that does not exist. +pub fn unroll_skip_reason(env_override: bool, enable_opts: &[String]) -> Option<&'static str> { + if env_override { + Some("PYRE_NO_UNROLL env override") + } else if !enable_opts.iter().any(|opt| opt == "unroll") { + Some("`unroll` is absent from this jitdriver's enable_opts") + } else { + None + } +} + pub fn stall_window() -> u64 { static VAL: std::sync::LazyLock = std::sync::LazyLock::new(|| { std::env::var("MAJIT_STALL_WINDOW") diff --git a/majit/majit-metainterp/src/pyjitpl.rs b/majit/majit-metainterp/src/pyjitpl.rs index 1fc9d6dde69..82316cc4861 100644 --- a/majit/majit-metainterp/src/pyjitpl.rs +++ b/majit/majit-metainterp/src/pyjitpl.rs @@ -6357,12 +6357,11 @@ impl MetaInterp { // PyPy: pyjitpl.py:3016-3017 gates unrolling on `unroll` in // warmstate.enable_opts. PYRE_NO_UNROLL remains a diagnostic override. - let no_unroll = crate::no_unroll_enabled() - || !self - .warm_state - .get_enable_opts() - .iter() - .any(|opt| opt == "unroll"); + let no_unroll_reason = crate::unroll_skip_reason( + crate::no_unroll_enabled(), + self.warm_state.get_enable_opts(), + ); + let no_unroll = no_unroll_reason.is_some(); // Use UnrollOptimizer for preamble peeling when available. // compile.py: compile_loop → PreambleCompileData + LoopCompileData. @@ -6462,14 +6461,11 @@ impl MetaInterp { crate::optimizeopt::unroll::ExportedState, )> = None; let optimize_start = Instant::now(); - let optimize_result = if no_unroll { + let optimize_result = if let Some(reason) = no_unroll_reason { if crate::majit_log_enabled() { - eprintln!( - "[jit] PYRE_NO_UNROLL: skipping unroll optimizer at key={}", - green_key, - ); + eprintln!("[jit] skipping unroll optimizer at key={green_key} ({reason})"); } - Err(crate::optimize::InvalidLoop("PYRE_NO_UNROLL")) + Err(crate::optimize::InvalidLoop(reason)) } else { unroll_opt.optimize_trace_with_constants_and_inputs_vable_out( &trace_ops, @@ -6494,13 +6490,25 @@ impl MetaInterp { let reason = invalid_loop.0; { if crate::majit_log_enabled() { - eprintln!( - "[jit] abort trace at key={} (InvalidLoop: {})", - green_key, reason, - ); + // A suppressed unroll reaches here through the same + // `Err` as a real one, but nothing was abandoned: the + // retry below compiles the simple loop and the trace + // is not counted as aborted. Saying "abort trace" for + // it puts the log and `Traces aborted` in open + // disagreement, with the log the one that is wrong. + if no_unroll { + eprintln!( + "[jit] unroll suppressed at key={green_key} ({reason}); \ + compiling the trace as a simple loop", + ); + } else { + eprintln!( + "[jit] abort trace at key={green_key} (InvalidLoop: {reason})", + ); + } } - // When PYRE_NO_UNROLL is set, the InvalidLoop is synthetic - // — skip the cancel_count gate and go straight to the + // A suppressed unroll makes the InvalidLoop synthetic — + // skip the cancel_count gate and go straight to the // simple-loop retry path. if !no_unroll { self.cancel_count += 1; diff --git a/majit/majit-metainterp/tests/unroll_skip_reason.rs b/majit/majit-metainterp/tests/unroll_skip_reason.rs new file mode 100644 index 00000000000..334fd89da0f --- /dev/null +++ b/majit/majit-metainterp/tests/unroll_skip_reason.rs @@ -0,0 +1,93 @@ +//! A suppressed unroll has two possible causes, and the log has to say which. +//! +//! `compile_loop` skips the unroll optimizer when either the `PYRE_NO_UNROLL` +//! env override is set OR the jitdriver's `enable_opts` does not list `unroll`. +//! Both paths used to emit the same line — `[jit] PYRE_NO_UNROLL: skipping +//! unroll optimizer` — and raise the same `InvalidLoop("PYRE_NO_UNROLL")`. +//! +//! That is not a cosmetic difference. A frontend that deliberately leaves +//! `unroll` out of its `enable_opts` (measuring that peeling the preamble costs +//! it more than it returns is a normal outcome for a loop body with nothing +//! loop-invariant to hoist) gets a log naming an environment variable. A reader +//! checks their shell, finds it unset, and has been sent after a cause that +//! does not exist — the real one is a line of the frontend's own setup. + +use majit_metainterp::unroll_skip_reason; + +fn opts(list: &[&str]) -> Vec { + list.iter().map(|s| (*s).to_string()).collect() +} + +/// The full option list, as a frontend that wants unrolling would pass it. +const ALL_OPTS: &[&str] = &[ + "intbounds", + "rewrite", + "virtualize", + "string", + "pure", + "earlyforce", + "heap", + "unroll", +]; + +#[test] +fn each_cause_is_named_as_itself() { + let with_unroll = opts(ALL_OPTS); + let reason = unroll_skip_reason(true, &with_unroll) + .expect("the env override suppresses unrolling whatever the opts say"); + assert!( + reason.contains("PYRE_NO_UNROLL"), + "the env override must name the variable a reader can check; got {reason:?}", + ); + + // `ALL_OPTS` minus `unroll` — the shape a frontend that opted out passes. + let without_unroll = opts(&ALL_OPTS[..ALL_OPTS.len() - 1]); + let reason = unroll_skip_reason(false, &without_unroll) + .expect("an enable_opts list without `unroll` suppresses unrolling"); + assert!( + reason.contains("enable_opts"), + "the configuration cause must name the configuration, not the env \ + variable a reader would then look for in vain; got {reason:?}", + ); + assert!( + !reason.contains("PYRE_NO_UNROLL"), + "…and must not name the env override at all: it is unset in exactly \ + this case, which is what made the old message misleading; got {reason:?}", + ); +} + +/// The control. Without it every assertion above is satisfied by a function +/// that suppresses unrolling unconditionally, which would be a far worse defect +/// than the message it was meant to fix. +#[test] +fn a_driver_that_asked_for_unrolling_gets_it() { + assert_eq!( + unroll_skip_reason(false, &opts(ALL_OPTS)), + None, + "`unroll` is listed and the env override is unset, so nothing suppresses it", + ); +} + +/// An empty list is the absent-configuration case, not a request for unrolling. +/// +/// Worth pinning separately: "no opts were configured" and "opts were +/// configured and `unroll` was left out" arrive here as the same value, and +/// both must suppress. A predicate that treated empty as "unrestricted" would +/// unroll for a driver that never asked. +#[test] +fn an_empty_option_list_suppresses_rather_than_permits() { + assert!( + unroll_skip_reason(false, &[]).is_some(), + "an empty enable_opts does not list `unroll`, so unrolling stays off", + ); +} + +/// Substring matching would accept a longer option that merely contains the +/// name. The comparison is on whole entries and this is what says so. +#[test] +fn a_different_option_containing_the_name_does_not_enable_unrolling() { + assert!( + unroll_skip_reason(false, &opts(&["unroll_safe", "heap"])).is_some(), + "only an exact `unroll` entry enables unrolling", + ); +} From 19f1b752cf2e18963bf504bbccbb2c7434335c25 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 15 Aug 2026 21:17:57 +0900 Subject: [PATCH 4/6] Name the JitCodes the jit_interp/jit_inline macros build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `JitCodeBuilder` defaults `name` to the empty string and no macro emit site called `set_name`, so every macro-built JitCode reported `""`. The bytecode encoder's register/const ceiling audit prints that field, and `log_bytecode_abort` discriminated the two `BC_ABORT` emitter families by frame shape because the name was unavailable. Call `set_name` at the three emit sites: an inline helper's JitCode takes its `#[jit_inline]` function's name, a machine's dispatch JitCode takes the machine function's name, and each arm sub-JitCode takes `::` — the spelling `record_degraded_dispatch_arm` already uses for the same arm. `log_bytecode_abort` now prints the name alongside the frame shape. Assisted-by: Claude --- .../src/jit_interp/codegen_trace.rs | 5 + .../src/jit_interp/jitcode_lower/dispatch.rs | 33 +++- majit/majit-macros/src/lib.rs | 8 + .../majit-metainterp/src/pyjitpl/dispatch.rs | 15 +- majit/majit-metainterp/tests/jitcode_names.rs | 147 ++++++++++++++++++ 5 files changed, 200 insertions(+), 8 deletions(-) create mode 100644 majit/majit-metainterp/tests/jitcode_names.rs diff --git a/majit/majit-macros/src/jit_interp/codegen_trace.rs b/majit/majit-macros/src/jit_interp/codegen_trace.rs index ef81468db28..4c3e4661bcf 100644 --- a/majit/majit-macros/src/jit_interp/codegen_trace.rs +++ b/majit/majit-macros/src/jit_interp/codegen_trace.rs @@ -14,6 +14,7 @@ pub fn generate_trace_fn(config: &JitInterpConfig, func: &ItemFn) -> TokenStream let trace_fn_name = format_ident!("__trace_{}", fn_name); let prebuild_fn_name = format_ident!("__prebuild_jitcode_liveness_{}", fn_name); let dispatch_jitcode_fn_name = format_ident!("__dispatch_jitcode_{}", fn_name); + let dispatch_jitcode_name = fn_name.to_string(); let declare_schema_fn_name = format_ident!("__declare_jit_schema_{}", fn_name); // Must match `codegen_state.rs`'s spelling: the symbolic-state struct is one // module-level item shared by both emitters, suffixed so two machines can @@ -248,6 +249,10 @@ pub fn generate_trace_fn(config: &JitInterpConfig, func: &ItemFn) -> TokenStream return None; } let mut __builder = majit_metainterp::JitCodeBuilder::new(); + // `jitcode.py:15 self.name = name`. This is the root JitCode of + // the machine, so it names the dispatch function itself; its arms' + // sub-JitCodes name the arm they came from. + __builder.set_name(#dispatch_jitcode_name); let _live_offset_patch = __builder.live_placeholder(); #dispatch_body __builder.finalize_liveness(__asm); diff --git a/majit/majit-macros/src/jit_interp/jitcode_lower/dispatch.rs b/majit/majit-macros/src/jit_interp/jitcode_lower/dispatch.rs index a7849adf19b..c8c7fde963c 100644 --- a/majit/majit-macros/src/jit_interp/jitcode_lower/dispatch.rs +++ b/majit/majit-macros/src/jit_interp/jitcode_lower/dispatch.rs @@ -2725,6 +2725,18 @@ pub(super) fn lower_dispatch_chain( majit_metainterp::record_degraded_dispatch_arm(#interp, #arm_name, #reason); } }; + // `jitcode.py:15 self.name = name` — the name every sub-JitCode built + // for this arm carries. The same spelling the degraded-arm record + // uses, so a log line and a `degraded_dispatch_arms()` entry for one + // arm can be matched by eye. + // + // Every arm gets it, not just the ones that degrade: `BC_ABORT` has + // two emitter families and one opcode, and the abort log discriminates + // them by frame shape (`depth>1 body=1`) precisely because the name was + // empty. A name only the abort stubs carried would still leave the + // healthy arms indistinguishable from each other in the bytecode + // encoder's ceiling audit, which prints one line per finished builder. + let sub_jitcode_name = format!("{degraded_interp_name}::{degraded_arm_name}"); if std::env::var_os("MAJIT_MACRO_DEBUG").is_some() { let pat = &arm.pat; @@ -2867,6 +2879,7 @@ pub(super) fn lower_dispatch_chain( let __arm_jc: Option = (|| -> Option { let mut __sub_builder = majit_metainterp::JitCodeBuilder::new(); + __sub_builder.set_name(#sub_jitcode_name); __sub_builder.ensure_i_regs(#min_i_regs); __sub_builder.ensure_r_regs(#min_r_regs); __sub_builder.ensure_f_regs(#min_f_regs); @@ -2887,6 +2900,7 @@ pub(super) fn lower_dispatch_chain( // BC_ABORT. #record_degraded let mut __sub_builder = majit_metainterp::JitCodeBuilder::new(); + __sub_builder.set_name(#sub_jitcode_name); __sub_builder.ensure_i_regs(#min_i_regs); __sub_builder.ensure_r_regs(#min_r_regs); __sub_builder.ensure_f_regs(#min_f_regs); @@ -2909,6 +2923,7 @@ pub(super) fn lower_dispatch_chain( { #record_degraded let mut __sub_builder = majit_metainterp::JitCodeBuilder::new(); + __sub_builder.set_name(#sub_jitcode_name); __sub_builder.abort(); __sub_builder.finish() } @@ -2927,17 +2942,30 @@ pub(super) fn lower_dispatch_chain( // make the channel fire on every `_ => break` and // `_ => panic!` in the corpus and stop discriminating. crate::jit_interp::classify::ArmPattern::Nop => ( - quote::quote! { majit_metainterp::JitCodeBuilder::new().finish() }, + quote::quote! { + { + let mut __sub_builder = majit_metainterp::JitCodeBuilder::new(); + __sub_builder.set_name(#sub_jitcode_name); + __sub_builder.finish() + } + }, dispatch_arm_inline_call_tokens(&[]), ), crate::jit_interp::classify::ArmPattern::Halt => ( - quote::quote! { majit_metainterp::JitCodeBuilder::new().finish() }, + quote::quote! { + { + let mut __sub_builder = majit_metainterp::JitCodeBuilder::new(); + __sub_builder.set_name(#sub_jitcode_name); + __sub_builder.finish() + } + }, dispatch_arm_inline_call_tokens(&[]), ), crate::jit_interp::classify::ArmPattern::AbortPermanent => ( quote::quote! { { let mut __sub_builder = majit_metainterp::JitCodeBuilder::new(); + __sub_builder.set_name(#sub_jitcode_name); __sub_builder.abort_permanent(); __sub_builder.finish() } @@ -2954,6 +2982,7 @@ pub(super) fn lower_dispatch_chain( { #record_degraded let mut __sub_builder = majit_metainterp::JitCodeBuilder::new(); + __sub_builder.set_name(#sub_jitcode_name); __sub_builder.abort(); __sub_builder.finish() } diff --git a/majit/majit-macros/src/lib.rs b/majit/majit-macros/src/lib.rs index ffb0701d461..b9d243a9951 100644 --- a/majit/majit-macros/src/lib.rs +++ b/majit/majit-macros/src/lib.rs @@ -2528,6 +2528,7 @@ pub fn jit_inline(attr: TokenStream, item: TokenStream) -> TokenStream { let helper_with_asm_name = format_ident!("__majit_inline_jitcode_{}_with_asm", sig.ident); let helper_prebuild_name = format_ident!("__majit_inline_jitcode_{}_prebuild", sig.ident); let policy_name = format_ident!("__majit_call_policy_{}", sig.ident); + let helper_source_name = sig.ident.to_string(); let helper_body = helper.body; let helper_liveness_prebuild = helper.liveness_prebuild; let return_reg = helper.return_reg; @@ -2596,6 +2597,13 @@ pub fn jit_inline(attr: TokenStream, item: TokenStream) -> TokenStream { __asm: &mut majit_metainterp::Assembler, ) -> majit_metainterp::JitCode { let mut __builder = majit_metainterp::JitCodeBuilder::new(); + // `jitcode.py:15 self.name = name` — every jitcode upstream is + // named at construction. The builder defaults the field to the + // empty string, and the diagnostics that print it (the bytecode + // encoder's register/const ceiling audit among them) then identify + // nothing: a declined helper reports `""` and the reader has no way + // to tell which of a consumer's dozens it was. + __builder.set_name(#helper_source_name); #(#ensure_param_regs)* #helper_body #helper_return diff --git a/majit/majit-metainterp/src/pyjitpl/dispatch.rs b/majit/majit-metainterp/src/pyjitpl/dispatch.rs index 6e55f89162c..132bdd2aa3d 100644 --- a/majit/majit-metainterp/src/pyjitpl/dispatch.rs +++ b/majit/majit-metainterp/src/pyjitpl/dispatch.rs @@ -3346,11 +3346,13 @@ where /// inline frame whose whole body is the single abort byte, so /// `depth>1 body=1` is the stub and the dispatch body is neither. /// - /// `JitCode.name` would be the direct answer and is NOT usable here: - /// `majit_metainterp::JitCodeBuilder::new()` takes no name (unlike - /// `majit-translate`'s), so every macro-built JitCode carries `name: ""`. - /// Threading one through the macro's emit sites is the real fix; until then - /// these three numbers discriminate without it. + /// `JitCode.name` is the direct answer and the macro now supplies it: the + /// arm's sub-JitCode is named `::` and the dispatch + /// body's is the machine function's own name, so the line says which arm + /// aborted instead of leaving it to be inferred. The frame shape stays in + /// the message because it is the independent check — a name is a label the + /// emitter chose, `depth`/`body` is what actually ran — and because a host + /// still on an older builder emits `""` here. fn log_bytecode_abort(&mut self, insn: &str) { if !crate::majit_log_enabled() { return; @@ -3358,9 +3360,10 @@ where let depth = self.frames.len(); let frame = self.frames.current_mut(); eprintln!( - "[jit] {insn} decided at pos={} depth={depth} body={} \ + "[jit] {insn} decided in {:?} at pos={} depth={depth} body={} \ (depth>1 body=1 = a degraded arm stub; the merge point's \ `trace action at pc=` reports the PORTAL pc, not this one)", + frame.jitcode.name(), frame.last_opcode_position, frame.jitcode.code.len(), ); diff --git a/majit/majit-metainterp/tests/jitcode_names.rs b/majit/majit-metainterp/tests/jitcode_names.rs new file mode 100644 index 00000000000..bab03901b54 --- /dev/null +++ b/majit/majit-metainterp/tests/jitcode_names.rs @@ -0,0 +1,147 @@ +//! Every macro-built JitCode carries the name of what it was built from. +//! +//! `jitcode.py:14-15 def __init__(self, name, ...): self.name = name` — upstream +//! has no unnamed JitCode, and the name is what `dump()` and every diagnostic +//! that prints one use to say *which* code they are talking about. +//! +//! `JitCodeBuilder` defaults the field to the empty string, and the macro emit +//! sites never called `set_name`, so the whole macro-built population reported +//! `""`. The cost was not cosmetic: `BC_ABORT` has two emitter families and one +//! opcode, so the abort log had to discriminate them by frame shape +//! (`depth>1 body=1` = a degraded arm stub) — a heuristic standing in for a +//! name that was simply absent. The bytecode encoder's ceiling audit prints one +//! line per finished builder and identified none of them. +//! +//! The names here are diagnostic strings, not identity: nothing keys a cache or +//! a comparison on them. What the tests pin is that each JitCode names its own +//! source, because a shared or empty name is exactly as useless as no name. + +use majit_macros::jit_inline; +use majit_metainterp::{Assembler, JitCode, JitDriver}; + +#[repr(C)] +struct Cell { + value: i64, +} + +#[jit_inline(ref_params = { cell: ref(Cell) })] +fn bump_named_cell(cell: usize) -> i64 { + let value = cell.value; + value + 1 +} + +#[test] +fn an_inline_helper_names_its_source_function() { + let mut asm = Assembler::new(); + let jitcode = __majit_inline_jitcode_bump_named_cell_with_asm(&mut asm); + assert_eq!( + jitcode.name(), + "bump_named_cell", + "an inline helper's JitCode names the `#[jit_inline]` function it was \ + lowered from", + ); +} + +struct NamedState { + a: i64, +} + +const OP_NOP: u8 = 0; +const OP_INC_A: u8 = 1; + +pub type Bytecode = [u8]; + +#[majit_macros::jit_interp( + state = NamedState, + env = Bytecode, + state_fields = { a: int }, + greens = [], +)] +#[allow(unused_assignments, unused_variables)] +fn named_dispatch(program: &Bytecode, threshold: u32) -> i64 { + let mut driver: JitDriver = JitDriver::new(threshold); + let mut pc: usize = 0; + let mut state = NamedState { a: 0 }; + { + use majit_metainterp::JitState as _; + state + .build_meta(0, program) + .install_canonical_liveness(&mut driver); + } + while pc < program.len() { + jit_merge_point!(); + let opcode = program[pc]; + pc += 1; + match opcode { + OP_NOP => {} + OP_INC_A => state.a += 1, + _ => break, + } + } + state.a +} + +fn build_named_dispatch() -> JitCode { + let mut asm = Assembler::new(); + asm.set_canonical_liveness_triple(vec![0], vec![], vec![]); + __prebuild_jitcode_liveness_named_dispatch(&mut asm); + let _ = asm.ensure_canonical_liveness_offset(); + __dispatch_jitcode_named_dispatch(&mut asm, 0i64).expect("dispatch lower must succeed") +} + +#[test] +fn the_dispatch_jitcode_names_the_machine_function() { + assert_eq!( + build_named_dispatch().name(), + "named_dispatch", + "the root JitCode of a `#[jit_interp]` machine names the dispatch \ + function itself", + ); +} + +/// The arm sub-JitCodes, in the order the dispatch registered them. +fn arm_names(dispatch: &JitCode) -> Vec { + dispatch + .exec + .descrs + .iter() + .filter_map(|descr| descr.as_jitcode()) + .map(|sub| sub.name().to_string()) + .collect() +} + +#[test] +fn each_arm_subjitcode_names_the_arm_it_came_from() { + let dispatch = build_named_dispatch(); + let names = arm_names(&dispatch); + assert_eq!( + names.len(), + 2, + "one sub-JitCode per non-default arm; got {names:?}", + ); + + // The interp prefix is what makes a name readable in a log that carries + // more than one machine's traces — `OP_NOP` alone does not say whose. + for name in &names { + assert!( + name.starts_with("NamedState::"), + "an arm sub-JitCode names the state type it dispatches on; got {name:?}", + ); + } + + // The arm's own spelling, and the reason the whole exercise is worth + // anything: two arms of one machine must not answer with the same string. + assert!( + names.iter().any(|n| n.contains("OP_NOP")), + "the nop arm names its own pattern; got {names:?}", + ); + assert!( + names.iter().any(|n| n.contains("OP_INC_A")), + "the lowered arm names its own pattern; got {names:?}", + ); + assert_ne!( + names[0], names[1], + "two arms sharing a name identify nothing, the same as sharing the \ + empty one; got {names:?}", + ); +} From 4dd5de97aa6950f900a64108b0ac48384f7c03a3 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 15 Aug 2026 21:18:07 +0900 Subject: [PATCH 5/6] Move the parallel-move algorithm into the jump module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `jump.rs` held a `remap_frame_layout` that returned an empty move list and was documented as incorrect when the source and destination sets overlap. It had no callers: both backends carried their own copy of the real algorithm as inherent methods, and those two copies — `loc_as_key`, `loc_width`, `remap_frame_layout`, `remap_frame_layout_mixed`, 138 lines — were byte-identical. Move that implementation into `jump.rs` as free functions over a `RegallocMoves` trait carrying the three primitives that do differ per backend (`regalloc_mov`, `regalloc_push`, `regalloc_pop`), and have both backends implement the trait and call the shared functions. The algorithm and the primitives are unchanged. `src_locations` and `dst_locations` keep pyre's order, which is the reverse of `jump.py`'s; the module header says so. Assisted-by: Claude --- .../src/aarch64/assembler.rs | 350 ++++++----------- .../src/aarch64/opassembler.rs | 1 + majit/majit-backend-dynasm/src/jump.rs | 204 +++++++++- majit/majit-backend-dynasm/src/lib.rs | 2 +- .../majit-backend-dynasm/src/x86/assembler.rs | 354 ++++++------------ 5 files changed, 411 insertions(+), 500 deletions(-) diff --git a/majit/majit-backend-dynasm/src/aarch64/assembler.rs b/majit/majit-backend-dynasm/src/aarch64/assembler.rs index 116fee440b3..539904c229b 100644 --- a/majit/majit-backend-dynasm/src/aarch64/assembler.rs +++ b/majit/majit-backend-dynasm/src/aarch64/assembler.rs @@ -27,6 +27,7 @@ use crate::jitframe::{ FIRST_ITEM_OFFSET, JF_DESCR_OFS, JF_FORCE_DESCR_OFS, JF_FRAME_OFS, JF_GCMAP_OFS, JF_GUARD_EXC_OFS, }; +use crate::jump::RegallocMoves; use crate::regalloc::{RegAlloc, RegAllocOp}; use crate::regloc::{Loc, RegLoc}; use crate::runner::GuardGcTypeInfo; @@ -778,246 +779,6 @@ impl<'a> AssemblerARM64<'a> { slot } - // ── Location-aware code emission (RPython regalloc parity) ── - // assembler.py regalloc_mov: move value between any two locations. - - /// assembler.py:1145 regalloc_mov(from_loc, to_loc). - /// Emit a move between any two locations: reg↔reg, reg↔frame, imm→reg, imm→frame. - pub(crate) fn regalloc_mov(&mut self, src: &Loc, dst: &Loc) { - if majit_ir::debug::have_debug_prints() { - majit_ir::debug::log_one("jit-backend", &format!("remap-mov: {src:?} -> {dst:?}")); - } - match (src, dst) { - (Loc::Reg(s), Loc::Reg(d)) if s == d => {} - (Loc::Reg(s), Loc::Reg(d)) => { - if s.is_xmm && d.is_xmm { - dynasm!(self.mc ; .arch aarch64 ; fmov D(d.value), D(s.value)); - } else if !s.is_xmm && !d.is_xmm { - dynasm!(self.mc ; .arch aarch64 ; mov X(d.value), X(s.value)); - } else if s.is_xmm && !d.is_xmm { - dynasm!(self.mc ; .arch aarch64 ; fmov X(d.value), D(s.value)); - } else { - dynasm!(self.mc ; .arch aarch64 ; fmov D(d.value), X(s.value)); - } - } - (Loc::Reg(s), Loc::Frame(f)) => { - let ofs = f.ebp_loc.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; - if d.is_xmm { - self.emit_ldr_fp_d(d.value, ofs); - } else { - self.emit_ldr_fp(d.value, ofs); - } - } - (Loc::Immed(i), Loc::Reg(d)) => { - if d.is_xmm { - self.emit_mov_imm64(16, i.value); // x16 = scratch - dynasm!(self.mc ; .arch aarch64 ; fmov D(d.value), X(16)); - } else { - self.emit_mov_imm64(d.value as u32, i.value); - } - } - (Loc::Immed(i), Loc::Frame(f)) => { - let ofs = f.ebp_loc.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; - self.emit_ldr_fp(16, o1); - self.emit_str_fp(16, o2); - } - _ => {} - } - } - - fn loc_as_key(loc: &Loc) -> i32 { - 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::Immed(_) => i32::MIN, - Loc::Addr(a) => a.offset, - } - } - - fn loc_width(loc: &Loc) -> usize { - match loc { - Loc::Reg(r) => r.get_width(), - Loc::Frame(f) => f.ebp_loc.get_width(), - Loc::Ebp(e) => e.get_width(), - _ => WORD, - } - } - - fn regalloc_push(&mut self, loc: &Loc) { - match loc { - Loc::Reg(r) if r.is_xmm => { - dynasm!(self.mc ; .arch aarch64 ; str D(r.value), [sp, #-16]!); - } - 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); - dynasm!(self.mc ; .arch aarch64 ; str D(15), [sp, #-16]!); - } - Loc::Frame(f) => { - self.emit_ldr_fp(16, f.ebp_loc.value); - dynasm!(self.mc ; .arch aarch64 ; str x16, [sp, #-16]!); - } - _ => {} - } - } - - fn regalloc_pop(&mut self, loc: &Loc) { - match loc { - Loc::Reg(r) if r.is_xmm => { - dynasm!(self.mc ; .arch aarch64 ; ldr D(r.value), [sp], #16); - } - Loc::Reg(r) => { - dynasm!(self.mc ; .arch aarch64 ; ldr X(r.value), [sp], #16); - } - Loc::Frame(f) if f.ebp_loc.is_float => { - dynasm!(self.mc ; .arch aarch64 ; ldr D(15), [sp], #16); - self.emit_str_fp_d(15, f.ebp_loc.value); - } - Loc::Frame(f) => { - dynasm!(self.mc ; .arch aarch64 ; ldr x16, [sp], #16); - self.emit_str_fp(16, f.ebp_loc.value); - } - _ => {} - } - } - - fn remap_frame_layout(&mut self, src_locations: &[Loc], dst_locations: &[Loc], tmpreg: Loc) { - let mut pending_dests = dst_locations.len() as i32; - let mut srccount: IndexMap = IndexMap::new(); - for dst in dst_locations { - srccount.insert(Self::loc_as_key(dst), 0); - } - for i in 0..dst_locations.len() { - let src = src_locations[i]; - if src.is_immed() { - continue; - } - let key = Self::loc_as_key(&src); - if let Some(cnt) = srccount.get_mut(&key) { - if key == Self::loc_as_key(&dst_locations[i]) { - *cnt = -(dst_locations.len() as i32) - 1; - pending_dests -= 1; - } else { - *cnt += 1; - } - } - } - - while pending_dests > 0 { - let mut progress = false; - for i in 0..dst_locations.len() { - let dst = dst_locations[i]; - let key = Self::loc_as_key(&dst); - if srccount.get(&key).copied().unwrap_or(-1) == 0 { - srccount.insert(key, -1); - pending_dests -= 1; - let src = src_locations[i]; - if !src.is_immed() { - let src_key = Self::loc_as_key(&src); - if let Some(cnt) = srccount.get_mut(&src_key) { - *cnt -= 1; - } - } - if dst.is_stack() && src.is_stack() { - self.regalloc_mov(&src, &tmpreg); - self.regalloc_mov(&tmpreg, &dst); - } else { - self.regalloc_mov(&src, &dst); - } - progress = true; - } - } - if !progress { - let mut sources: IndexMap = IndexMap::new(); - for i in 0..dst_locations.len() { - sources.insert(Self::loc_as_key(&dst_locations[i]), src_locations[i]); - } - for dst in dst_locations { - let originalkey = Self::loc_as_key(dst); - if srccount.get(&originalkey).copied().unwrap_or(-1) >= 0 { - self.regalloc_push(dst); - let mut cur_dst = *dst; - loop { - let key = Self::loc_as_key(&cur_dst); - srccount.insert(key, -1); - pending_dests -= 1; - let src = sources[&key]; - if Self::loc_as_key(&src) == originalkey { - break; - } - if cur_dst.is_stack() && src.is_stack() { - self.regalloc_mov(&src, &tmpreg); - self.regalloc_mov(&tmpreg, &cur_dst); - } else { - self.regalloc_mov(&src, &cur_dst); - } - cur_dst = src; - } - self.regalloc_pop(&cur_dst); - } - } - } - } - } - - fn remap_frame_layout_mixed( - &mut self, - src_locations1: &[Loc], - dst_locations1: &[Loc], - tmpreg1: Loc, - src_locations2: &[Loc], - dst_locations2: &[Loc], - tmpreg2: Loc, - ) { - let mut extrapushes = Vec::new(); - let mut dst_keys = IndexMap::new(); - for loc in dst_locations1 { - dst_keys.insert(Self::loc_as_key(loc), ()); - } - let mut src_locations2red = Vec::new(); - let mut dst_locations2red = Vec::new(); - for i in 0..src_locations2.len() { - let loc = src_locations2[i]; - let dstloc = dst_locations2[i]; - if loc.is_stack() { - let key = Self::loc_as_key(&loc); - if dst_keys.contains_key(&key) - || (Self::loc_width(&loc) > WORD && dst_keys.contains_key(&(key + WORD as i32))) - { - self.regalloc_push(&loc); - extrapushes.push(dstloc); - continue; - } - } - src_locations2red.push(loc); - dst_locations2red.push(dstloc); - } - self.remap_frame_layout(src_locations1, dst_locations1, tmpreg1); - self.remap_frame_layout(&src_locations2red, &dst_locations2red, tmpreg2); - while let Some(loc) = extrapushes.pop() { - self.regalloc_pop(&loc); - } - } - /// Load a (lhs, src) pair for a 3-operand binop into registers, /// returning the register numbers to use. Uses x17 for lhs scratch /// and x16 for src scratch when the loc is Frame/Immed. @@ -3153,7 +2914,8 @@ impl<'a> AssemblerARM64<'a> { } let tmpreg1 = Loc::Reg(crate::regloc::RegLoc::new(16, false)); let tmpreg2 = Loc::Reg(crate::regloc::RegLoc::new(15, true)); - self.remap_frame_layout_mixed( + crate::jump::remap_frame_layout_mixed( + self, &src_locations1, &dst_locations1, tmpreg1, @@ -5911,8 +5673,8 @@ impl<'a> AssemblerARM64<'a> { // aarch64/callbuilder.py:62-65 — remap non-float then float args. let tmp_nf = Loc::Reg(crate::regloc::RegLoc::new(16, false)); // x16 (ip0) let tmp_fp = Loc::Reg(crate::regloc::RegLoc::new(15, true)); // d15 - self.remap_frame_layout(&non_float_src, &non_float_dst, tmp_nf); - self.remap_frame_layout(&float_src, &float_dst, tmp_fp); + crate::jump::remap_frame_layout(self, &non_float_src, &non_float_dst, tmp_nf); + crate::jump::remap_frame_layout(self, &float_src, &float_dst, tmp_fp); // Immediate args after remap (each targets a distinct ABI reg). for (abi_idx, val, is_float) in immed_args { @@ -8158,3 +7920,105 @@ mod tests { ); } } + +/// `jump.py`'s three primitives. The parallel-move algorithm that drives +/// them is one shared implementation in `crate::jump`; only these differ per +/// backend, which is what makes them the trait and it the free function. +impl<'a> crate::jump::RegallocMoves for AssemblerARM64<'a> { + fn regalloc_mov(&mut self, src: &Loc, dst: &Loc) { + if majit_ir::debug::have_debug_prints() { + majit_ir::debug::log_one("jit-backend", &format!("remap-mov: {src:?} -> {dst:?}")); + } + match (src, dst) { + (Loc::Reg(s), Loc::Reg(d)) if s == d => {} + (Loc::Reg(s), Loc::Reg(d)) => { + if s.is_xmm && d.is_xmm { + dynasm!(self.mc ; .arch aarch64 ; fmov D(d.value), D(s.value)); + } else if !s.is_xmm && !d.is_xmm { + dynasm!(self.mc ; .arch aarch64 ; mov X(d.value), X(s.value)); + } else if s.is_xmm && !d.is_xmm { + dynasm!(self.mc ; .arch aarch64 ; fmov X(d.value), D(s.value)); + } else { + dynasm!(self.mc ; .arch aarch64 ; fmov D(d.value), X(s.value)); + } + } + (Loc::Reg(s), Loc::Frame(f)) => { + let ofs = f.ebp_loc.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; + if d.is_xmm { + self.emit_ldr_fp_d(d.value, ofs); + } else { + self.emit_ldr_fp(d.value, ofs); + } + } + (Loc::Immed(i), Loc::Reg(d)) => { + if d.is_xmm { + self.emit_mov_imm64(16, i.value); // x16 = scratch + dynasm!(self.mc ; .arch aarch64 ; fmov D(d.value), X(16)); + } else { + self.emit_mov_imm64(d.value as u32, i.value); + } + } + (Loc::Immed(i), Loc::Frame(f)) => { + let ofs = f.ebp_loc.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; + self.emit_ldr_fp(16, o1); + self.emit_str_fp(16, o2); + } + _ => {} + } + } + + fn regalloc_push(&mut self, loc: &Loc) { + match loc { + Loc::Reg(r) if r.is_xmm => { + dynasm!(self.mc ; .arch aarch64 ; str D(r.value), [sp, #-16]!); + } + 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); + dynasm!(self.mc ; .arch aarch64 ; str D(15), [sp, #-16]!); + } + Loc::Frame(f) => { + self.emit_ldr_fp(16, f.ebp_loc.value); + dynasm!(self.mc ; .arch aarch64 ; str x16, [sp, #-16]!); + } + _ => {} + } + } + + fn regalloc_pop(&mut self, loc: &Loc) { + match loc { + Loc::Reg(r) if r.is_xmm => { + dynasm!(self.mc ; .arch aarch64 ; ldr D(r.value), [sp], #16); + } + Loc::Reg(r) => { + dynasm!(self.mc ; .arch aarch64 ; ldr X(r.value), [sp], #16); + } + Loc::Frame(f) if f.ebp_loc.is_float => { + dynasm!(self.mc ; .arch aarch64 ; ldr D(15), [sp], #16); + self.emit_str_fp_d(15, f.ebp_loc.value); + } + Loc::Frame(f) => { + dynasm!(self.mc ; .arch aarch64 ; ldr x16, [sp], #16); + self.emit_str_fp(16, f.ebp_loc.value); + } + _ => {} + } + } +} diff --git a/majit/majit-backend-dynasm/src/aarch64/opassembler.rs b/majit/majit-backend-dynasm/src/aarch64/opassembler.rs index 37c0f50e3a6..e8946a0f2a0 100644 --- a/majit/majit-backend-dynasm/src/aarch64/opassembler.rs +++ b/majit/majit-backend-dynasm/src/aarch64/opassembler.rs @@ -10,6 +10,7 @@ use dynasmrt::{DynasmApi, dynasm}; use super::assembler::AssemblerARM64; +use crate::jump::RegallocMoves; use crate::regloc::{Loc, RegLoc}; impl<'a> AssemblerARM64<'a> { diff --git a/majit/majit-backend-dynasm/src/jump.rs b/majit/majit-backend-dynasm/src/jump.rs index 813651c3f81..ea1229694f5 100644 --- a/majit/majit-backend-dynasm/src/jump.rs +++ b/majit/majit-backend-dynasm/src/jump.rs @@ -1,16 +1,196 @@ -/// jump.py: Frame layout remapping for register/frame moves. -/// -/// remap_frame_layout(assembler, src_locs, dst_locs, tmpreg) -/// — emit code to move values from src locations to dst locations, -/// handling overlaps via a temporary register. +//! `jump.py` — parallel assignment between location sets. +//! +//! Moving a set of values into a set of destinations is not a sequence of +//! independent moves: a destination may still be some other move's source, so +//! an order has to be found, and when the dependency graph has a cycle no +//! order exists and one value must be parked. `remap_frame_layout` is that +//! algorithm (`jump.py:4-64`), and `remap_frame_layout_mixed` (`jump.py:67-97`) +//! is the two-register-class variant used when integer and float arguments are +//! remapped together. +//! +//! ⚠ `dst_locations` and `src_locations` are swapped relative to upstream, +//! which spells the call `remap_frame_layout(assembler, dst_locations, +//! src_locations, tmpreg)`. Both are `&[Loc]`, so nothing catches a call +//! written from upstream's order — read the signature, not the memory of the +//! Python one. +//! +//! The algorithm is identical for every backend; only the three primitive +//! emitters it drives are not, which is why they are the trait and this is a +//! free function over it. + +use crate::arch::WORD; use crate::regloc::Loc; +use indexmap::IndexMap; + +/// The three emitters `remap_frame_layout` drives, named as `jump.py` calls +/// them on the `assembler` it is handed. +/// +/// `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. +pub(crate) trait RegallocMoves { + /// `assembler.py:1145 regalloc_mov(from_loc, to_loc)`. + fn regalloc_mov(&mut self, src: &Loc, dst: &Loc); + fn regalloc_push(&mut self, loc: &Loc); + fn regalloc_pop(&mut self, loc: &Loc); +} + +/// 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. +pub(crate) fn loc_as_key(loc: &Loc) -> i32 { + 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::Immed(_) => i32::MIN, + Loc::Addr(a) => a.offset, + } +} + +pub(crate) fn loc_width(loc: &Loc) -> usize { + match loc { + Loc::Reg(r) => r.get_width(), + Loc::Frame(f) => f.ebp_loc.get_width(), + Loc::Ebp(e) => e.get_width(), + _ => WORD, + } +} + +/// `jump.py:4 remap_frame_layout` — emit the moves that put `src_locations` +/// into `dst_locations`, in an order no move invalidates. +/// +/// `tmpreg` is needed for a stack-to-stack pair, which no machine here can +/// move in one instruction. +pub(crate) fn remap_frame_layout( + asm: &mut A, + src_locations: &[Loc], + dst_locations: &[Loc], + tmpreg: Loc, +) { + let mut pending_dests = dst_locations.len() as i32; + let mut srccount: IndexMap = IndexMap::new(); + for dst in dst_locations { + srccount.insert(loc_as_key(dst), 0); + } + for i in 0..dst_locations.len() { + let src = src_locations[i]; + if src.is_immed() { + continue; + } + let key = loc_as_key(&src); + if let Some(cnt) = srccount.get_mut(&key) { + if key == loc_as_key(&dst_locations[i]) { + *cnt = -(dst_locations.len() as i32) - 1; + pending_dests -= 1; + } else { + *cnt += 1; + } + } + } + + while pending_dests > 0 { + let mut progress = false; + for i in 0..dst_locations.len() { + let dst = dst_locations[i]; + let key = loc_as_key(&dst); + if srccount.get(&key).copied().unwrap_or(-1) == 0 { + srccount.insert(key, -1); + pending_dests -= 1; + let src = src_locations[i]; + if !src.is_immed() { + let src_key = loc_as_key(&src); + if let Some(cnt) = srccount.get_mut(&src_key) { + *cnt -= 1; + } + } + if dst.is_stack() && src.is_stack() { + asm.regalloc_mov(&src, &tmpreg); + asm.regalloc_mov(&tmpreg, &dst); + } else { + asm.regalloc_mov(&src, &dst); + } + progress = true; + } + } + if !progress { + let mut sources: IndexMap = IndexMap::new(); + for i in 0..dst_locations.len() { + sources.insert(loc_as_key(&dst_locations[i]), src_locations[i]); + } + for dst in dst_locations { + let originalkey = loc_as_key(dst); + if srccount.get(&originalkey).copied().unwrap_or(-1) >= 0 { + asm.regalloc_push(dst); + let mut cur_dst = *dst; + loop { + let key = loc_as_key(&cur_dst); + srccount.insert(key, -1); + pending_dests -= 1; + let src = sources[&key]; + if loc_as_key(&src) == originalkey { + break; + } + if cur_dst.is_stack() && src.is_stack() { + asm.regalloc_mov(&src, &tmpreg); + asm.regalloc_mov(&tmpreg, &cur_dst); + } else { + asm.regalloc_mov(&src, &cur_dst); + } + cur_dst = src; + } + asm.regalloc_pop(&cur_dst); + } + } + } + } +} -/// jump.py:4 remap_frame_layout — emit moves to rearrange locations. +/// `jump.py:67 remap_frame_layout_mixed` — two location sets remapped with a +/// temporary each, as integer and float arguments need. /// -/// This handles the problem of parallel assignment: if src[i] overlaps -/// with dst[j], we need a temporary to break the cycle. -pub fn remap_frame_layout(_src_locs: &[Loc], _dst_locs: &[Loc], _tmpreg: Loc) -> Vec<(Loc, Loc)> { - // TODO: implement cycle-breaking parallel move algorithm - // For now, return direct moves (incorrect if overlaps exist) - Vec::new() +/// The sets are not independent: a set-2 stack source may be a set-1 +/// destination, and set 1 runs first. Those sources are pushed before either +/// remap and popped into place after, which is why they are dropped from the +/// set-2 lists rather than reordered. +pub(crate) fn remap_frame_layout_mixed( + asm: &mut A, + src_locations1: &[Loc], + dst_locations1: &[Loc], + tmpreg1: Loc, + src_locations2: &[Loc], + dst_locations2: &[Loc], + tmpreg2: Loc, +) { + let mut extrapushes = Vec::new(); + let mut dst_keys = IndexMap::new(); + for loc in dst_locations1 { + dst_keys.insert(loc_as_key(loc), ()); + } + let mut src_locations2red = Vec::new(); + let mut dst_locations2red = Vec::new(); + for i in 0..src_locations2.len() { + let loc = src_locations2[i]; + let dstloc = dst_locations2[i]; + 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))) + { + asm.regalloc_push(&loc); + extrapushes.push(dstloc); + continue; + } + } + src_locations2red.push(loc); + dst_locations2red.push(dstloc); + } + remap_frame_layout(asm, src_locations1, dst_locations1, tmpreg1); + remap_frame_layout(asm, &src_locations2red, &dst_locations2red, tmpreg2); + while let Some(loc) = extrapushes.pop() { + asm.regalloc_pop(&loc); + } } diff --git a/majit/majit-backend-dynasm/src/lib.rs b/majit/majit-backend-dynasm/src/lib.rs index 76ca69054ff..d5a69adb100 100644 --- a/majit/majit-backend-dynasm/src/lib.rs +++ b/majit/majit-backend-dynasm/src/lib.rs @@ -25,7 +25,7 @@ pub mod guard; pub(crate) mod j2plan; pub use majit_backend::jitframe; pub use majit_backend::llmodel; -pub mod jump; +pub(crate) mod jump; #[expect( clippy::too_many_arguments, reason = "the register-allocation entry points preserve RPython's explicit state-threading signatures; bundling those arguments would diverge from the audited line-by-line backend port" diff --git a/majit/majit-backend-dynasm/src/x86/assembler.rs b/majit/majit-backend-dynasm/src/x86/assembler.rs index c04e5710aec..c7af6bde4d7 100644 --- a/majit/majit-backend-dynasm/src/x86/assembler.rs +++ b/majit/majit-backend-dynasm/src/x86/assembler.rs @@ -29,6 +29,7 @@ use crate::jitframe::{ FIRST_ITEM_OFFSET, JF_DESCR_OFS, JF_FORCE_DESCR_OFS, JF_FORWARD_OFS, JF_FRAME_OFS, JF_GCMAP_OFS, JF_GUARD_EXC_OFS, }; +use crate::jump::RegallocMoves; use crate::regalloc::{RegAlloc, RegAllocOp}; use crate::regloc::Loc; use crate::runner::GuardGcTypeInfo; @@ -1194,248 +1195,6 @@ impl<'a> Assembler386<'a> { slot } - // ── Location-aware code emission (RPython regalloc parity) ── - // assembler.py regalloc_mov: move value between any two locations. - - /// assembler.py:1145 regalloc_mov(from_loc, to_loc). - /// Emit a move between any two locations: reg↔reg, reg↔frame, imm→reg, imm→frame. - pub(crate) fn regalloc_mov(&mut self, src: &Loc, dst: &Loc) { - match (src, dst) { - (Loc::Reg(s), Loc::Reg(d)) if s == d => {} - (Loc::Reg(s), Loc::Reg(d)) => { - if s.is_xmm && d.is_xmm { - dynasm!(self.mc ; .arch x64 ; movsd Rx(d.value), Rx(s.value)); - } else if !s.is_xmm && !d.is_xmm { - dynasm!(self.mc ; .arch x64 ; mov Rq(d.value), Rq(s.value)); - } else if s.is_xmm && !d.is_xmm { - dynasm!(self.mc ; .arch x64 ; movq Rq(d.value), Rx(s.value)); - } else { - dynasm!(self.mc ; .arch x64 ; movq Rx(d.value), Rq(s.value)); - } - } - (Loc::Reg(s), Loc::Frame(f)) => { - let ofs = f.ebp_loc.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; - if d.is_xmm { - dynasm!(self.mc ; .arch x64 ; movsd Rx(d.value), [rbp + ofs]); - } else { - dynasm!(self.mc ; .arch x64 ; mov Rq(d.value), [rbp + ofs]); - } - } - (Loc::Immed(i), Loc::Reg(d)) => { - if d.is_xmm { - let scratch = crate::regloc::X86_64_SCRATCH_REG.value; - dynasm!(self.mc ; .arch x64 - ; mov Rq(scratch), QWORD i.value - ; movq Rx(d.value), Rq(scratch) - ); - } else { - dynasm!(self.mc ; .arch x64 ; mov Rq(d.value), QWORD i.value); - } - } - (Loc::Immed(i), Loc::Frame(f)) => { - let ofs = f.ebp_loc.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; - let scratch = crate::regloc::X86_64_SCRATCH_REG.value; - dynasm!(self.mc ; .arch x64 - ; mov Rq(scratch), [rbp + o1] - ; mov [rbp + o2], Rq(scratch) - ); - } - _ => {} - } - } - - fn loc_as_key(loc: &Loc) -> i32 { - 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::Immed(_) => i32::MIN, - Loc::Addr(a) => a.offset, - } - } - - fn loc_width(loc: &Loc) -> usize { - match loc { - Loc::Reg(r) => r.get_width(), - Loc::Frame(f) => f.ebp_loc.get_width(), - Loc::Ebp(e) => e.get_width(), - _ => WORD, - } - } - - fn regalloc_push(&mut self, loc: &Loc) { - match loc { - Loc::Reg(r) if r.is_xmm => { - dynasm!(self.mc ; .arch x64 ; sub rsp, 8 ; movsd [rsp], Rx(r.value)); - } - 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); - } - Loc::Frame(f) => { - dynasm!(self.mc ; .arch x64 ; push QWORD [rbp + f.ebp_loc.value]); - } - _ => {} - } - } - - fn regalloc_pop(&mut self, loc: &Loc) { - match loc { - Loc::Reg(r) if r.is_xmm => { - dynasm!(self.mc ; .arch x64 ; movsd Rx(r.value), [rsp] ; add rsp, 8); - } - 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); - } - Loc::Frame(f) => { - dynasm!(self.mc ; .arch x64 ; pop QWORD [rbp + f.ebp_loc.value]); - } - _ => {} - } - } - - fn remap_frame_layout(&mut self, src_locations: &[Loc], dst_locations: &[Loc], tmpreg: Loc) { - let mut pending_dests = dst_locations.len() as i32; - let mut srccount: IndexMap = IndexMap::new(); - for dst in dst_locations { - srccount.insert(Self::loc_as_key(dst), 0); - } - for i in 0..dst_locations.len() { - let src = src_locations[i]; - if src.is_immed() { - continue; - } - let key = Self::loc_as_key(&src); - if let Some(cnt) = srccount.get_mut(&key) { - if key == Self::loc_as_key(&dst_locations[i]) { - *cnt = -(dst_locations.len() as i32) - 1; - pending_dests -= 1; - } else { - *cnt += 1; - } - } - } - - while pending_dests > 0 { - let mut progress = false; - for i in 0..dst_locations.len() { - let dst = dst_locations[i]; - let key = Self::loc_as_key(&dst); - if srccount.get(&key).copied().unwrap_or(-1) == 0 { - srccount.insert(key, -1); - pending_dests -= 1; - let src = src_locations[i]; - if !src.is_immed() { - let src_key = Self::loc_as_key(&src); - if let Some(cnt) = srccount.get_mut(&src_key) { - *cnt -= 1; - } - } - if dst.is_stack() && src.is_stack() { - self.regalloc_mov(&src, &tmpreg); - self.regalloc_mov(&tmpreg, &dst); - } else { - self.regalloc_mov(&src, &dst); - } - progress = true; - } - } - if !progress { - let mut sources: IndexMap = IndexMap::new(); - for i in 0..dst_locations.len() { - sources.insert(Self::loc_as_key(&dst_locations[i]), src_locations[i]); - } - for dst in dst_locations { - let originalkey = Self::loc_as_key(dst); - if srccount.get(&originalkey).copied().unwrap_or(-1) >= 0 { - self.regalloc_push(dst); - let mut cur_dst = *dst; - loop { - let key = Self::loc_as_key(&cur_dst); - srccount.insert(key, -1); - pending_dests -= 1; - let src = sources[&key]; - if Self::loc_as_key(&src) == originalkey { - break; - } - if cur_dst.is_stack() && src.is_stack() { - self.regalloc_mov(&src, &tmpreg); - self.regalloc_mov(&tmpreg, &cur_dst); - } else { - self.regalloc_mov(&src, &cur_dst); - } - cur_dst = src; - } - self.regalloc_pop(&cur_dst); - } - } - } - } - } - - fn remap_frame_layout_mixed( - &mut self, - src_locations1: &[Loc], - dst_locations1: &[Loc], - tmpreg1: Loc, - src_locations2: &[Loc], - dst_locations2: &[Loc], - tmpreg2: Loc, - ) { - let mut extrapushes = Vec::new(); - let mut dst_keys = IndexMap::new(); - for loc in dst_locations1 { - dst_keys.insert(Self::loc_as_key(loc), ()); - } - let mut src_locations2red = Vec::new(); - let mut dst_locations2red = Vec::new(); - for i in 0..src_locations2.len() { - let loc = src_locations2[i]; - let dstloc = dst_locations2[i]; - if loc.is_stack() { - let key = Self::loc_as_key(&loc); - if dst_keys.contains_key(&key) - || (Self::loc_width(&loc) > WORD && dst_keys.contains_key(&(key + WORD as i32))) - { - self.regalloc_push(&loc); - extrapushes.push(dstloc); - continue; - } - } - src_locations2red.push(loc); - dst_locations2red.push(dstloc); - } - self.remap_frame_layout(src_locations1, dst_locations1, tmpreg1); - self.remap_frame_layout(&src_locations2red, &dst_locations2red, tmpreg2); - while let Some(loc) = extrapushes.pop() { - self.regalloc_pop(&loc); - } - } - /// Emit: ADD/SUB/AND/OR/XOR reg, loc fn emit_binop_reg_loc(&mut self, opcode: OpCode, dst_reg: u8, src: &Loc) { // aarch64: load src to x16 scratch if not in register @@ -4150,7 +3909,8 @@ impl<'a> Assembler386<'a> { majit_ir::debug::debug_print(&format!(" int[{i}]: {s:?} → {d:?}")); } } - self.remap_frame_layout_mixed( + crate::jump::remap_frame_layout_mixed( + self, &src_locations1, &dst_locations1, tmpreg1, @@ -7277,7 +7037,9 @@ impl<'a> Assembler386<'a> { } let tmpreg1 = Loc::Reg(crate::regloc::X86_64_SCRATCH_REG); let tmpreg2 = Loc::Reg(crate::regloc::XMM15); - self.remap_frame_layout_mixed(&int_src, &int_dst, tmpreg1, &xmm_src, &xmm_dst, tmpreg2); + crate::jump::remap_frame_layout_mixed( + self, &int_src, &int_dst, tmpreg1, &xmm_src, &xmm_dst, tmpreg2, + ); // Call. For Immed/Frame targets, load rax now (parallel move // never touches rax or rbp, so this is safe). For Reg targets, @@ -9043,3 +8805,107 @@ impl<'a> Assembler386<'a> { self.store_rax_to_result(op.pos.get()); } } + +/// `jump.py`'s three primitives. The parallel-move algorithm that drives +/// them is one shared implementation in `crate::jump`; only these differ per +/// backend, which is what makes them the trait and it the free function. +impl<'a> crate::jump::RegallocMoves for Assembler386<'a> { + fn regalloc_mov(&mut self, src: &Loc, dst: &Loc) { + match (src, dst) { + (Loc::Reg(s), Loc::Reg(d)) if s == d => {} + (Loc::Reg(s), Loc::Reg(d)) => { + if s.is_xmm && d.is_xmm { + dynasm!(self.mc ; .arch x64 ; movsd Rx(d.value), Rx(s.value)); + } else if !s.is_xmm && !d.is_xmm { + dynasm!(self.mc ; .arch x64 ; mov Rq(d.value), Rq(s.value)); + } else if s.is_xmm && !d.is_xmm { + dynasm!(self.mc ; .arch x64 ; movq Rq(d.value), Rx(s.value)); + } else { + dynasm!(self.mc ; .arch x64 ; movq Rx(d.value), Rq(s.value)); + } + } + (Loc::Reg(s), Loc::Frame(f)) => { + let ofs = f.ebp_loc.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; + if d.is_xmm { + dynasm!(self.mc ; .arch x64 ; movsd Rx(d.value), [rbp + ofs]); + } else { + dynasm!(self.mc ; .arch x64 ; mov Rq(d.value), [rbp + ofs]); + } + } + (Loc::Immed(i), Loc::Reg(d)) => { + if d.is_xmm { + let scratch = crate::regloc::X86_64_SCRATCH_REG.value; + dynasm!(self.mc ; .arch x64 + ; mov Rq(scratch), QWORD i.value + ; movq Rx(d.value), Rq(scratch) + ); + } else { + dynasm!(self.mc ; .arch x64 ; mov Rq(d.value), QWORD i.value); + } + } + (Loc::Immed(i), Loc::Frame(f)) => { + let ofs = f.ebp_loc.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; + let scratch = crate::regloc::X86_64_SCRATCH_REG.value; + dynasm!(self.mc ; .arch x64 + ; mov Rq(scratch), [rbp + o1] + ; mov [rbp + o2], Rq(scratch) + ); + } + _ => {} + } + } + + fn regalloc_push(&mut self, loc: &Loc) { + match loc { + Loc::Reg(r) if r.is_xmm => { + dynasm!(self.mc ; .arch x64 ; sub rsp, 8 ; movsd [rsp], Rx(r.value)); + } + 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); + } + Loc::Frame(f) => { + dynasm!(self.mc ; .arch x64 ; push QWORD [rbp + f.ebp_loc.value]); + } + _ => {} + } + } + + fn regalloc_pop(&mut self, loc: &Loc) { + match loc { + Loc::Reg(r) if r.is_xmm => { + dynasm!(self.mc ; .arch x64 ; movsd Rx(r.value), [rsp] ; add rsp, 8); + } + 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); + } + Loc::Frame(f) => { + dynasm!(self.mc ; .arch x64 ; pop QWORD [rbp + f.ebp_loc.value]); + } + _ => {} + } + } +} From b1888745d72fd5174a178853a84ec8d6d536d63f Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 15 Aug 2026 21:20:25 +0900 Subject: [PATCH 6/6] Describe the register policy the call arms actually run under Both backends' call-dispatch arms carried "For now, flush all register-resident values to their frame slots before the call". No such flush exists in the emitter or anywhere it calls. `consider_call` runs `before_call` with `SAVE_ALL_REGS`, `SAVE_GCREF_REGS` or `SAVE_DEFAULT_REGS` per the descr, and `spill_or_move_registers_before_call` (`regalloc.py:714`) drops values dying at the call, leaves callee-saved ones in place and moves the rest to a free callee-saved register where one exists. Replace the comment with that. Assisted-by: Claude --- .../src/aarch64/assembler.rs | 17 ++++++++++------- majit/majit-backend-dynasm/src/x86/assembler.rs | 17 ++++++++++------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/majit/majit-backend-dynasm/src/aarch64/assembler.rs b/majit/majit-backend-dynasm/src/aarch64/assembler.rs index 539904c229b..c01e9992297 100644 --- a/majit/majit-backend-dynasm/src/aarch64/assembler.rs +++ b/majit/majit-backend-dynasm/src/aarch64/assembler.rs @@ -3059,14 +3059,17 @@ impl<'a> AssemblerARM64<'a> { } } // ── Calls ── - // RPython: regalloc consider_call does before_call (save caller-saved - // regs), collects arglocs, calls after_call for result. The assembler - // receives arglocs = [func_addr_or_descr_info..., arg_locs...] and - // result_loc = register for return value. + // `consider_call` (regalloc.rs) has already captured `arglocs = + // [func_addr_or_descr_info..., arg_locs...]` and run `before_call` + // for this op, so the register file is in its across-the-call shape + // by the time the emitter sees it. Which registers that saved is the + // `SAVE_ALL_REGS / SAVE_GCREF_REGS / SAVE_DEFAULT_REGS` choice made + // there, not a blanket spill: `spill_or_move_registers_before_call` + // (`regalloc.py:714`) drops values that die at the call, leaves + // callee-saved ones where they are, and prefers moving the rest to a + // free callee-saved register over spilling them. // - // For now, flush all register-resident values to their frame slots - // before the call, use the existing frame-slot genop_call, then - // mark the result in the allocated register. + // What is left here is the call and the result placement. OpCode::CallI | OpCode::CallF | OpCode::CallN diff --git a/majit/majit-backend-dynasm/src/x86/assembler.rs b/majit/majit-backend-dynasm/src/x86/assembler.rs index c7af6bde4d7..7349b5f57cb 100644 --- a/majit/majit-backend-dynasm/src/x86/assembler.rs +++ b/majit/majit-backend-dynasm/src/x86/assembler.rs @@ -4083,14 +4083,17 @@ impl<'a> Assembler386<'a> { } } // ── Calls ── - // RPython: regalloc consider_call does before_call (save caller-saved - // regs), collects arglocs, calls after_call for result. The assembler - // receives arglocs = [func_addr_or_descr_info..., arg_locs...] and - // result_loc = register for return value. + // `consider_call` (regalloc.rs) has already captured `arglocs = + // [func_addr_or_descr_info..., arg_locs...]` and run `before_call` + // for this op, so the register file is in its across-the-call shape + // by the time the emitter sees it. Which registers that saved is the + // `SAVE_ALL_REGS / SAVE_GCREF_REGS / SAVE_DEFAULT_REGS` choice made + // there, not a blanket spill: `spill_or_move_registers_before_call` + // (`regalloc.py:714`) drops values that die at the call, leaves + // callee-saved ones where they are, and prefers moving the rest to a + // free callee-saved register over spilling them. // - // For now, flush all register-resident values to their frame slots - // before the call, use the existing frame-slot genop_call, then - // mark the result in the allocated register. + // What is left here is the call and the result placement. OpCode::CallI | OpCode::CallF | OpCode::CallN