# Code Review: osmarks/ipu-stack A from-scratch Rust compiler, `.ipuexe` package format, Linux driver, and host runtime for Graphcore IPU21 (C600 / Colossus) accelerators that bypasses Poplar entirely. Poplar is used only as an offline kernel compiler (`popc`) and as a reverse-engineering oracle: SDK-produced tile ELFs are disassembled to recover the exchange-fabric sync sequences, XREQ routing bitmaps, and host-exchange packet encodings, which are then reproduced independently and checked against a live C600. Per the author, the stack is fully agent-written by a GPT-5.6 "Sol" instance: 248 commits over six days, roughly 35k lines of Rust plus hand-written Colossus assembly, one author. The notable claim is not the line count but that an agent recovered an undocumented proprietary accelerator ABI and landed it on real hardware. ## Verdict Genuinely impressive and mostly well-engineered. Nine focused crates, a forward-compatible Cap'n Proto package with content-addressed blob dedup, structured `tracing` logging, an independent C++ oracle for exchange-plan parity, and seeded randomized hardware tests. The defect pattern is consistent and diagnostic: local, oracle-checkable correctness is high, while cross-boundary invariants, untrusted-input handling, and buffer-extent reasoning are thin. That is the profile you would predict for strong agent-written systems code. It is rigorous where a hard local pass/fail signal exists (relocation tables checked against SDK vectors, instruction encoders with golden bytes, a numeric format with a checkable boundary), and soft where correctness depends on a contract spanning two files, an unmodeled hardware peer, or an adversarial input. ## Correctness findings, by severity ### Real or latent bugs 1. **Attention `scores`/PV buffer over-reservation (HIGH).** `scores` is reserved and interleaved-SRAM-bounds-checked at `query_rows x key_block_columns x 2` (`attention.rs:681,688`), but the PV kernel takes `task.scores` as its output with shape `[query_rows, key_block_columns, padded_head_dimension]` (`attention.rs:984,992`), writing a `query_rows x padded_head_dimension` product. Whenever `padded_head_dimension > key_block_columns` the write overruns the reservation, and the guard at `:688` measures the smaller width so it cannot catch it. SigLIP head-dim 72 (padded 80) against a 16-column key block is a 5x overrun. Invisible today only because `scores` sits alone at the interleaved base, so overflow lands in free SRAM instead of a neighbor. 2. **`blob_offset` overflow validate-bypass.** `segment.blob_offset + u64::from(segment.file_size) > blobs[..].len()` uses a plain `+` (`package/lib.rs:467`) one line below a correct `checked_add` on the address (`:459`). With release overflow checks off (no `[profile.release]` override), `blob_offset = u64::MAX, file_size = 1` wraps to 0, passes `validate()`, and then `tile_image` slices `bytes[usize::MAX..usize::MAX+1]` and panics (`:739-742`). 3. **`add_f16.S` missing zero-worker guard.** Unlike `add_bias_f16.S:38`, `gelu_relayout_f16.S:70`, and `relayout_f16.S:33`, the `add_f16` worker (`add_f16.S:28-54`) has no `brz` on its element count. A lane assigned zero elements does `sub; brnz` into a roughly four-billion-iteration store loop: memory corruption or hang. Latent for small buffers. 4. **`rowwise` multi-pass staging offset conflates coordinate with position.** `staging = EXCHANGE_WINDOW_BASE + (block.block_column - first_block as u16) * block_bytes` (`rowwise.rs:460`) is correct only because `block_column` equals the sorted pass position, which holds for zero-based contiguous 64-column blocks. Any non-contiguous column offset underflows the `u16` into a wild address, with no exchange-window bound check (its siblings at `lib.rs:1159, 1325` have one). Found by both review passes. 5. **`gemm_f32` repeat-count underflow.** `rpt $m4` with `$m4 = rows_per_worker - 2` (`gemm_f32_64_amp.S:214,298`) and no zero-row guard is safe only when every worker gets at least two rows, i.e. total rows a multiple of 6 and at least 12. The default `GEMM_SMALL_ROWS=12` is safe; `rows = 6` or `7` underflows catastrophically. Invariant undocumented and unasserted. 6. **`project_f143` computes the scale factor in the weight dtype.** `factor = 2^-scale` is evaluated in `values.dtype` (`quantize_siglip_f143.py:169`); with `scale` in `[-32, 31]` this overflows or underflows in fp16, and the `nearest` path passes weight-dtype tensors, so an fp16/bf16 checkpoint yields inf/0 weights. The GPTQ path is safe because it upcasts with `.float()`. Fix is to `.float()` the projection unconditionally. 7. **`static_runtime.S:87` uses `exitnz $m15` where every other worker exits `exitz $m15`.** With `m15 = 0` the nonzero-exit condition is not satisfied. Anomalous and likely a bug; unassemblable in review, so flagged for confirmation against arch semantics. 8. **Unvalidated-tile `.unwrap()` panics.** `topology.physical(...).unwrap()` in the public binding builders (`blocked_data.rs:44`, `siglip.rs:1085`) and the SigLIP weight/name lookups (`siglip.rs:530-532`) panic on out-of-range tiles or missing tensors rather than returning `Result`. `siglip.rs` has zero offline tests. ### Soundness and unsafe 9. **DMA aliasing.** `HostBuffer::bytes`/`bytes_mut` hand out `&[u8]`/`&mut [u8]` over host memory the device DMAs into and out of, accessed via non-volatile `copy_from_slice` and ordered only by `fence(SeqCst)` (`driver/lib.rs:817-823`, used at `:961,1436,1471`). The device is an invisible second writer, so this is UB under Rust's aliasing model even though it works in practice. Should be `UnsafeCell` plus volatile access. Found by both passes. 10. **No register-level device lock.** `read_config`/`write_config` mutate MMIO through a shared raw pointer behind `&self` (`driver/lib.rs:303,308`); safety rests entirely on `Device` being `!Send + !Sync` and single-thread-owned. The README's "in-process lock" is only the test-harness `static Mutex<()>`, not register serialization. Sound today, worth stating as an explicit contract. 11. **ioctl `_IOC` size mismatch.** `BUFFER_ATTACH` and `MAILBOX_WRITE_READ` encode size 8 (`driver/lib.rs:208,212`) while `AttachBufferData` and `MailboxArgument` are both 16 bytes (`:245-262`). Almost certainly correct by reverse-engineering against a driver that demonstrably loads, but it is an unaudited layout assumption with no `static_assert` or comment. The most fragile FFI point. ### Untrusted-input robustness 12. **capnp traversal limit disabled.** `traversal_limit_in_words(None)` (`package/lib.rs:21`) removes capnp's amplification guard for all three readers (application, profile, memory). Wrong default for an untrusted `.ipuexe`. [osmarks: this is deliberate, the ipuexes are trusted] 13. **zstd allocation bomb.** `zstd::bulk::decompress(stored, item.get_uncompressed_size() as usize)` (`package/lib.rs:660`) sizes the output buffer from an attacker-controlled field with no cap, pre-allocating before the SHA check runs. A tiny blob can demand a multi-gigabyte allocation. [same thing] 14. **Validation blind spots.** `validate()` (`package/lib.rs:436-561`) never checks `entry_point`/`command_address`, device-config-write offsets, or `file_offset` fields, and the profile and memory-profile readers validate only the schema version. A malformed-but-plausible package with an out-of-range entry point passes. ### Fragile invariants and readability traps (correct today, unguarded) 15. **Silent-masking encoder helpers.** `delay`/`delay_pic`/`delay_xpic`/`send_off` (`exchange/lib.rs:1157-1189`) mask overflow and return raw `u32`, unlike the validating `encode_*` family. Correctness hinges on `MAX_TRANSFER_WORDS=4148` and c600 physical-tile bounds that `Topology::new` does not enforce (`:784`), so a custom topology silently corrupts encodings. Several inputs sit exactly at field maxima by design (`count-53 = 4095` into the 12-bit `delay_xpic`), which is elegant but undefended. [I suppose you should maybe normalize/unify these.] 16. **Two `logicalPairForPhysical` formulas, written differently, computing the identical permutation** `[0,4,1,5,2,6,3,7]` (`flash_attention_f16.cpp:65`, `attention_pack_f16.cpp:18`). Consistent and correct, but the divergent unexplained formulas invite a "cleanup" that would break the pairing. [sure, fix this] 17. **`HEAD_DIMENSION` divergent defaults** across the three attention files (`flash_attention_f16.cpp:5` = 64, `attention_pack_f16.cpp` = 72/80, `attention_unpack_f16.cpp:7` = 64). Safe only if the build `-D`s them consistently. 18. **`AllocationOccupancyCache`** keys on `schedule as *const _ as usize` plus a monotonic counter, with a hand-rolled `PartialEq` that always returns true (`compiler/lib.rs:426-583`). A dropped-then-reallocated `Schedule` at the same address is a stale-cache hazard. 19. **F143 scale granularity is unenforced end to end.** The tool derives one scale per `block_size x block_size` tile (`quantize_siglip_f143.py:159`) and saves dequantized fp weights, while the device expander applies one `signed char` scale per contiguous invocation (`expand_f8_f143_to_f16.cpp:13-27`). Nothing in-repo ties the tiling to the device layout, so the contract is implicit and unverified. Highest silent-accuracy risk in the quantizer. ## What is genuinely excellent - **`ipu-elf` linker and relocator.** Exhaustive per-relocation width, alignment, and overflow checks, negative-value rejection, bounds-checked reads and writes, oracle-tested. Both passes named it the model the rest of the stack should follow. - **Exchange DSATUR scheduler and staging dataflow** (`compiler/lib.rs:2815`). Deterministic coloring with explicit tie-breaks, relay-aware staging-source selection, coalesced multicast, all seeded-fuzz-tested for determinism. The strongest subsystem. - **Package content-addressing and the deterministic build digest.** SHA-256 over uncompressed bytes, re-verified on read; a manual length-prefixed digest that iterates only `Vec`s and contains no floats, so it is order-stable and reproducible. - **Public `encode_*` primitives** with bounds checks and golden byte vectors (`exchange/lib.rs`), and the `import_ipuimg` legacy path with airtight `checked_add`/`.get()` bounds throughout: proof the author can write untrusted parsing when the layer is treated as untrusted. - **F143 numeric core.** Subnormal handling, mantissa round-with-carry, and a provably continuous subnormal/normal boundary at `2^-7`. - **Double-emit size reconciliation** (`runtime/lib.rs:2203`): a preliminary sizing pass reconciled against the final emit, a strong defensive check around an otherwise dangerous reserve-then-fill design. ## Testing reality Offline unit tests are real but narrow: encodings, allocation, lowering, package structure, and oracle vectors. Everything end to end is hardware-gated. `hardware_e2e.rs` has no `#[ignore]` or feature gate, and `device()` merely locks a mutex with no probe before the child binaries `unwrap()` `Device::open`, so `cargo test` hard-fails wholesale without `/dev/ipu0`. There is no simulator or mock. Critically, no offline test asserts the bytes `emit()` produces: the codegen composition, template patching, SigLIP encoder, and host protocol have hardware-only confidence. "Tests pass" is meaningless off-hardware. [osmarks: I'm fine with this for these purposes.] ## Bottom line As a capability demonstration this is remarkable, and the core is more rigorous than "vibe-coded" implies. As software it is a research artifact. Trust the oracle-anchored layers (ELF linking, exchange encoding, package integrity). Treat the untrusted-input surface (package readers) and the buffer-extent and aliasing paths (attention `scores`, `HostBuffer`, the silent-masking encoders under any non-c600 topology) as unproven. Highest-value fixes, in order: the attention `scores`/PV reservation, the `blob_offset` `checked_add`, the capnp traversal limit and a zstd size cap, the `add_f16` and `gemm_f32` low-row guards, and gating `hardware_e2e` behind a feature so offline CI means something. [I'm not going to run CI and this is fine if it can run the model I want.] --- ## Appendix: review method and model comparison The review ran two independent passes of five subagents each, one slice per crate group (compiler; driver/exchange/elf; runtime/codegen; package/profile/cli; device kernels and the F143 quantizer). The first pass ran on Claude Opus 4.8, the second on Claude Fable 5, with identical prompts. Every divergence between the two was then verified by hand against the source. On this codebase Fable materially outperformed Opus. It found the most severe defect (item 1) that Opus missed, found a real release-mode bug (item 2) that Opus rated safe, caught the ioctl size mismatch (item 11) that Opus described as "correctly repr(C)", and correctly declined a false positive that Opus raised (a claimed 10-bit `delay_xpic` truncation; the field is 12 bits and the fed values sit exactly at its maximum). It also proved the two `logicalPairForPhysical` formulas identical rather than leaving a vague worry, and independently found the `add_f16` hang, the `exitnz $m15` anomaly, and the `project_f143` fp16 overflow. Opus surfaced nothing that Fable missed. | Finding | Opus | Fable | Verified verdict | |---|---|---|---| | Attention `scores`/PV over-reservation (`attention.rs:681`/`992`) | missed | high-severity latent OOB | Fable right | | `blob_offset` overflow validate-bypass (`package/lib.rs:467`) | rated safe | real release-mode panic | Fable right | | ioctl `_IOC` size 8 vs 16-byte structs (`driver/lib.rs:207-262`) | "correctly repr(C)" | caught mismatch | Fable right | | `delay_xpic` field width (`exchange/lib.rs:1166`) | flagged 10-bit truncation | 12-bit, fits exactly | Fable right (Opus false positive) | This is a single trial with identical prompts, so it is not a general verdict on the two models. On this particular hard-systems review, the gap was clear.