Impl/topic27

[FeelTheBeats]

No description provided.

[github-actions]

🤖 AI Code Review

共审查 7 个变更文件

📁 .github/workflows/ci.yml

🟡 Redundant mkdir -p — Line ~228: benchmark_reports/ directory is already created by preceding benchmark steps in the same job (see const_merge_report section above). The mkdir -p is harmless but unnecessary if the parent directory always exists by that point. Harmless to leave; just noting.

💭 Consider --timeout for the new test step — The new regression test block (line ~110) has no timeout guard. If test_rv32_bench.py hangs, the job blocks indefinitely. Existing test steps may share this issue, but since you're adding new ones, it's a good moment to add timeout-minutes or a pytest --timeout plugin.

💭 Two-step dependency coupling — Tests (line ~110) and report generation (line ~228) are in different jobs with no explicit dependency between them. If the test job fails, the report job still runs — which means you could get benchmark reports from a build where the corresponding regression tests already failed. This matches existing const_merge pattern, so it's consistent, but worth noting if you ever want to fail-fast.

Overall the diff is clean — consistent with existing conventions, proper conditional summary guard, no security concerns.

---

📁 benchmarks/run_topic27_rv32_bench_case.py

🔴 Bug: executed == ops_total invariant in _probe_honest is semantically wrong — Lines 256–261:

probe.get("executed") == probe.get("ops_total")

For budget_exhausted, the model was truncated — executed should equal limit, not ops_total. If ops_total represents total ops in the model (not total executed), this invariant is inverted and the probe will always fail (or pass vacuously if the framework happens to truncate ops_total too, which would be a data-loss bug in the upstream). Verify against rv32_bench's actual semantics of scratchv.dynamic.ops.total.

🔴 Bug: Budget limit (1000) may not trigger budget_exhausted — Line 53:

BUDGET_LIMIT = 1000

The model is an 8×8 input, 3×3 kernel, 1-channel conv → 36 output pixels × 9 MACs = ~324 arithmetic ops. Even with emulator overhead, 1000 instructions may complete the model with completion="halted", making the budget probe report status="unexpected" and hard-failing the case. Verify empirically, or add an assertion that the probe actually triggers budget_exhausted (log a warning if it doesn't).

🟡 In-process rv32_bench.main call risks state leakage — Line 93:

exit_code = rv32_bench.main(argv)

Three sequential calls (primary, budget_probe, timeout_probe) share global state. If rv32_bench.main mutates any module-level caches, file handles, or env vars, later probes may see contaminated state. Consider subprocess.run or at minimum a defensive note in the docstring.

🟡 _audit_probe can pass vacuously when simulator is unavailable — Lines 208–222:

When source != "simulated", only 2 tampers are generated (ratio forgery + schema version). If audit_provenance doesn't reject the ratio forgery when the original ratio is None, the probe passes with fewer checks than intended. Consider always testing at least 3 independent forgery axes regardless of simulator state.

🟡 Deprecated numpy API — Line 49:

rng = np.random.RandomState(0)

RandomState is deprecated in NumPy ≥1.18. Use np.random.default_rng(0) for forward compatibility (note: different algorithm, but since this is a self-contained case model with fixed seed, the output just needs to be deterministic within this version).

🟡 Model file side effect in output directory — Lines 591–595:

model_path = args.json.parent / "topic27_rv32_bench_feature.onnx"
build_case_model(model_path)

Writing the model next to the JSON report is a silent side effect. On repeated runs the stale model file persists and evaluate uses it without regenerating. Consider writing to work_root (the temp dir) instead, or explicitly overwriting each time.

🟡 No input validation in evaluate — Line 305:

"sha256": _sha256(model_path),

If evaluate is called directly (not through _execute) with a non-existent path, _sha256 raises uncaught FileNotFoundError. Add a guard or propagate a meaningful error.

💭 case_builder parameter is misleading — Line 293: The parameter is just a metadata string label, not a builder function. Renaming to builder_label would clarify intent.

---

📁 docs/topics/27-RV32全量Benchmark-开发文档.md

Code Review

🟡 **Chunk interval too large for halt detection** — C8/C11: `chunk_instructions=10_000_000` with ~70K instr/s means ~143s before checking `pc==halt_addr` or budget. Consider defaulting to a much smaller chunk (e.g., 100K) so halt/budget are detected promptly and timeout granularity is better.

🟡 **T1 exact-equality comparison is fragile** — Table T1: "与 RV32EmulatorFast 对照 total/load/store/branch 精确相等" — TinyFive and RV32EmulatorFast have different instruction classification schemes (`Cat_*` vs ops dict). Exact equality on `total` may hold only if both count the same instruction count semantics. Consider asserting `abs(total_a - total_b) < threshold` or documenting which categories are structurally guaranteed identical.

🟡 **`halt_addr` formula collision risk** — `halt_addr = align_up(binary_bytes, 16)`, but with `gp=128MiB`, `input=160MiB`, `output=192MiB` as fixed registers, `halt_addr` for a binary >128MiB would collide with `sp`'s region. `compute_layout()` GUARD check is the safety net, but the formula itself should be documented as assuming `binary_bytes << gp_base`. Add a comment in §2.3 step 6 or §2.3 step 5 that `halt_addr < gp_base` is an invariant.

🟡 **`build_input_q16` range is suspiciously narrow** — §2.3 step 3: `int((r.random() - 0.5) * 0.2 * 65536)` produces values in ≈[-6554, +6554], which is only ±0.2 in q16.16 — well within the [-1.0, +1.0] range. This is fine for avoiding saturation but means convolutions will produce very small activations, potentially masking underflow/rounding bugs. Consider documenting why 0.2 is chosen or adding a `--input-scale` CLI flag.

🟡 **9 vs 8 test discrepancy** — Test table (§六) lists T1–T8 (8 cases) but the implementation results say "9 个新用例". Either a case is missing from the table or the count is wrong.

🟡 **LLVM ISA mismatch set may miss compressed RV64** — C9 item 3: the set `{ld, sd, lwu, addw, ...}` doesn't include compressed RV64-only forms (e.g., `c.lwsp` is RV64, `c.sd` doesn't exist in RV32 but `c.lw`/`c.sw` are shared). If `onnx_to_llvm_standalone.py` can emit compressed instructions, some RV64-only ops might slip through. Consider also scanning for `fswd`/`flw` and documenting that only explicit 32-bit mnemonics are checked.

