feat(standalone)：new operators and platform-standard RISC-V asm backend

[FeelTheBeats]

in standalone

[github-actions]

🤖 AI Code Review

共审查 10 个变更文件
⚠️ 另有 2 个文件超过上限（最多 10 个）未审查

📁 pyproject.toml

Review:

No blockers — change is clean and internally consistent.

🟡 Update CI test matrix — Dropping 3.8/3.9/3.10 here will break builds if .github/workflows (or equivalent) still tests those versions. Mirror this change in CI config.

🟡 Document the drop — Add a changelog entry or release note noting the minimum version bump. Downstream users and packagers need explicit heads-up.

💭 Verify dependency floors — Some listed deps (e.g., onnx>=1.14, numpy>=1.24) may have their own Python version floors. Ensure none require <3.11, and consider tightening floors now that 3.11 is the baseline (e.g., numpy>=1.26 if needed for 3.11 compatibility).

---

📁 requirements/ci.txt

🟡 numpy <3 may be too loose — Line 7: numpy>=1.26,<3 admits numpy 2.x, which removed np.float_, changed default float type in some ops, and has other breaking API changes. If the codebase isn't explicitly tested against 2.x, pin to numpy>=1.26,<2 and add a separate CI matrix job for 2.x later.

🟡 protobuf has no upper bound — Line 9: protobuf>=4.21 admits 5.x (released 2025), which has breaking changes in generated code and Message.ToByteString(). Add <6 or test explicitly.

🟡 Inconsistent Python target in comments — Lines 3-4: Header says "Target Python: 3.11 - 3.12" but the install example hardcodes python3.11. Either parameterize or note python3.12 works too.

💭 Move the "Python 3.8 .venv is broken" warning — That's operational history, not dependency metadata. Belongs in README.md or CONTRIBUTING.md. Requirements files should describe what to install, not what not to do in the past.

💭 Consider --require-hashes for CI — If this file drives CI verification, unpinned + unhashed deps are a supply-chain risk. Either pin exact versions or use pip-compile --generate-hashes.

💭 Optional deps as comments is fragile — Lines 14-15: onnxruntime / llvmlite as commented-out lines is documentation, not enforcement. If the project already has a pyproject.toml with [all] extras (line 6 hints at this), let that be the single source of truth and remove the comments.

---

📁 scratchv/standalone/spike_sim.py

🟡 Empty env var falls through silently — _resolve_tool: if SCRATCHV_SPIKE is set to "", or skips it and falls to PATH. Likely intended, but worth an assertion or explicit comment if empty-string-as-"use default" is the contract, since shutil.which returning None is invisible to the caller until subprocess raises FileNotFoundError at runtime.

💭 Nit — shutil.which is called at import time for all three tools even if only one is used. Fine for this script's size, but if the module ever grows, consider lazy resolution.

Otherwise clean — removes hardcoded paths, proper fallback chain, good naming.

---

📁 tests/test_platform_asm.py

🔴 emit_platform_asm 调用签名不一致 — _build_platform_asm 传 2 个参数，test_platform_asm_is_size_independent_for_fwht 传 3 个（多了 model）。如果第三个参数是必需的，_build_platform_asm 会崩；如果是可选的，两处写法应统一。

🟡 冗余导入 — 第 12 行顶部已 from ... import emit_platform_asm，第 76 行在 test_platform_asm_is_size_independent_for_fwht 内又导入一次。删除函数内导入即可。

🟡 重复 setup 逻辑 — _build_platform_asm 和 FWHT 测试各自重复了 ONNXModel/MemoryPlan/CNNRISCVGenerator/redirect_stdout/generate/emit 的全套流程。可提取共用 helper，FWHT 测试只替换 model 构造部分。

🟡 大小写敏感的字面量断言可能静默通过 — 第 82 行 assert "li t2, 64" not in asm：若实际输出用 tab 或不同空格数（如 li\tt2, 64），该断言不会匹配、测试误判通过。建议用正则或 re.search(r'\bli\s+t2\s*,\s*64\b', asm) is None。

🟡 测试名 "size_independent" 名不副实 — 实际只测了 64 一种尺寸，仅验证了 "使用运行时 N 而非编译期常量"。若真要验证 size independence，应用至少两个不同尺寸生成 asm 并对比。否则建议改名为 test_platform_asm_uses_runtime_bounds_for_fwht。

💭 分支目标正则漏掉无逗号形式 — 第 110 行的 regex 要求逗号分隔（, \s*-?\d+$），但 RISC-V 的 j / jal 可以无逗号（j 100）。如果代码生成器可能输出这种形式，当前检查不会捕获。

