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 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
15 writes pages 9–11 and fails T7; removing the sanity check fails all four
|
||||||
T7b assertions.
|
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`,
|
For the record, libogc2 writes `TLBP = 0x0000`, `BP = 0x0001`,
|
||||||
`RHBP = 0x000F` — exactly the old hardcoded layout, which is why this never
|
`RHBP = 0x000F` — exactly the old hardcoded layout, which is why this never
|
||||||
showed up in practice.
|
showed up in practice.
|
||||||
|
|||||||
@@ -182,17 +182,41 @@ class RXFrameAssembler(Elaboratable):
|
|||||||
# garbage write (BP = 0, or RHBP below BP) would give a zero or
|
# 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
|
# 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.
|
# is reserved for the TX buffer, so BP must be at least 1.
|
||||||
geom_ok = Signal()
|
# ⚠️ REGISTERED, not combinational. Computing these every cycle put
|
||||||
pg_first = Signal(8)
|
# bp/rhbp -> comparators -> Mux -> subtract -> pg_total -> add -> raw
|
||||||
pg_last = Signal(8)
|
# -> compare -> free -> compare -> have_room all in ONE combinational
|
||||||
pg_total = Signal(5)
|
# path, where it used to be constant-folded. `clk` fmax collapsed
|
||||||
m.d.comb += [
|
# from ~29 MHz to ~17 MHz and ALL EIGHT SEEDS FAILED the 24 MHz
|
||||||
geom_ok.eq((self.bp >= _RX_PAGE_FIRST)
|
# target. bp/rhbp only change when the GC writes BP/RHBP, so one
|
||||||
& (self.rhbp <= _RX_PAGE_LAST)
|
# cycle of staleness costs nothing and splits the path in two.
|
||||||
& (self.rhbp > self.bp)),
|
# ⚠️ REGISTERED AND 4 BITS WIDE, both deliberately.
|
||||||
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)),
|
# Computing these combinationally put bp/rhbp -> comparators -> Mux ->
|
||||||
pg_total.eq(pg_last - pg_first + 1),
|
# 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)
|
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
|
# 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,
|
# "empty" and lose the lot. Usable = (rrp - rwp - 1) mod pg_total,
|
||||||
# which is pg_total-1 when the ring is empty.
|
# which is pg_total-1 when the ring is empty.
|
||||||
rrp_s = Signal(8)
|
# rrp_s is REGISTERED too: the clamp is two comparisons against the
|
||||||
m.d.comb += rrp_s.eq(Mux((self.rrp >= pg_first)
|
# (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_last),
|
||||||
self.rrp, pg_first))
|
self.rrp[0:4], pg_first))
|
||||||
raw = Signal(5)
|
raw = Signal(5)
|
||||||
free = Signal(5)
|
free = Signal(5)
|
||||||
with m.If(rrp_s >= rwp):
|
with m.If(rrp_s >= rwp):
|
||||||
|
|||||||
Reference in New Issue
Block a user