🟡 **SIGALRM + chunk polling redundancy** — C8: both `SIGALRM` timeout and chunk-loop halt/budget check exist. If `SIGALRM` fires mid-chunk, `_timed_out` is set but execution continues until the current chunk ends (up to 143s at default chunk size). Clarify whether the intent is that SIGALRM interrupts immediately or only sets a flag checked at chunk boundaries.

💭 **Exit code 7 is non-standard** — §5.1: using 7 for `--fail-on-incomplete` works but is outside common conventions (0-3, 126-127). Fine if documented and consistent, but a brief note in the doc explaining why 7 was chosen (vs. 2 or a new code in 100-range) would help future maintainers.

💭 **Regex fragility not in risk table** — §2.3 step 1 defines three regexes parsing compiler stdout. R6 covers label format drift but not stdout format drift. Add these three regexes to R6's scope or add a separate risk row.

💭 **`--full` flag semantics could be simpler** — §2.2: "`--max-instructions 0` (default) 与 `--full` 等价；两者与 `--max-instructions N>0` 冲突时 `parser.error(...)`" — so `--full` is purely redundant with the default. If the goal is "抵抗上游/CI默认值污染", consider making the default `None` and requiring explicit `--full` or `--max-instructions N` to run, so a bare invocation without either flag exits with a usage error.

---

📁 docs/topics/27-RV32全量Benchmark-设计文档.md

🔴 Bug: ra 寄存器被覆盖导致停机条件永久失效 — §2.1.4: harness 设 x1 = halt_addr，_done: ret 即 jalr x0, x1, 0。但若生成代码中出现任何 jal ra, func（函数调用），x1 被覆盖，PC 永不到达 halt_addr，全量模式只能等超时。文档虽提到此风险，却未给出解决方案。建议：在 §2.1.4 明确验证编译器是否生成函数调用；若可能生成，则改用专用寄存器（如 x16/t0）承载停机地址，或在 _start prologue 中将 halt_addr 存入专用寄存器，_done 改为 jalr x0, <专用寄存器>, 0（需改编译器输出，与 §4.9 "不改编译器" 矛盾，需显式处理）。

🔴 Bug: 内存布局校验缺项——output 区与 code+weights 区未做碰撞检测 — §2.1.2 约束公式中缺少：192 MiB >= halt_addr + 4。当前约束只验证 halt_addr + 4 <= mem_size 和 192 MiB + out_bytes <= mem_size，但不验证 output 区起始地址（192 MiB）是否在 code+weights 区之后。当模型较大（如 26 MB 权重 + 代码 ≈ 26.7 MiB，halt_addr ≈ 0x019F0000），192 MiB 远大于此值，看似安全。但若 --mem-size 被调小（如 64 MiB），halt_addr 仍可能落在 64 MiB 以内，output 区地址 192 MiB 已超出 mem_size，此时 load_data(weight_bytes, data_offset) 写入范围 [0, halt_addr) 与 output 读取地址 192 MiB 不冲突（192 MiB > mem_size 意味着输出区根本不存在）。建议增加约束 192 MiB <= mem_size 并在校验失败时给出明确错误。