---

📁 tests/test_platform_stage2.py

🟡 Reproducibility: hash() in _conv_model — rng = np.random.default_rng(abs(hash((N,C,H,W,K,Cout))) % 2**32).
hash() of tuples is randomized per-process (PYTHONHASHSEED), so weights change every run. Internally consistent (model + ref read the same tensor), so no false pass/fail, but you cannot reproduce a failure or pin a regression. Use hashlib.md5(repr(cfg).encode()).digest() or an explicit seed table.

🟡 Reference mirrors kernel numeric recipe, not a ground truth — _conv_ref/_spmm_ref do per-product >> 16 then accumulate, exactly like the kernels they test. If the kernel's accumulation order or shift point drifts, this oracle drifts with it and the test still passes. The docstring names the convention, which is good; consider also asserting against a float reference with a documented quantization tolerance so the kernel can be wrong without the oracle moving.

🟡 _conv_ref silently assumes N == 1 — it indexes xq[0], but CONV entries are all N=1 today. Add assert N == 1 (or generalize) so a future N>1 data point fails loudly instead of silently testing only the first image.

🟡 Magic-number memory layout with no overlap guard — inp/out/wp/bp/par and SpMM's va/ca/ra/ba/out/par are hardcoded offsets. Current max sizes fit (0x200000 conv, 0x4000000 SpMM), but if a data point grows or emit_platform_asm changes an output base, you get silent memory corruption and a confusing assert diff. A tiny helper that reserves ranges and asserts disjointness would catch this at setup time.

🟡 Coupling between test constants and emit_platform_asm output contract — out = 0x80000, par = 0x140000, and the SpMM block layout (block, block+4, block+8 → va/ca/ra) are implicit contracts with the emitter. A one-line comment at each call site stating "kernel writes to 0x80000, reads params from 0x140000" would make this explicit; today it's only inferable by reading the emitter.

🟡 _conv_model weight distribution vs. _fwht_model/input ranges — weights use N(0, 0.3) while inputs use U(-0.4, 0.4). With Q = 65536, a K=5, C=8 conv accumulates ~8·25 = 200 products of magnitude up to ~0.4·0.3·Q² >> 16 ≈ ±196k before bias — fits i32, but the headroom assumption isn't documented. Worth a comment stating the intended worst-case bit-width so a denser/larger future point doesn't silently overflow.

💭 SPMM.index(cfg) — O(n²) lookup for a 10-element list; harmless but for i, cfg in enumerate(SPMM) + SPMM_IDS = [...] is clearer and avoids the "first equal element" footgun if specs ever repeat.

💭 Naming collision: w is both "weight" and "write-word" lambda — w(wp + 4*i, int(v)) next to wq, wt, inits["w"]. Rename the lambda to wr32 (and r32 → rd32) for symmetry.

💭 Import hygiene — from onnx import numpy_helper is imported inside two test bodies; hoist with the other ONNX imports. np/contextlib/io are at top, so mixing styles.

💭 max_steps=400_000_000 passed through _run but the interpreter's own default is presumably different; a comment on why 4e8 (largest SpMM point estimate) would help future readers tune it.

💭 Conv par array duplicates H, W — [N, C, H, W, Cout, K, pad, 1, H, W, wp, bp] carries H, W at offsets 2,3 and again at 8,9. Presumably input vs. output spatial dims, but it's unexplained; a named tuple or comment would prevent an accidental reorder.

---

📁 tests/test_standalone_fwht.py

Review: tests/test_standalone_fwht.py

🟡 Fragile divisor formula — Line 71: Q >> (n.bit_length() - 1) only computes Q // n correctly for powers of 2. If a non-power-of-2 n were added to the parametrize list, the expected value would be silently wrong (e.g., n=48: shift=5 gives 2048, but 65536//48≈1365). Use Q // n — equally correct for all current cases, fails loudly for invalid ones.

🟡 Byte-by-byte memory write is needlessly slow and fragile — Lines 63-64: Iterating emu.mem[0x04000000 + i] = b is O(n) Python calls per test. If emu.mem supports slice assignment (bytearray), replace with emu.mem[0x04000000:0x04000000+len(in_bytes)] = in_bytes. Even if it doesn't today, a helper wrapping this pattern would be cleaner than repeating the idiom inline.

