#115 - rtl: InterGemminiXbar v1 — N×N decoupled crossbar with round-robin arbitration - Fluid/fluidpop-v1 - Git PopSolutions: Pop.Coop

navigator commented

2026-05-26 18:18:36 -03:00

Owner

Materialises the v1 body of InterGemminiXbar per the Draft SPEC and the issue acceptance criteria.

Scope

class InterGemminiXbar(n: Int = 4, payloadBits: Int = 32, fifoDepth: Int = 4) — n tracks PopSoCConfig.TileCountKey (ADR-001), rejected at elaboration if n <= 0 per SPEC §Invariants.
Payload bundle: tileId = UInt(log2Ceil(n).W) + opaque payload = UInt(payloadBits.W), annotated Open per SPEC §Interface (concrete shape pending PopRoCCRouter cluster-side attach decision).
Per-ingress FIFO of depth fifoDepth (Open per SPEC §Behavior — default 4, annotated).
Dimension-ordered routing: each ingress FIFO head packet is offered only to the egress identified by bits.tileId.
Round-robin arbitration at every egress via Chisel RRArbiter — fairness under uniform traffic plus the no-deadlock invariant under any single-egress back-pressure.
Per-source ordering preserved per SPEC §Invariants; cross-source ordering not preserved (the crossbar is non-blocking, not a total-order network).

Tests

rtl/tests/InterGemminiXbar/InterGemminiXbarSpec.scala extended with the three scenarios required by the issue, each parameterised on N ∈ {2, 4}:

All N×N (ingress, egress) pairs route correctly with payload integrity.
Sustained back-pressure on egress 0 does not block egress 1 (no-deadlock witness).
Two ingress streams contending for one egress arbitrate fairly under round-robin (skew ≤ 2 across 8 fires).

sbt -no-colors test green — 20/20 suites.

Knock-on

MultiGemminiCluster already instantiates new InterGemminiXbar; its IO bundle is no longer empty, so the cluster skeleton ties xbar.io.ingress to DontCare and xbar.io.egress(_).ready to false. Cluster ↔ xbar wiring lands when MultiGemminiCluster.SPEC.md is lifted from stub.

Open Questions (untouched, as instructed)

Payload bundle shape — Open per SPEC §Interface (parameterised + commented).
FIFO depth — Open per SPEC §Behavior (default 4, annotated).
Pro 16×16 area trade-off — Open. n already parameterises everything; no v1 changes needed for Pro.

Refs

SPEC: rtl/src/pop/specs/InterGemminiXbar.SPEC.md
PLAN.md §8.2, §12.3; ADR-003 (4×4 xbar), ADR-001 (4 / 16 Gemminis).

Closes #112

Materialises the v1 body of `InterGemminiXbar` per the Draft SPEC and the issue acceptance criteria. ## Scope - `class InterGemminiXbar(n: Int = 4, payloadBits: Int = 32, fifoDepth: Int = 4)` — `n` tracks `PopSoCConfig.TileCountKey` (ADR-001), rejected at elaboration if `n <= 0` per SPEC §Invariants. - Payload bundle: `tileId = UInt(log2Ceil(n).W)` + opaque `payload = UInt(payloadBits.W)`, annotated Open per SPEC §Interface (concrete shape pending PopRoCCRouter cluster-side attach decision). - Per-ingress FIFO of depth `fifoDepth` (Open per SPEC §Behavior — default 4, annotated). - Dimension-ordered routing: each ingress FIFO head packet is offered only to the egress identified by `bits.tileId`. - Round-robin arbitration at every egress via Chisel `RRArbiter` — fairness under uniform traffic plus the no-deadlock invariant under any single-egress back-pressure. - Per-source ordering preserved per SPEC §Invariants; cross-source ordering not preserved (the crossbar is non-blocking, not a total-order network). ## Tests `rtl/tests/InterGemminiXbar/InterGemminiXbarSpec.scala` extended with the three scenarios required by the issue, each parameterised on N ∈ {2, 4}: 1. All N×N (ingress, egress) pairs route correctly with payload integrity. 2. Sustained back-pressure on egress 0 does not block egress 1 (no-deadlock witness). 3. Two ingress streams contending for one egress arbitrate fairly under round-robin (skew ≤ 2 across 8 fires). `sbt -no-colors test` green — 20/20 suites. ## Knock-on `MultiGemminiCluster` already instantiates `new InterGemminiXbar`; its IO bundle is no longer empty, so the cluster skeleton ties `xbar.io.ingress` to DontCare and `xbar.io.egress(_).ready` to false. Cluster ↔ xbar wiring lands when `MultiGemminiCluster.SPEC.md` is lifted from stub. ## Open Questions (untouched, as instructed) - Payload bundle shape — Open per SPEC §Interface (parameterised + commented). - FIFO depth — Open per SPEC §Behavior (default 4, annotated). - Pro 16×16 area trade-off — Open. `n` already parameterises everything; no v1 changes needed for Pro. ## Refs - SPEC: `rtl/src/pop/specs/InterGemminiXbar.SPEC.md` - PLAN.md §8.2, §12.3; ADR-003 (4×4 xbar), ADR-001 (4 / 16 Gemminis). Closes #112