🟡 Suggestion: provenance 规则 2 存在合法误判 — §2.2.4-2: "completion=='halted' but executed == limit and limit != null" 被标记为伪造。但当 budgeted 模式下程序恰好执行到 limit 条指令时停机，这是合法场景。应改为：验证 pc == halt_addr 而非比较 executed 与 limit；halted 的唯一判据应为停机地址命中，与 limit 无关。

🟡 Suggestion: 猴补丁 exe() 重绑的脆弱性 — §2.1.4: 对机器实例重绑 exe 方法。若 ProfiledMachine.run() 内部不通过 self.exe() 分发（如直接调用 self._machine.exe()），重绑失效且静默。建议在实现时添加断言验证重绑生效（例如 assert m.exe.__name__ != 'exe' 或检查 wrapper 签名），并在 §4.8 测试中加入重绑有效性校验用例。

🟡 Suggestion: 缺少执行状态机图 — §2.1.1 定义了 run_mode、completion、分块循环、超时、预算等状态转换，逻辑较复杂。建议补充 Mermaid 状态图或时序图，尤其是 chunk → SIGALRM set → run → SIGALRM clear → check halt/budget/timeout 的交互，以及异常路径（last_error、_timed_out 标志、not_run）的转换。当前纯文本描述在实现时容易遗漏边界条件。

🟡 Suggestion: static_insns == data_offset // 4 假设所有指令 4 字节 — §4.5 和测试用例 5 均依赖此等式。若编译器使用 RV32C（压缩指令，2 字节），此假设不成立。建议在文档中显式声明"编译器只生成 RV32I base ISA，不使用 RVC"，或改用 .text 段字节数与指令字数的比值检测来校验。

💭 Nit: §4.4 伪代码中 chunk 计算边界 — 当 budgeted 且 executed + chunk_instructions > limit 时，min(chunk_instructions, limit - executed) 可能返回 0（若 executed == limit）。虽然循环顶部有 executed == limit 的前置检查，但伪代码中 chunk 赋值发生在 m.run() 之前，读者可能误判执行顺序。建议将 chunk 计算移至停机检查之后，或添加 assert chunk > 0。

💭 Nit: §5.1 JSON 示例中 input_shape: [1, 3, 250, 250] 与 input_elements: 187500 计算正确（1×3×250×250 = 187500），但 output_shape: [1, 1] 对应 output.elements: 1——建议补充说明 output 元素数如何从 shape 推导，以便 validate_report_schema() 可自动校验。

---

📁 scratchv/standalone/bench_report.py

Code Review: scratchv/standalone/bench_report.py

🔴 Bug: int() will crash on hex-string addrs — render_markdown (~L580, L640):

f"halt=0x{int(sv_dyn.get('halt_addr') or 0):x}"
f"(addr=0x{int(sv_out.get('addr') or 0):x}, ..."

If the simulator serializes addresses as strings ("0x8000"), int() raises ValueError and the whole report fails to render. validate_report_schema never checks the type of halt_addr/addr, so this slips through.
Suggestion: use int(v, 0) or validate as int in validate_report_schema.

🔴 Bug: required-field check is wrong for incomparable_reason — validate_report_schema (~L870):

require("comparison.incomparable_reason",
        lambda v: v is None or (isinstance(v, str) and bool(v)))

require treats a missing key as an error, so this forces the key to be present even when dynamic_instruction_ratio is a valid number — inconsistent with the render code, which only reads it when ratio is None. Either make the key optional, or drop the check and let render_markdown handle absence.

🟡 Fragile header detection — _md_to_html (~L665):

tag = "th" if not any(mark.startswith("<tr>") for mark in out[-2:]) else "td"

Correct today, but it infers "first row" from string-sniffing the tail of out. It breaks if a comment/blank handling or any future change inserts a line inside the table. Track it explicitly: first_row = not in_table captured before appending <table>.

🟡 Silent type coercion masks schema bugs — render_bench_json (~L688):

return json.dumps(report, sort_keys=False, indent=2, default=str)

default=str will quietly stringify a datetime, Path, Enum, etc., so a broken generator passes silently and downstream consumers get "2024-01-01 12:00:00" where they expect ISO-8601. Consider dropping default=str and letting it raise, or restricting it to known safe types.

🟡 Missing cross-field consistency checks — validate_report_schema:

• executed vs dynamic.ops.total (should be >= / equal).
• output.elements is required to be is_int, but for partial: true the field is plausibly None — validation would flag a legitimately partial run as invalid.
• q16_16 length check happens, but static_instruction_mix counts are never checked to sum to static_insns.