🟡 Missing non-trivial input test — Impulse-only tests verify the Hadamard matrix row sums (all 1s) but not butterfly ordering or sign correctness for general inputs. Consider adding one case like n=4, input [Q, 2*Q, 3*Q, 0], with known expected output from a reference. This would catch sign-flip or stage-order bugs that impulse tests miss (impulse produces uniform output, masking certain reorderings).

💭 Bare assert rc == 0 — Line 56: Add context so a failure tells you why conversion failed: assert rc == 0, f"ONNX→RISC-V conversion returned rc={rc}".

💭 Fragile error-message matching — Lines 77-78, 84-85: Substring checks ("power-of-two", "direction") will break if the error message wording changes. Consider matching on a regex or checking the exception type with a class-level assertion.

💭 Swallowed stdout — Line 54: redirect_stdout(io.StringIO()) silently discards converter output. If the converter prints warnings/diagnostics during normal operation, they'd be invisible on failure. Consider using pytest's capsys fixture instead — it captures stdout and makes it inspectable in failure output.

---

📁 tests/test_standalone_int32_weights.py

🟡 Style: Imports inside test functions — Lines 72, 88: TensorInfo imported inside test bodies. Move to top-level imports for consistency with other imports.

🟡 Test coverage: Missing edge cases — No tests for:

• Empty INT32 tensors (shape ())
• Alignment/padding between mixed float/INT32 tensors
• INT32 tensors larger than typical index arrays

💭 Nit: Unclear comment — Line 91: "Q16.16 conversion would inflate 2 -> 131072" — add calculation: 2 * 65536 = 131072 (Q16.16 fixed-point scaling).

💭 Nit: Hardcoded magic numbers — Lines 100-101: 0x8000 and -0x4000 for Q16.16 values. Consider adding brief comment showing conversion: 0.5 * 65536 = 0x8000, -0.25 * 65536 = -0x4000.

---

Tests are well-structured with clear intent. The four test cases cover the core INT32 parser and layout behaviors effectively. Assertions verify actual binary content rather than relying on intermediate objects.

---

📁 tests/test_standalone_optional_bias.py

Review: tests/test_standalone_optional_bias.py

🟡 Missing assertion on input count — Lines 23–28 (_conv_model) and 39–46 (_gemm_model): if someone accidentally made the fix ignore the bias entirely (instead of treating it as zero), the output would still match and the test would silently pass. Add a guard:

node = helper.make_node("Conv", inputs, ["y"], pads=[1, 1, 1, 1])
g = helper.make_graph([node], "conv", [x], [y], inits)
m = helper.make_model(g, opset_imports=[helper.make_opsetid("", 13)])
onnx.checker.check_model(m)  # catches malformed graphs
assert len(node.input) == (3 if bias is not None else 2)
return m

Same for _gemm_model.

🟡 Hardcoded register addresses lack documentation — Lines 69–70:

emu.regs[10] = 0x04000000  # input buffer address
emu.regs[11] = 0x05000000  # output buffer address

Add inline comments explaining what these registers represent (input/output data pointers). Without them, a future reader has to trace the emulator ABI to understand.

🟡 No upper-bound check on output read — Lines 76–77:

out_el = int(meta["output_elements"])
out = [emu.read_mem_i32(0x05000000 + 4 * i) for i in range(out_el)]

If the conversion produces a corrupt output_elements (e.g., a huge number), this reads arbitrary memory and hangs or OOMs. Add:

assert 0 < out_el <= 1024, f"unexpected output_elements={out_el}"

💭 in_el → input_elements — Lines 66, 85, 92: the name in_el is cryptic; input_elements or input_size would be self-documenting.

💭 Consider pytest.mark.parametrize for multiple seeds — The tests use a single random seed (7 for inputs, 0/1 for weights). A silent bug could hide in a specific numerical range. Adding @pytest.mark.parametrize("seed", [0, 1, 42, 123]) to _run would be cheap insurance.

💭 Module docstring ends abruptly — Line 7: the docstring explains the bug but not what this file tests. Consider appending:

Tests:
    test_conv_without_bias_matches_zero_bias
    test_gemm_without_bias_matches_zero_bias