navigator added 1 commit

2026-05-26 18:18:37 -03:00

rtl: InterGemminiXbar v1 — N×N decoupled crossbar with round-robin arbitration

build / scalafmt-check (pull_request) Successful in 5s

Details

build / sbt-compile (pull_request) Successful in 4s

Details

build / shell-lint (pull_request) Successful in 1m13s

Details

9faf5a1a10

Materialise the v1 body per SPEC §Interface / §Behavior:
- Parameterised on N (default 4 per ADR-001/ADR-003), opaque payload
  width (default 32), and per-ingress FIFO depth (default 4 — Open
  per SPEC §Behavior).
- Per-ingress Queue preserves per-source ordering (SPEC §Invariants);
  dimension-ordered routing presents each head packet only to its
  destination egress; one RRArbiter per egress provides round-robin
  fairness and keeps each egress drainable independently of the others
  (no-deadlock under any single-egress back-pressure).
- Rejects N <= 0 at elaboration.

Extend the chiseltest bench to the three scenarios mandated by the
acceptance criteria — all N×N point-to-point pairs, sustained
back-pressure on egress 0 leaving egress 1 live, and round-robin
fairness between two ingress sources contending for one egress — each
parameterised on N ∈ {2, 4}. `sbt -no-colors test` green
(20/20 suites).

Tie off the now-non-empty xbar.io ports in MultiGemminiCluster
(DontCare on ingress, ready=0 on egress) so the cluster skeleton keeps
elaborating; concrete cluster ↔ xbar wiring lands when the cluster
SPEC is lifted from stub.

Closes #112

navigator added the

ready-for-review

label

2026-05-26 18:18:47 -03:00

navigator commented

2026-05-26 18:22:25 -03:00

Author

Owner

VERDICT: FINDINGS

PR #115 lifts InterGemminiXbar from elaborate-only into a v1 N×N decoupled crossbar with per-ingress FIFOs, in-band tile-id routing, and per-egress RRArbiter arbitration — solid implementation work overall. Mechanical guardrails pass: SPDX headers preserved (CHARRUA-1.2 source, AGPL-3.0-or-later test), package pop everywhere, scope bounded (three files: source + test + a minimal MultiGemminiCluster knock-on that wires xbar.io.ingress := DontCare and ties xbar.io.egress(_).ready := false.B — necessary because the IO bundle is no longer empty), no AI attribution. The tests exercise the three SPEC-invariant-tied scenarios from Issue #112: point-to-point routing for all N×N pairs, no-deadlock under egress 0 back-pressure, and round-robin fairness under contention with a documented skew bound. The XbarPacket design with opaque payload and explicit tileId is a defensible v1 — the SPEC's "Address-range routing table" §Interface category is implicitly handled by pushing address-decode upstream into the producer rather than committing to a CSR shape inside the xbar (which is consistent with the SPEC's Open question on programming discipline "matching whichever CSR fabric MultiGemminiCluster exposes", and the cluster CSR is itself still Open). One concrete blocking issue: the elaboration-time bound check doesn't match the SPEC §Invariants commitment.

Findings

Elaboration check allows N outside SPEC §Invariants [4, 16] range — rtl/src/pop/InterGemminiXbar.scala require(n > 0, ...) — The SPEC §Invariants "Crossbar size tracks active PopSoCConfig" commits explicitly to Elaboration must reject any N outside this range where the range is [4, 16] per ADR-003 Consequences ("Crossbar grows quadratically with N; manageable at 4-16"). The PR's require(n > 0, ...) enforces only the floor with the attribution "per SPEC §Invariants", but the SPEC §Invariants commits to the full [4, 16] bound — so the v1 silently allows n = 1, 2, 3, 17, … to elaborate, contradicting the SPEC. The test suite parameterises on Ns = Seq(2, 4), which is presumably why the require was relaxed: under a strict [4, 16] check, the N=2 scenarios would fail to elaborate. This means the tests at N=2 are passing against an implementation the SPEC says cannot elaborate. Either the implementation should match the SPEC, or the SPEC should be updated first via a separate spec-designer PR. Suggested fix: require(n >= 4 && n <= 16, s"InterGemminiXbar N must be in [4, 16] per SPEC §Invariants / ADR-003 Consequences (got $n)") and change the test parameterisation from Ns = Seq(2, 4) to Ns = Seq(4) (or Seq(4, 8, 16) if intermediate values are intentionally valid). If Issue #112's "N ∈ {2, 4}" acceptance criterion is in tension with the SPEC's [4, 16] bound, that tension belongs in a SPEC-update PR (the spec-designer role), not silently absorbed into a relaxed require in the implementation.

