Revert "feat(standalone): emit FP32/rv32imf platform kernels for Stage-2 prob…"

[jizhenjun]

Reverts #98

[github-actions]

🤖 AI Code Review

共审查 3 个变更文件

📁 scratchv/standalone/onnx_to_riscv_standalone.py

🔴 ABI violation: callee-saved registers not preserved — _emit_platform_conv:
Uses s0–s11 without save/restore at entry/exit. RISC-V ABI requires cnn_entry to preserve s-regs if it's a normal call. SpMM also uses s2–s6. If the platform calls this via jal from managed code, the caller's state is corrupted. Either emit addi sp,-N / sd s*, ... prologue+epilogue, or document that this is bare-metal/interrupt-context only (like the old FWHT).

🔴 Breaking ABI change in Conv — _emit_platform_conv:
Old: a0 = self-describing [batch,H,W,Cin,Cout,K] + NHWC data + OIHW weights inline.
New: a0 = raw NCHW feature map, a2 = param block, w_ptr/b_ptr as extra fields.
Any caller emitting the old layout (including the rest of this file's _gen_winogradconv path) will silently produce garbage. If this is intentional, the old format needs a compatibility shim or a version flag; otherwise this is a regression.

🔴 Numeric precision regression vs. oracle tolerance — both _emit_platform_conv and _emit_platform_spmm:
srai a5, a5, 16 truncates each tap's product before accumulation. With C_in·K² taps (e.g. 64·9=576 for conv, or up to nnz_per_row for SpMM), per-tap truncation error accumulates into a bias term. The old float32 path matched the oracle at rtol=1e-4; integer-truncated accumulation will not. Consider: (a) wider shift (srai by 10, keeping int22 range in the accumulator), (b) Kahan-like compensation, or (c) document the new tolerance.

🟡 SpMM accumulation is memory-bound — _emit_platform_spmm, _p_spmm_n loop:

lw   t4, 0(t3) ; add t4, t4, t2 ; sw t4, 0(t3)

Loads and stores the accumulator from memory on every n iteration. Keep the accumulator in a register across the inner loop, store once after the blt s4, a5 exit. This is O(nnz·N) memory traffic instead of O(M·N). Also makes the new _p_spmm_zero row-zeroing loop entirely redundant — drop it.

🟡 Dispatch regression: WinogradConv no longer maps to the dedicated kernel — emit_platform_asm:
op_type in ("Conv", "WinogradConv") → op_type == "Conv". Models with a WinogradConv node now fall through to the fixed-shape _emit_platform_listing, which assumes compile-time shapes. If Winograd was supposed to keep the size-independent path, restore the in (...) check; otherwise confirm the model builder no longer emits WinogradConv.

🟡 Docstring/code mismatch — emit_platform_asm:
Docstring says "currently FWHT" but the body still dispatches SpmmCsr and Conv. Either update the docstring to list all supported op types, or delete the now-unreachable branches.

💭 Nit: The shortened docstrings lose useful operational-order info ("matches platform fwht oracle bit-for-bit... butterfly order"). If the shift-to-integer change is intentional, the docstring should say so explicitly rather than just deleting the float32 wording — a future reader will assume the semantics are unchanged.

---

📁 tests/test_platform_kernels.py

🔴 Correctness risk: FWHT test quantization mismatch — The reference does a = [float(v)/Q for v in x] then runs butterflies in float64 and only quantizes at the very end with np.trunc(v*Q). This only equals an integer kernel if the kernel does pure int32 add/sub with no intermediate rounding. That's true for the plain FWHT butterflies, but the test now asserts got == ref (exact), so any future kernel change (e.g., rescale-per-stage, or srai in a butterfly) will silently break this. Consider adding a comment "kernel must be pure int32 add/sub, no rescale" or use np.allclose with a small tolerance to keep the test robust.

🟡 Conv test reference assumes bias-in-accumulator semantics — acc = int(bq[oc]) then add products. If the kernel adds bias at the end (or with a different truncation path), got == ref will fail. Add a comment pinning this contract, or restructure to compute both acc_no_bias and check got == acc_no_bias + int(bq[oc]) so the invariant is explicit.

🟡 Conv test coverage narrowed — Removed (3,6,6,5,8) (odd Hout when H=K=6, pad=2, K=5? Actually H=6,K=5,pad=2 → Hout=6) and (4,5,5,3,6). The removed cases exercised non-square kernels with mismatched cin/cout and smaller spatial dims. If the kernels assume cin==cout or power-of-two shapes, this hides regressions. Consider keeping at least one odd-shape / asymmetric case.

🟡 srli removed from interpreter but not audited — No change notes whether any kernel still emits srli (logical shift right, commonly needed for unsigned quantization or packing). If one does, _run will silently ignore it and the test will produce wrong got. Grep the generated asm (e.g., in a failing run) or add an else: raise AssertionError(f"unknown op {op}") branch — right now unknown ops are silently swallowed, which will produce cryptic got != ref failures.

🟡 Silent-op bug in _run — The final elif chain has no else. Any instruction not in the whitelist (srli, fadd.s, jalr, beq, andi, etc.) is a no-op that advances PC. That's how a kernel using srli/and would pass _parse but produce garbage output with no error. Add:

else:
    raise AssertionError(f"unimplemented RVC op: {op} at pc={pc}")

This is important because the diff shrinks the interpreter's supported ISA — future kernels will silently mis-test.

🟡 SPMM test drops nnz and changes ABI — Old: regs[10..12] = inp, out, nnz with a flat inp blob. New: regs[10..12] = block, out, par where block points to a descriptor and par is a param table. If a downstream consumer still builds the flat layout, this test no longer protects it. Fine if the kernel ABI actually changed in this PR, but worth confirming the ABI change is intentional and reflected in the kernel/model, not just the test.

🟡 Reference arithmetic order for SPMM/Conv — Reference does p = _s32((a * w) & 0xFFFFFFFF) >> 16 per-product, then accumulates with _s32. This matches a specific kernel pattern: (int32)(int16*int16) >> 16 then int32 add. Verify the kernel actually computes in this order (not, e.g., widen to int64, shift, then truncate — the tie-breaking on negatives differs by 1 in some cases). Since >> on a Python int is arithmetic and RISC-V srai is arithmetic, this should line up, but only if the kernel's multiply is signed and fits in 32 bits pre-shift. 26214 * 26214 ≈ 6.9e8 fits, good — but call this out.

💭 np.asarray(arr).reshape(-1) in put — Nice touch, handles scalars and arrays uniformly. But for int scalars like w(par + 0, Nn), Nn is a Python int (not np.int64), so int(v & 0xFFFFFFFF) in w still works. Fine.

💭 Comment "per-product arithmetic shift then 32-bit accumulate (matches kernel)" — Good, but should be moved above the loop rather than between got and reference to make it clearer which semantics are being pinned.

💭 Consider a tiny _i16s(v) helper — Q=65536 appears 3 times; if int16 quantization is the standard, a Q_INT16 = 65536 constant at module scope would document intent and make tests less surprising.

---

📁 tests/test_platform_stage2.py

🔴 Bug: CONV reference assumes batch N=1 — _conv_ref(xq[0], ...) strips the batch dimension, but the kernel writes N * Cout * H * W outputs. All current CONV entries have N=1 so it passes, but the test will silently break (index-out-of-range or wrong comparison) if anyone adds a batch>1 case. Either assert N == 1 explicitly or make _conv_ref loop over batches.

🔴 Bug: SPMM density semantics silently flipped — Old: mask = random < (1.0 - dens) (dens = fraction of zeros). New: mask = random < dens (dens = fraction of non-zeros). Values like 0.90 now mean 90% dense (i.e. near-dense matrices), which defeats the purpose of an SpMM test. Verify this matches stage2数据点.md v1.2; if the doc means sparsity, invert.

🔴 Bug: Reference/kernels diverge on quantization rounding — _q uses np.trunc (toward zero). RV32 ssra x, x, 16 is arithmetic shift (toward −∞). These differ for negative values like -1.5 * 65536. If the kernel rounds the way the reference expects, fine — but please document that both sides use trunc-toward-zero, or use >> 16-equivalent math in _q.

🟡 Suggestion: FWHT test uses strict == on lists — Exact equality on 4096 int32s is fine for pure butterfly ops, but any subtle ordering/overflow divergence produces a hard failure with no delta info. Wrap with assert (np.array(got) - np.array(_fwht_ref(x))).sum() == 0 or similar to keep pytest failure messages readable.

🟡 Suggestion: _conv_ref accumulator is int64, kernel almost certainly int32 — Safe today (max accumulated product ≈ 3·10⁵ << 2³¹), but if the range widens beyond [-0.4, 0.4] they diverge silently. Either force int32 in the reference or add a comment capping the safe range.

🟡 Suggestion: test_stage2_conv unpacks cfg redundantly — N, C, H, W, K, Cout = cfg then model = _conv_model(*cfg) — only the first is used downstream. Delete the unpack or use _conv_model(N, C, H, W, K, Cout) for one source of truth.

🟡 Suggestion: Non-deterministic-looking seed from hash((N, C, H, W, K, Cout)) — Python int/tuple hashing is stable within a run and, for pure int tuples, across runs, but it looks fragile to future readers. Use a fixed seed or abs(zlib.crc32(repr(cfg).encode())) for clarity.

🟡 Suggestion: Test IDs are opaque — c1…c10, sp1…sp10 don't reveal which shape failed. Try ids=[f"N{N}C{C}H{H}W{W}K{K}CO{Cout}" for N,C,H,W,K,Cout in CONV].

🟡 Suggestion: Memory addresses hardcoded with no collision guard — Conv uses 0x10000/0x80000/0x100000/0x120000/0x140000, SpMM uses 0x1000/0x100000/0x600000/0xB00000/0x1000000/0x1100000/0x1200000. With (1024,1024,1,0.95) SpMM the B matrix alone is 4 MB, but ba=0x1000000 and out=0x1100000 give B only 1 MB of space before clobbering out. Double-check the largest case fits; consider computing offsets from sizes.

💭 Nit: Docstring says "per-product >>16" but FWHT has no products — the butterfly is pure add/sub. Reword to "per-product >>16 for conv/spmm, raw int32 add/sub for fwht".

💭 Nit: Range narrowed from [-0.9, 0.9] to [-0.4, 0.4] — fine for overflow headroom, but confirm this matches the actual data distribution in the spec; otherwise the test no longer exercises the tails.

---