From 6b2e6724a23c94ba6bd01aced4c5d76cc25c5152 Mon Sep 17 00:00:00 2001 From: Dennis Brentjes Date: Sat, 10 Oct 2026 14:16:43 +0200 Subject: [PATCH] Fix the 0/8 timing failure the ring-geometry change caused The sweep after honouring BP/RHBP came back clk 16.5-17.6 MHz, 0/8 seeds passing against the 24 MHz target -- from ~29 MHz before. The build was unusable. Cause: the sanitised bounds were combinational, so bp/rhbp -> comparators -> Mux -> subtract -> pg_total -> add -> raw -> compare -> free -> compare -> have_room became ONE path where it had been constant-folded. Two fixes were needed, and the first alone was not enough: 1. Register pg_first/pg_last/pg_total and rrp_s. They change only when the GC writes BP/RHBP/RRP, so a cycle of staleness costs nothing, and `free` already reaches the FSM through the registered have_room. This got it to ~20 MHz, still failing. 2. Narrow them from 8 bits to 4. Ring pages are 1-15. An 8-bit variable-vs- variable comparison is a carry chain where variable-vs-CONSTANT folds into a couple of LUTs -- precisely what making the bounds dynamic had thrown away. geom_ok guarantees bp < rhbp <= 0x0F, which makes the narrow slice safe. Now 26.90 and 28.50 MHz on seeds 1-2, against 28.00/29.21 for the build without the geometry change. Attribution needed an A/B: the register-space decode (item 4) landed in the same stretch and had never been swept on its own. Measured separately it closes 28.0-29.2 MHz and costs nothing, so the whole regression was the assembler. The critical-path report named rx_frame_assembler.py:215 -- the rrp clamp -- which is what pointed at the comparator width rather than at the arithmetic everyone would suspect first. Co-Authored-By: Claude Opus 5 --- docs/bba_compat_todo.md | 28 ++++++++++++++++++ exi_bba/rx_frame_assembler.py | 56 ++++++++++++++++++++++++++--------- 2 files changed, 70 insertions(+), 14 deletions(-) diff --git a/docs/bba_compat_todo.md b/docs/bba_compat_todo.md index 0a4fe13..0b977f1 100644 --- a/docs/bba_compat_todo.md +++ b/docs/bba_compat_todo.md @@ -87,6 +87,34 @@ fallback layout still stores). Mutation-tested: wrapping at the hardcoded page 15 writes pages 9–11 and fails T7; removing the sanity check fails all four T7b assertions. +⚠️ **This change cost a 0/8 timing failure before it was tuned, and the shape +of that is worth remembering.** The first version computed the sanitised +bounds combinationally, which put + +``` +bp/rhbp -> comparators -> Mux -> subtract -> pg_total -> add -> raw + -> compare -> free -> compare -> have_room +``` + +in ONE combinational path where it had previously been constant-folded. `clk` +fmax collapsed **29 → 17 MHz and all eight seeds failed** the 24 MHz target. +Two things were needed: + +1. **Register the bounds** (`pg_first`/`pg_last`/`pg_total`, and `rrp_s`). + They change only when the GC writes `BP`/`RHBP`/`RRP`, so a cycle of + staleness costs nothing, and `free` already reaches the FSM through the + registered `have_room`. That alone got it to ~20 MHz — still failing. +2. **Narrow them to 4 bits.** Ring pages are 1–15. An 8-bit + variable-vs-variable comparison is a carry chain, where + variable-vs-CONSTANT folds into a couple of LUTs — which is exactly what + was lost by making the bounds dynamic. `geom_ok` guarantees + `bp < rhbp <= 0x0F`, which is what makes the narrow slice safe. + +Result: 26.9–28.5 MHz on the two sampled seeds, against 28.0–29.2 for the +build without this change. **Always sweep after touching the assembler's +ring arithmetic** — and note the register-space decode (item 4) was measured +separately and cost nothing, so attributing a regression needs the A/B. + For the record, libogc2 writes `TLBP = 0x0000`, `BP = 0x0001`, `RHBP = 0x000F` — exactly the old hardcoded layout, which is why this never showed up in practice. diff --git a/exi_bba/rx_frame_assembler.py b/exi_bba/rx_frame_assembler.py index 6a2e4c9..f69abfb 100644 --- a/exi_bba/rx_frame_assembler.py +++ b/exi_bba/rx_frame_assembler.py @@ -182,17 +182,41 @@ class RXFrameAssembler(Elaboratable): # garbage write (BP = 0, or RHBP below BP) would give a zero or # negative ring size and the assembler would write outside it. Page 0 # is reserved for the TX buffer, so BP must be at least 1. - geom_ok = Signal() - pg_first = Signal(8) - pg_last = Signal(8) - pg_total = Signal(5) - m.d.comb += [ - geom_ok.eq((self.bp >= _RX_PAGE_FIRST) - & (self.rhbp <= _RX_PAGE_LAST) - & (self.rhbp > self.bp)), - pg_first.eq(Mux(geom_ok, self.bp[0:8], _RX_PAGE_FIRST)), - pg_last .eq(Mux(geom_ok, self.rhbp[0:8], _RX_PAGE_LAST)), - pg_total.eq(pg_last - pg_first + 1), + # ⚠️ REGISTERED, not combinational. Computing these every cycle put + # bp/rhbp -> comparators -> Mux -> subtract -> pg_total -> add -> raw + # -> compare -> free -> compare -> have_room all in ONE combinational + # path, where it used to be constant-folded. `clk` fmax collapsed + # from ~29 MHz to ~17 MHz and ALL EIGHT SEEDS FAILED the 24 MHz + # target. bp/rhbp only change when the GC writes BP/RHBP, so one + # cycle of staleness costs nothing and splits the path in two. + # ⚠️ REGISTERED AND 4 BITS WIDE, both deliberately. + # + # Computing these combinationally put bp/rhbp -> comparators -> Mux -> + # subtract -> pg_total -> add -> raw -> compare -> free -> compare -> + # have_room in ONE path, where it used to be constant-folded. `clk` + # fmax collapsed from ~29 MHz to ~17 MHz and ALL EIGHT SEEDS FAILED + # the 24 MHz target. Registering split it, but 8-bit variable-vs- + # variable comparators are carry chains where variable-vs-CONSTANT + # folds into a couple of LUTs, so it was still only ~20 MHz. + # + # Ring pages are 1-15, so 4 bits is the natural width; geom_ok + # guarantees bp < rhbp <= 0x0F, which is what makes the narrow slice + # safe. bp/rhbp change only when the GC writes BP/RHBP, so a cycle of + # staleness costs nothing. + geom_ok = Signal() + geom_ok_c = Signal() + pg_first = Signal(4, init=_RX_PAGE_FIRST) + pg_last = Signal(4, init=_RX_PAGE_LAST) + pg_total = Signal(5, init=_PAGES_TOTAL) + m.d.comb += geom_ok_c.eq((self.bp >= _RX_PAGE_FIRST) + & (self.rhbp <= _RX_PAGE_LAST) + & (self.rhbp > self.bp)) + m.d.sync += [ + geom_ok .eq(geom_ok_c), + pg_first.eq(Mux(geom_ok_c, self.bp[0:4], _RX_PAGE_FIRST)), + pg_last .eq(Mux(geom_ok_c, self.rhbp[0:4], _RX_PAGE_LAST)), + pg_total.eq(Mux(geom_ok_c, self.rhbp[0:4] - self.bp[0:4] + 1, + _PAGES_TOTAL)), ] m.d.comb += self.geom_honoured.eq(geom_ok) @@ -200,10 +224,14 @@ class RXFrameAssembler(Elaboratable): # rwp == rrp means EMPTY, so writing the whole ring would wrap back to # "empty" and lose the lot. Usable = (rrp - rwp - 1) mod pg_total, # which is pg_total-1 when the ring is empty. - rrp_s = Signal(8) - m.d.comb += rrp_s.eq(Mux((self.rrp >= pg_first) + # rrp_s is REGISTERED too: the clamp is two comparisons against the + # (now variable) bounds, and the GC writes RRP about as often as it + # writes BP/RHBP. `free` already reaches the FSM through a register + # (`have_room`), so this adds no behavioural latency that matters. + rrp_s = Signal(4, init=_RX_PAGE_FIRST) + m.d.sync += rrp_s.eq(Mux((self.rrp >= pg_first) & (self.rrp <= pg_last), - self.rrp, pg_first)) + self.rrp[0:4], pg_first)) raw = Signal(5) free = Signal(5) with m.If(rrp_s >= rwp):