From acda985f1fe2425652ab308c8afc583176cf8fcb Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Wed, 1 Jul 2026 21:44:17 +0200 Subject: [PATCH] =?UTF-8?q?fix(#498):=20estimator=E2=86=94encoder=20alignm?= =?UTF-8?q?ent=20=E2=80=94=20close=2010=20pure-estimator=20size=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Estimator-only change (`estimate_arm_byte_size` in synth-synthesis): make the hand-maintained byte-size mirror match what the Thumb-2 encoder already emits. No emitted `.text` changes — frozen anchors bit-identical (3/3). Closed gaps (op → old estimate → correct encoder length): - Cmn (high-reg reg-form) 2 → 4 (CMN.W, no 16-bit high-reg form) - Adds (high-reg reg-form) 2 → 4 (ADDS.W, flag-setting) - Subs (high-reg reg-form) 2 → 4 (SUBS.W, flag-setting) (+ imm forms of all three pinned to their always-32-bit length) - Popcnt 2 → 86/84 (84 body + 2 for rd!=rm MOV) - I64Popcnt 200 → 172 - I64Extend32S 8 → 6 (MOV + ASR) - I64DivU 100 → 74 - I64RemU 100 → 78 - I64DivS 150 → 126 - I64RemS 150 → 124 Oracle (`estimator_encoder_agreement`) tightened: these 10 now assert EXACT agreement (moved from KNOWN_GAP to `agree`), so any future drift fails loudly. Two gaps remain pinned, NOT simple estimator table errors: - far BOffset/BCondOffset (structural): the estimator runs on a 0-offset placeholder before displacements are resolved; offset-sensitivity would be cosmetic or entangle the displacement pass. - Mov negative small imm (encoder-side, deferred): the encoder's signed `imm <= 255` test emits a wrong-value 2-byte MOVS #(imm&0xFF); aligning the estimator down would endorse a buggy encoding. Real fix is encoder-side (byte-changing → separate gated PR). Co-Authored-By: Claude Opus 4.8 --- .../tests/estimator_encoder_agreement.rs | 120 ++++++++---------- .../synth-synthesis/src/optimizer_bridge.rs | 70 ++++++++-- 2 files changed, 112 insertions(+), 78 deletions(-) diff --git a/crates/synth-backend/tests/estimator_encoder_agreement.rs b/crates/synth-backend/tests/estimator_encoder_agreement.rs index 4cc93b22..4efa4091 100644 --- a/crates/synth-backend/tests/estimator_encoder_agreement.rs +++ b/crates/synth-backend/tests/estimator_encoder_agreement.rs @@ -15,38 +15,48 @@ //! estimator and the real encoder. This oracle pins one against the other for //! every op the optimized path emits, at the operand shapes it emits them in. //! -//! ## Scope — this is a gap-documenting REGRESSION GUARD, not a #498 fix. +//! ## Scope — the estimator-side gaps are now CLOSED; two survivors remain. //! -//! Several estimator entries diverge from the encoder today; this test does not -//! correct them (that is byte-changing codegen, a separately-gated step — it -//! shifts every branch displacement on the optimized path and must be re-frozen, -//! execution-differentialed, and silicon-checked). Instead it freezes the -//! divergence as a measured `KNOWN_GAP`, so: +//! The #498 estimator-alignment step (this PR) corrected every gap that was a +//! pure estimator under/over-count against a CORRECT encoder length. Those are +//! estimator-only edits (`estimate_arm_byte_size` in synth-synthesis) — they do +//! not change emitted `.text` (verified: frozen anchors bit-identical), only the +//! branch-displacement bookkeeping the estimator feeds. The oracle now asserts +//! EXACT agreement for them, so any future drift fails loudly. Two divergences +//! remain pinned as `KNOWN_GAP` because they are NOT simple table errors: //! //! - a NEW estimator drift (an added op left at the `_ => 2` default, or a //! changed encoding) fails the agreement assertion loudly, and -//! - a future #498 fix that closes a gap fails the `assert_ne!` "still -//! diverges" check, forcing the gap to be removed from the list. +//! - a re-closure of one of the two survivors fails the "gap closed" check, +//! forcing the gap to be removed from the list. //! //! ## Findings recorded here (correct + extend #498's original report) //! - #498 claimed `Cmp` high-reg drifts. It does NOT: the 16-bit CMP (T2, //! `0x45xx`) encodes high registers, so `cmp r10, r8` is 2 bytes and the -//! estimator's default is right. The real high-reg drifts are `Cmn`/`Adds`/ +//! estimator's default is right. The real high-reg drifts were `Cmn`/`Adds`/ //! `Subs` (no 16-bit high-reg form / flag-setting) → 4 bytes, est 2. -//! - `Popcnt` is absent from the estimator entirely → `_ => 2`, but the -//! encoder expands it to a 21-instruction sequence = 86 bytes. An 84-byte -//! hole — the largest single drift. -//! - The i64 long-sequence estimates `I64DivU/RemU/DivS/RemS`, `I64Popcnt` -//! and `I64Extend32S` OVER-estimate (e.g. DivU est 100 vs 74) — branch -//! lands long. -//! - Far branches (`BOffset`/`BCondOffset` with a large displacement) need -//! the 4-byte `.w` form but the estimator (which runs BEFORE displacements -//! are known, with a 0-offset placeholder) sizes them 2 — a chicken-and-egg -//! the single-pass estimator cannot resolve. -//! - `Mov` with a small NEGATIVE immediate: estimator sizes 4 (prudent), but -//! the encoder's `if *imm <= 255` is a *signed* test and emits a 2-byte -//! `MOVS Rd, #(imm & 0xFF)` — a wrong-value 0xFF, a latent ENCODER bug for -//! negative imm8, surfaced here as a side effect. Tracked separately. +//! CLOSED: the estimator now guards the high-reg reg-form (and the +//! always-32-bit imm form) of all three. +//! - `Popcnt` was absent from the estimator entirely → `_ => 2`, but the +//! encoder expands it to a fixed bit-twiddle sequence = 84 body + 2 for the +//! `rd != rm` MOV. CLOSED: estimator now mirrors `if rd != rm { 86 } else +//! { 84 }`. +//! - The i64 long-sequence estimates `I64DivU/RemU/DivS/RemS` (100/100/150/150), +//! `I64Popcnt` (200) and `I64Extend32S` (8) OVER-estimated. CLOSED: pinned to +//! the exact encoder lengths (74/78/126/124, 172, 6). +//! - SURVIVOR (structural): far branches (`BOffset`/`BCondOffset` with a large +//! displacement) need the 4-byte `.w` form but the estimator runs BEFORE +//! displacements are known, on a 0-offset placeholder, so it sizes them 2 — +//! a chicken-and-egg the single-pass estimator cannot resolve. Making it +//! offset-sensitive would either be cosmetic (production always sees offset 0) +//! or entangle with the displacement-resolution pass (byte-changing). Left as +//! a documented structural gap, not an estimator-table fix. +//! - SURVIVOR (encoder-side, deferred): `Mov` with a small NEGATIVE immediate: +//! estimator sizes 4 (prudent), but the encoder's `if *imm <= 255` is a +//! *signed* test and emits a 2-byte `MOVS Rd, #(imm & 0xFF)` — a wrong-value +//! 0xFF, a latent ENCODER bug for negative imm8. Aligning the estimator DOWN +//! to 2 would endorse a buggy encoding; the real fix is encoder-side (emits +//! different bytes → frozen-affecting → a separate gated PR). Kept pinned. use synth_backend::ArmEncoder; use synth_synthesis::{ArmOp, Condition, MemAddr, Operand2, Reg, estimate_arm_byte_size}; @@ -618,76 +628,60 @@ fn cases() -> Vec { rnlo: Reg::R2, }, ), - // ============================ KNOWN GAPS ============================ - // High-reg flag-setting / no-16-bit-form ops: estimator default 2, but - // the encoder must use the 32-bit `.w` form (4 bytes). The #498 core. - gap( + // === #498 closed gaps: estimator now mirrors the encoder exactly === + // High-reg flag-setting / no-16-bit-form ops: the encoder uses the + // 32-bit `.w` form (4 bytes); the estimator now guards the high-reg + // reg-form (and the always-32-bit imm form) instead of defaulting to 2. + agree( "Cmn_hi", Cmn { rn: Reg::R10, op2: Operand2::Reg(Reg::R8), }, - 2, - 4, - "CMN has no 16-bit high-reg form → CMN.W (4); estimator _=>2. Under-estimate.", ), - gap( + agree( "Adds_hi", Adds { rd: Reg::R10, rn: Reg::R10, op2: Operand2::Reg(Reg::R8), }, - 2, - 4, - "flag-setting ADDS high-reg → ADDS.W (4); estimator _=>2. Under-estimate.", ), - gap( + agree( "Subs_hi", Subs { rd: Reg::R10, rn: Reg::R10, op2: Operand2::Reg(Reg::R8), }, - 2, - 4, - "flag-setting SUBS high-reg → SUBS.W (4); estimator _=>2. Under-estimate.", ), - // Popcnt: absent from the estimator entirely; encoder expands it. - gap( + // Popcnt: the estimator now sizes the encoder's fixed bit-twiddle + // sequence (84 body + 2 for the rd!=rm MOV = 86). + agree( "Popcnt", Popcnt { rd: Reg::R0, rm: Reg::R1, }, - 2, - 86, - "Popcnt absent from estimator (_=>2); encoder emits a 21-insn sequence (86). 84-byte under-estimate.", ), - // i64 long sequences whose hand-counted estimate is too large. - gap( + // i64 long sequences: estimator now pinned to the exact encoder lengths. + agree( "I64Popcnt", I64Popcnt { rd: Reg::R0, rnlo: Reg::R1, rnhi: Reg::R2, }, - 200, - 172, - "estimate 200 (~180 guess) vs real 172. Over-estimate.", ), - gap( + agree( "I64Extend32S", I64Extend32S { rdlo: Reg::R0, rdhi: Reg::R1, rnlo: Reg::R2, }, - 8, - 6, - "estimate 8 vs real 6 (MOV + ASR). Over-estimate.", ), - gap( + agree( "I64DivU", I64DivU { rdlo: Reg::R0, @@ -697,11 +691,8 @@ fn cases() -> Vec { rmlo: Reg::R4, rmhi: Reg::R5, }, - 100, - 74, - "estimate 100 (~80-150 guess) vs real 74. Over-estimate.", ), - gap( + agree( "I64RemU", I64RemU { rdlo: Reg::R0, @@ -711,11 +702,8 @@ fn cases() -> Vec { rmlo: Reg::R4, rmhi: Reg::R5, }, - 100, - 78, - "estimate 100 vs real 78. Over-estimate.", ), - gap( + agree( "I64DivS", I64DivS { rdlo: Reg::R0, @@ -725,11 +713,8 @@ fn cases() -> Vec { rmlo: Reg::R4, rmhi: Reg::R5, }, - 150, - 126, - "estimate 150 vs real 126. Over-estimate.", ), - gap( + agree( "I64RemS", I64RemS { rdlo: Reg::R0, @@ -739,10 +724,9 @@ fn cases() -> Vec { rmlo: Reg::R4, rmhi: Reg::R5, }, - 150, - 124, - "estimate 150 vs real 124. Over-estimate.", ), + // ===================== REMAINING KNOWN GAPS ===================== + // Two survivors, NOT simple estimator table errors — kept pinned. // Far branches: the estimator sizes the 0-offset placeholder (it runs // before displacements are resolved) as 2, but a far target needs the // 4-byte form. Chicken-and-egg in the single-pass estimator. diff --git a/crates/synth-synthesis/src/optimizer_bridge.rs b/crates/synth-synthesis/src/optimizer_bridge.rs index 46e2c2d5..83f21ecd 100644 --- a/crates/synth-synthesis/src/optimizer_bridge.rs +++ b/crates/synth-synthesis/src/optimizer_bridge.rs @@ -349,6 +349,52 @@ pub fn estimate_arm_byte_size(op: &ArmOp) -> usize { ArmOp::Adc { .. } | ArmOp::Sbc { .. } => 4, // CLZ, RBIT are always 32-bit Thumb-2 ArmOp::Clz { .. } | ArmOp::Rbit { .. } => 4, + // Popcnt: the encoder expands it to a fixed bit-twiddle sequence using + // r11/r12 scratch (arm_encoder.rs). 84 bytes for the body, +2 for the + // leading 16-bit `MOV rd, rm` when rd != rm. #498: previously absent + // from the estimator (fell to `_ => 2`), an 82/84-byte under-estimate. + ArmOp::Popcnt { rd, rm } => { + if reg_num(rd) != reg_num(rm) { + 86 + } else { + 84 + } + } + // ADDS/SUBS with a register operand: 16-bit only when rd, rn, rm are all + // R0-R7 (flag-setting T1 form); any high register forces the 32-bit + // ADDS.W/SUBS.W (#178/#180). The immediate form is always 32-bit. + // #498: previously fell to `_ => 2`, under-estimating the high-reg form. + ArmOp::Adds { + rd, + rn, + op2: Operand2::Reg(rm), + } + | ArmOp::Subs { + rd, + rn, + op2: Operand2::Reg(rm), + } => { + if reg_num(rd) < 8 && reg_num(rn) < 8 && reg_num(rm) < 8 { + 2 + } else { + 4 + } + } + ArmOp::Adds { .. } | ArmOp::Subs { .. } => 4, + // CMN with a register operand: 16-bit T1 only for R0-R7; high registers + // (CMN has no 16-bit high-reg form, #184) force CMN.W (32-bit). The + // immediate form is always 32-bit (CMN.W). #498: previously `_ => 2`. + ArmOp::Cmn { + rn, + op2: Operand2::Reg(rm), + } => { + if reg_num(rn) < 8 && reg_num(rm) < 8 { + 2 + } else { + 4 + } + } + ArmOp::Cmn { .. } => 4, // SXTB, SXTH, UXTB, UXTH can be 16-bit for low registers ArmOp::Sxtb { rd, rm } | ArmOp::Sxth { rd, rm } @@ -376,18 +422,22 @@ pub fn estimate_arm_byte_size(op: &ArmOp) -> usize { ArmOp::I64Clz { .. } => 24, // I64Ctz: CMP.W(4) + BEQ(2) + RBIT.W(4) + CLZ.W(4) + B(2) + NOP(2) + RBIT.W(4) + CLZ.W(4) + ADD.W(4) + MOV(2) = 32 bytes ArmOp::I64Ctz { .. } => 32, - // I64Popcnt: large implementation with PUSH/POP and duplicate algorithm for lo and hi = ~180 bytes - ArmOp::I64Popcnt { .. } => 200, - // I64 sign extension: SXTB/SXTH/ASR + ASR = 4-8 bytes + // I64Popcnt: PUSH/POP + duplicate popcount for lo and hi word (#498: + // measured 172, was a ~180-guess 200 over-estimate). + ArmOp::I64Popcnt { .. } => 172, + // I64 sign extension: SXTB/SXTH/ASR + ASR. ArmOp::I64Extend8S { .. } => 8, ArmOp::I64Extend16S { .. } => 8, - ArmOp::I64Extend32S { .. } => 8, - // I64 division: PUSH + init + loop body + MOV + POP = ~80-150 bytes - ArmOp::I64DivU { .. } => 100, - ArmOp::I64RemU { .. } => 100, - // Signed versions have additional negation logic - ArmOp::I64DivS { .. } => 150, - ArmOp::I64RemS { .. } => 150, + // I64Extend32S: MOV (lo passthrough) + ASR (sign-fill hi) = 6 bytes + // (#498: measured 6, was an 8-byte over-estimate). + ArmOp::I64Extend32S { .. } => 6, + // I64 division: PUSH + init + binary long-division loop + MOV + POP. + // #498: exact measured sizes (were ~80-150 guesses). + ArmOp::I64DivU { .. } => 74, + ArmOp::I64RemU { .. } => 78, + // Signed versions have additional negation logic. + ArmOp::I64DivS { .. } => 126, + ArmOp::I64RemS { .. } => 124, // AND/OR/XOR: encoder always uses 32-bit Thumb-2 (.W) encoding ArmOp::And { .. } | ArmOp::Orr { .. } | ArmOp::Eor { .. } => 4, // LDR/STR with high base register or large offset need 32-bit