Overall: the test is well-designed, the no-bias vs zero-bias comparison is the right oracle, and the structure is clean. The only real risk is the silent-pass scenario (🟡 #1) — everything else is polish.

---

📁 tests/test_standalone_spike_paths.py

🟡 test_no_hardcoded_foreign_path_remains — This test inspects source code strings, not behavior. It's brittle (breaks on any refactor/move) and only catches two specific magic strings. A developer could introduce /Users/foo/spike and this test passes silently. If you want to prevent hardcoded paths, add a CI grep/lint rule instead — tests should verify behavior.

🟡 Missing edge case: empty-string env var — Consider:

def test_empty_env_var_falls_through(monkeypatch):
    monkeypatch.setenv("SCRATCHV_SPIKE", "")
    monkeypatch.setattr("scratchv.standalone.spike_sim.shutil.which", lambda name: f"/usr/bin/{name}")
    assert _resolve_tool("spike", "SCRATCHV_SPIKE") == "/usr/bin/spike"

An empty env var is truthy in shell but falsy in Python — worth pinning down the contract.

🟡 Bare-name fallback may mask real errors — test_falls_back_to_bare_name asserts that returning "spike" (with no resolution) is correct. If the tool doesn't actually exist on PATH, the caller gets a bare name that will fail at spawn time with an unclear error. Consider whether raising a FileNotFoundError or similar would give better diagnostics, and test for that instead.

💭 Test names are clear — good descriptions of intent. Minor: test_falls_back_to_path_lookup could be test_falls_back_to_which_when_env_unset to mirror the pattern of the other fallback test.

💭 test_falls_back_to_path_lookup monkeypatches shutil.which module-wide — Since _resolve_tool does shutil.which(...) on the shutil module, the patch correctly targets scratchv.standalone.spike_sim.shutil.which. Fine as-is, but a comment noting why the patch target is the module's shutil.which rather than the import would help future readers.

---

📁 tests/test_standalone_spmm.py

🟡 Rounding conventions not pinned down — _q16 uses np.trunc (toward zero) but _ref_csr uses >> 16 on a possibly-negative product (floor). Strict == asserts the compiled code truncates like the reference, so any rounding-to-nearest path in the compiler would fail with a confusing "got != ref" message. Add a comment stating the expected convention, or assert it explicitly (e.g. a dedicated boundary test where a*b is an exact 2^16-1 negative).

🟡 _wrap32 applied before the shift in _ref_csr — prod = _wrap32(a * b) >> 16. This silently drops bits if |a*b| >= 2^31. Currently safe only because inputs are quantized from [-0.4, 0.4]. Either drop the pre-wrap (products of two ~17-bit values fit in int32) or widen the range test — otherwise this masks real overflow in the operator.

🟡 Variable shadowing in _ref_csr — loop-local k = int(col[j]) shadows the parameter k. Works today only because k is unused afterwards, but it will bite the moment someone adds a bounds check like assert col < k. Rename to ci/col_idx.

🟡 Hardcoded memory/register conventions — 0x04000000, 0x05000000, regs[10], regs[11] are the linker/ABI contract, duplicated verbatim in the test. If the standalone emitter's layout changes this test fails with an opaque emulator crash. Prefer reading these from meta (same way code_bytes/output_elements already are), or at minimum a one-line comment citing the convention.

🟡 Values tensor declared TensorProto.FLOAT but holds int32 Q16 data — likely the framework's intended convention, but nothing documents it. A reader will reasonably think this is a bug. One-line comment in _spmm_model.

🟡 test_non_square covers only rectangularity of B — cols 0..3 are always in range for k in {4,6}, so the sparse-structure path is identical to the square cases. Consider a case with col_ind near k-1 and row_ptr touching nnz-1 to actually exercise index bounds.

💭 Byte-by-byte memory write — for i, byte in enumerate(in_bytes): emu.mem[...] = byte is O(n) Python; emu.mem[base:base+len(in_bytes)] = in_bytes (if slice-assignable) or emu.mem.set(bytes) is clearer and faster. Same for np.asarray(b_q).tobytes() instead of struct.pack.

💭 Missing alpha/beta coverage — the third and fourth SpmmCsr inputs are untested, so a regression in the beta * C accumulation path (including beta=0 specialization) would pass here.

💭 No pytest fixture/parametrize — test_spmv_n1, test_spmm_n2_with_empty_row, test_non_square are three copies of _case(m,k,n); a @pytest.mark.parametrize("m,k,n", [...]) over one test function removes the duplication and makes coverage gaps visible in output.

💭 Docstring says "N=1 ... non-square ... empty row" but the empty-row assertion is implicit (row 1 of A). Since _ROW[1] == _ROW[2] is the only thing making that case work, assert ref[1] == [0]*n explicitly so a future edit to _ROW can't silently retire the empty-row coverage.

---

---

⚠️ 未审查的文件

• tests/test_standalone_winograd.py
• tests/test_winograd_kernel_transform.py