**VERDICT:** FINDINGS PR #115 lifts `InterGemminiXbar` from elaborate-only into a v1 N×N decoupled crossbar with per-ingress FIFOs, in-band tile-id routing, and per-egress `RRArbiter` arbitration — solid implementation work overall. Mechanical guardrails pass: SPDX headers preserved (`CHARRUA-1.2` source, `AGPL-3.0-or-later` test), `package pop` everywhere, scope bounded (three files: source + test + a minimal MultiGemminiCluster knock-on that wires `xbar.io.ingress := DontCare` and ties `xbar.io.egress(_).ready := false.B` — necessary because the IO bundle is no longer empty), no AI attribution. The tests exercise the three SPEC-invariant-tied scenarios from Issue #112: point-to-point routing for all N×N pairs, no-deadlock under egress 0 back-pressure, and round-robin fairness under contention with a documented skew bound. The `XbarPacket` design with opaque `payload` and explicit `tileId` is a defensible v1 — the SPEC's "Address-range routing table" §Interface category is implicitly handled by pushing address-decode upstream into the producer rather than committing to a CSR shape inside the xbar (which is consistent with the SPEC's Open question on programming discipline "matching whichever CSR fabric MultiGemminiCluster exposes", and the cluster CSR is itself still Open). One concrete blocking issue: the elaboration-time bound check doesn't match the SPEC §Invariants commitment. ## Findings - **Elaboration check allows N outside SPEC §Invariants [4, 16] range** — `rtl/src/pop/InterGemminiXbar.scala` `require(n > 0, ...)` — The SPEC §Invariants "Crossbar size tracks active `PopSoCConfig`" commits explicitly to `Elaboration must reject any N outside this range` where the range is `[4, 16]` per ADR-003 Consequences ("Crossbar grows quadratically with N; manageable at 4-16"). The PR's `require(n > 0, ...)` enforces only the floor with the attribution "per SPEC §Invariants", but the SPEC §Invariants commits to the full `[4, 16]` bound — so the v1 silently allows `n = 1, 2, 3, 17, …` to elaborate, contradicting the SPEC. The test suite parameterises on `Ns = Seq(2, 4)`, which is presumably why the require was relaxed: under a strict `[4, 16]` check, the N=2 scenarios would fail to elaborate. This means the tests at N=2 are passing against an implementation the SPEC says cannot elaborate. Either the implementation should match the SPEC, or the SPEC should be updated first via a separate spec-designer PR. Suggested fix: `require(n >= 4 && n <= 16, s"InterGemminiXbar N must be in [4, 16] per SPEC §Invariants / ADR-003 Consequences (got $n)")` and change the test parameterisation from `Ns = Seq(2, 4)` to `Ns = Seq(4)` (or `Seq(4, 8, 16)` if intermediate values are intentionally valid). If Issue #112's "N ∈ {2, 4}" acceptance criterion is in tension with the SPEC's [4, 16] bound, that tension belongs in a SPEC-update PR (the spec-designer role), not silently absorbed into a relaxed `require` in the implementation.

navigator added the

review:findings

label

2026-05-26 18:22:25 -03:00

navigator commented

2026-05-26 20:28:38 -03:00

Author

Owner

No description provided.

navigator removed the

review:findings

label

2026-05-26 20:37:36 -03:00

navigator added 1 commit

2026-05-26 20:37:38 -03:00

rtl(InterGemminiXbar): tighten N bound to [4, 16] per SPEC §Invariants

build / scalafmt-check (pull_request) Successful in 3s

Details

build / sbt-compile (pull_request) Successful in 3s

Details

build / shell-lint (pull_request) Successful in 47s

Details

5234d0b38e