🟡 _fmt emits bare nan/inf — (~L452): f"{value:g}" produces "nan"/"inf" for non-finite floats, which is indistinguishable from a string. Return "—" or explicitly render "NaN"/"inf" if you accept non-finite data.

💭 Hardcoded — for LLVM op breakdown — (~L525):

dyn_rows.append((f"ops.{key}", _fmt(sv_ops.get(key)), _fmt(None)))

Reads as "LLVM has no ops data", which is misleading — LLVM presumably has them, they just weren't collected. A short comment (or an [n/a] marker with a footnote) would prevent future readers from thinking it's a rendering bug.

💭 Validation style inconsistency — comparison uses a manual "key" not in dict check for dynamic_instruction_ratio but require() for incomparable_reason. Pick one pattern for the whole function.

---

📁 tests/test_rv32_bench.py

🔴 Flaky timeout test — test_wall_clock_timeout_is_labeled_partial: --timeout 0.05 (50 ms) is dangerously tight for CI. On a fast runner the tiny model halts before the deadline, so completion becomes "halted" instead of "timeout" and the test fails non-deterministically.

Suggestion: Use a much smaller budget via --max-instructions (e.g. 1) combined with a timeout the model can't finish within, or set --timeout 0.001. If you truly need to test wall-clock timeout, pick a model/chunk config that provably exceeds the deadline, or inject a sleep into the simulation loop.

---

🟡 Inconsistent test fixtures mask real bugs — _fake_scratchv has executed=100 but ops sum to 7+3+5+10+0+4 = 29, and static_insns=160 vs static_instruction_mix sum = 29. If audit_provenance or validate_report_schema ever adds a check that ops.total == executed or sum(mix) == static_insns, every test using _valid_fake_report breaks with a confusing error.

Suggestion: Make _fake_scratchv internally consistent — set executed to match the ops dict total, and make static_insns match the sum of static_instruction_mix.

---

🟡 Tests depend on private APIs — _iter_asm_lines (T5), _read_output (T7b), and internal module state patched via monkeypatch.setattr(rv32_bench, "ProfiledMachine", …) / compile_llvm_rv32. These are fragile: a rename or refactor breaks tests silently.

Suggestion: For _read_output, test it through run_simulation or build_report. For _iter_asm_lines, consider testing parse_labels with crafted asm strings only (no iteration). For the monkeypatched ProfiledMachine, inject via a fixture that sets a module-level flag instead of swapping a class.

---

💭 Misleading test name — test_cli_defaults_to_full_simulation_no_hidden_truncation asserts args.full is False. The default is unlimited instructions (max_instructions == 0), not --full. Name suggests --full is the default.

Suggestion: Rename to test_cli_defaults_are_unlimited_not_hidden_truncation.

---

💭 Stub machine returns signed ints, not unsigned 32-bit — test_output_q16_16_is_always_a_list: _StubMachine.read_mem_i32 returns Python signed ints (-65536, -131072). Real emulators return unsigned 32-bit values (0xFFFF0000, 0xFFFFFF00). If _read_output relies on unsigned interpretation, this test doesn't catch signed/unsigned bugs.

Suggestion: Return unsigned values: self.words[addr // 4] & 0xFFFFFFFF, or explicitly test both signed and unsigned paths.

---

📁 tests/test_topic27_rv32_bench_case_report.py

🔴 Bug: Vacuous hard_checks pass — Line 52: all(base_report["hard_checks"].values()) returns True when hard_checks is {}. Add assert base_report["hard_checks"] first.

🟡 Brittle exit-code coupling — Line 89: assert exit_code == 2. If CLI exit codes change, this breaks silently. Consider assert exit_code != 0 or a named constant like case.EXIT_MISSING_MODEL.

🟡 Incomplete mock surface — _UnavailableProfiledMachine (lines 19-23) only sets available and mem_size. If rv32_bench calls methods (e.g., .get_memory(), .simulate()), the mock will raise AttributeError. Verify the full interface expected by evaluate().

🟡 Hard-coded message assertion — Line 155: assert "TinyFive is unavailable" in report["honesty"]. This couples tests to exact prose. Prefer checking a structured field (e.g., report["honesty"]["reason"]) or a regex pattern.

🟡 Missing corrupt-model test — Only missing-file is tested (line 82). Add a case with an invalid/corrupt .onnx file to cover parse-time failure paths.

💭 No parametrization for probes — test_probes_match_environment (line 139) branches on simulator_available inside one test. Split into two parametrized tests (@pytest.mark.parametrize("sim_avail", [True, False])) for clearer failure isolation.

💭 Redundant schema assertion — Lines 49-51 assert both top-level and nested schema_version. The inner assertion at line 110 repeats the same check. Consider a single shared assertion helper.

---