Reviewer flagged that the v1 require allowed n > 0, contradicting SPEC
§Invariants which pins N to [4, 16] per ADR-003 Consequences (crossbar
grows quadratically, manageable at 4-16). The relaxed bound let test
parameterisation use N=2, which the SPEC says cannot elaborate.

Tighten require to [4, 16]; change test Ns from {2, 4} to {4, 16} so
both ends of the SPEC bound are exercised. The original {2, 4} from
issue #112 contradicted the SPEC and is dropped.

navigator commented

2026-05-26 20:38:56 -03:00

Author

Owner

VERDICT: PASS

PR #115 has been resubmitted at a new head SHA (5234d0b3... vs the prior 9faf5a1a...) with my earlier finding directly addressed. The require constraint has been strengthened from require(n > 0, ...) to require(n >= 4 && n <= 16, s"InterGemminiXbar N must be in [4, 16] per SPEC §Invariants (ADR-003 Consequences: crossbar grows quadratically, manageable at 4-16) (got $n)") — exactly matching the SPEC §Invariants commitment that "Elaboration must reject any N outside this range" where the range is [4, 16]. The test parameterisation has been updated from Ns = Seq(2, 4) to Ns = Seq(4, 16) with explicit inline justification: SPEC §Invariants pins N ∈ [4, 16] (ADR-003 Consequences: quadratic area, manageable at 4-16). Issue #112 originally asked for N=2 in tests, but that contradicts the SPEC; tests cover N=4 (Edu floor) and N=16 (Pro ceiling) so the elaboration bound is exercised at both ends. This is exactly the fix-suggestion shape I proposed and it correctly resolves the issue-vs-SPEC tension in the SPEC's favor (the correct precedence — Issue acceptance criteria should not override SPEC §Invariants). Testing at both the floor (N=4) and the ceiling (N=16) gives stronger coverage than the prior N ∈ {2, 4} ever did — the Pro 16×16 elaboration path is now exercised by all three scenarios (point-to-point, back-pressure isolation, round-robin fairness). The rest of the mechanical guardrails remain clean: SPDX headers preserved (CHARRUA-1.2 source, AGPL-3.0-or-later test), package pop everywhere, no AI/Anthropic attribution, scope still bounded to three files (source + test + MultiGemminiCluster knock-on), no Chisel module rules violated. The XbarPacket in-band tileId design, the per-ingress FIFO architecture, the per-egress RRArbiter, and the per-source ordering preservation all remain unchanged from the prior version — those parts were already strong on the first review.

Findings

None.

**VERDICT:** PASS PR #115 has been resubmitted at a new head SHA (`5234d0b3...` vs the prior `9faf5a1a...`) with my earlier finding directly addressed. The `require` constraint has been strengthened from `require(n > 0, ...)` to `require(n >= 4 && n <= 16, s"InterGemminiXbar N must be in [4, 16] per SPEC §Invariants (ADR-003 Consequences: crossbar grows quadratically, manageable at 4-16) (got $n)")` — exactly matching the SPEC §Invariants commitment that "Elaboration must reject any N outside this range" where the range is `[4, 16]`. The test parameterisation has been updated from `Ns = Seq(2, 4)` to `Ns = Seq(4, 16)` with explicit inline justification: `SPEC §Invariants pins N ∈ [4, 16] (ADR-003 Consequences: quadratic area, manageable at 4-16). Issue #112 originally asked for N=2 in tests, but that contradicts the SPEC; tests cover N=4 (Edu floor) and N=16 (Pro ceiling) so the elaboration bound is exercised at both ends.` This is exactly the fix-suggestion shape I proposed and it correctly resolves the issue-vs-SPEC tension in the SPEC's favor (the correct precedence — Issue acceptance criteria should not override SPEC §Invariants). Testing at both the floor (N=4) and the ceiling (N=16) gives stronger coverage than the prior N ∈ {2, 4} ever did — the Pro 16×16 elaboration path is now exercised by all three scenarios (point-to-point, back-pressure isolation, round-robin fairness). The rest of the mechanical guardrails remain clean: SPDX headers preserved (`CHARRUA-1.2` source, `AGPL-3.0-or-later` test), `package pop` everywhere, no AI/Anthropic attribution, scope still bounded to three files (source + test + MultiGemminiCluster knock-on), no Chisel module rules violated. The `XbarPacket` in-band `tileId` design, the per-ingress FIFO architecture, the per-egress `RRArbiter`, and the per-source ordering preservation all remain unchanged from the prior version — those parts were already strong on the first review. ## Findings _None._

navigator added the

review:pass

label

2026-05-26 20:38:56 -03:00