🤖 AI Code Review

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

📁 .github/workflows/ci.yml

🔴 Bug: Pipeline failure masked — "Run topic13 peephole CLI smoke":

$PY -m scratchv.backend.asm_peephole ... | tee /tmp/peephole_report.json

No set -o pipefail, so if asm_peephole exits non-zero but tee succeeds, the step passes silently. Subsequent json.load and grep checks then operate on stale/missing data, giving confusing failures or false passes.

🟡 Fragile assertion — grep -q "8" /tmp/peephole_out.s:
Matches any occurrence of the digit 8 in the file (e.g., lw $ra, 8($sp)). If the intent is to verify a specific fused instruction or immediate value, this will produce false positives and serve as no real regression guard.

🟡 Duplicated detection block — The 10-line Python 3.12 if/elif/else is copy-pasted in both jobs. If the detection logic needs updating, you'll fix it in one place and miss the other. Consider a composite action or a small helper script (e.g., scripts/find_python312.sh).

🟡 Missing set -euo pipefail — The "Run topic13 peephole CLI smoke" step has no set -e. While GitHub Actions sets -e by default, the subsequent unconditional grep -q "unsound rule..." / exit 1 guard is only meaningful if the first grep for "addi+addi fusion" succeeds — which it will since the step continues regardless. Adding set -euo pipefail at the top makes the intent explicit and protects against the pipeline gap above.

📁 Makefile

Code Review

🔴 No output validation on fixture run — test-peephole: running the tool on input_addi_fusion.s and checking exit code is insufficient. If the optimizer is broken (e.g., produces wrong codegen), this still passes. Consider diffing the output against a golden file:

test-peephole:
	python3 -m pytest tests/test_asm_peephole*.py -v --tb=short
	python3 -m scratchv.backend.asm_peephole --list-rules
	python3 -m scratchv.backend.asm_peephole \
		tests/fixtures/asm_peephole/input_addi_fusion.s \
		-o /tmp/peephole_out.s --report --json
	diff -u tests/fixtures/asm_peephole/expected_addi_fusion.s /tmp/peephole_out.s

🟡 Hardcoded /tmp/ output never cleaned — /tmp/peephole_out.s leaks across runs and isn't in the clean target. Consider writing to a build-local path (e.g., build/peephole_out.s) and adding it to clean.

🟡 No fixture existence guard — If tests/fixtures/asm_peephole/input_addi_fusion.s is missing, the error will be an opaque file-open failure from the tool. A quick guard improves DX:

test-peephole:
	@test -f tests/fixtures/asm_peephole/input_addi_fusion.s || \
		{ echo "missing fixture"; exit 1; }

💭 JSON output pollutes CI logs — --json dumps structured output to stdout in test-peephole. If this is just to verify it runs without error, consider redirecting (> /dev/null or > /tmp/peephole_report.json) so CI logs stay readable.

📁 benchmarks/compare_peephole.py

🔴 **Bug: Broken Markdown when total_before == 0** — Line ~120: conditional f-string inserts "" into the list literal, producing a malformed table row (header separator `|` present but no data).

```python
# current: inserts empty string as a "row"
f"| DSL 基准... |" if report.total_before else "",

Suggestion: Guard outside the list literal, e.g. append conditionally or wrap the whole section.

🟡 Unnecessary disk write in compile_dsl — Line ~82: path.with_suffix(".s") writes an assembly file to disk that's never read. Wastes I/O and could collide with existing files or create artifacts in the source tree.

# suggestion: skip file path, or use a tempdir
result = driver.compile(str(path), str(out))

🟡 Hardcoded case count in Markdown — Line ~133: "## DSL 基准（23 个用例）" is a magic literal that drifts silently as test cases are added/removed.

Suggestion: f"## DSL 基准（{len(report.cases)} 个用例）"

🟡 Hardcoded opcode list — Line ~68: opcodes = ("addi", "li", "mv", "beq", "j", "jal", "ret") — misses any opcode not in this tuple. If peephole introduces a sw/lw fusion rule, it goes untracked.

Suggestion: Derive the opcode set dynamically from before_counts["_detailed"] keys, or at least add a comment explaining why this subset is intentional.

🟡 No directory existence check — Line ~157: if --cases points to a non-existent path, glob silently returns empty and the report shows zero cases with no warning.

Suggestion: if not cases_dir.is_dir(): parser.error(...)

🟡 to_markdown called unconditionally — Line ~209: generates Markdown every run even when --markdown isn't passed. Minor perf waste; also means formatting bugs surface in console runs.

💭 Fragile _ prefix convention — _total_static and _opcode_total rely on internal keys starting with _ to separate metadata from real counts. This is implicit; a typo like detailed would silently be counted as an instruction.

💭 Import from sibling bench file — Line ~96: from benchmarks.bench_asm_peephole import _gen_synthetic_asm imports a private helper. If that file gets refactored, this breaks silently at runtime. Consider extracting the generator into a shared util.

📁 docs/topics/13-窥孔优化器-设计文档.md

Review: docs/topics/13-窥孔优化器-设计文档.md

🔴 Rule 4 (mv 链) 健全性不足且检查机制未定义 — §5.1 算法、§6 规则 4

文档承认 Rule 4 无完整活跃性分析时"可能不健全"，但 §5.1 的算法描述中没有任何 liveness 检查步骤，§6 仅标注"best-effort"。对于 mv t0,t1; mv t2,t0，若后续代码使用 t0，优化后 t0 的值会丢失——这是语义错误，不是"可观测的 best-effort"。

建议：要么在算法中明确定义局部 lookahead 的范围与规则（如"检查后续 N 条指令中 t0 是否被读取"），要么将 Rule 4 降级为"仅当后续无任何指令引用第一条 mv 的 rd 时才改写"，并在代码中加断言/警告。

🔴 Rule 2 (li+addi) 缺少立即数范围约束 — §6 规则 2

Rule 1 明确要求 imm 和 ∈ [-2048, 2047]（simm12），但 Rule 2 的 li rd, imm_sum 无任何范围限制。如果后端 li 伪指令也受 12 位约束，li t0, 2048 + addi t0, t0, 1000 会生成非法汇编。

建议：补充 Rule 2 的 imm_sum 范围约束，或明确说明 li 在此后端支持的范围。

🟡 x0/zero 别名不一致影响多条规则 — §8 技术债

Rule 3 处理了 x0/zero 双名，但 Rule 5（rd==rs）、Rule 8（rd==rs）、Rule 4（mv 链匹配）均使用字符串比较。addi x0, zero, 0 不会被 Rule 5 删除，mv x0, zero 不会被 Rule 8 删除——这是规则间的不一致。

建议：在 §5.2 步骤 2 之前增加"规范化寄存器名"步骤（如统一将 zero → x0），或至少列出哪些规则需要手动处理别名。

🟡 Rule 1 的 rs1 记法有歧义 — §6 规则目录表格

替换模板写 addi rd, rs1, imm_sum，但 rs1 在本文档中既指"第一条指令的第一个源操作数"，也指 RISC-V 通用寄存器名。读者可能误解为"替换结果中的 rs1 就是第一条指令的 rs1"。

建议：改用更明确的记法，如 addi {rd0}, {rs1_0}, {imm_sum}，或直接写 addi rd, (第一条的rs1), imm_sum。

🟡 遗漏：bne x0, x0, L 和 beq rs, rs, L — §6 规则 3

beq rs, rs, L 对任意 rs 恒真（不只是 x0），bne x0, x0, L 恒假（可整条删除）。当前只覆盖 beq + x0/zero 的组合。

建议：§9.1 路线图中标注为"低难度可补"的规则。

💭 §5.1 的 for...else 写法 — 算法伪代码

for rule in rules: ... else: ... 是 Python 特有的 for-else 语义，非 Python 读者可能理解为 C/C++ 风格的 else-if。

建议：改用显式 matched = False 标志，或在注释中标注"Python for-else 语义"。

💭 §7.4 测试计数与 §12 重复 — 一致性

§7.4 写"83 PASSED"，§12 也写"83 PASSED"。如果测试数量变化，两处需同步更新。

建议：§12 引用 §7.4，或只在一处维护数字。

📁 docs/topics/13-窥孔优化器.md

🟡 日期可疑 — 头部写 2026-08-01，当前是 2025 年。若为计划日期应标注"预计"，否则是笔误。

🟡 导航链接路径未验证 — archive/topic13_asm_peephole_guide.md、13-窥孔优化器-设计文档.md、../../topic13/README.md、../../benchmark_reports/peephole_compare.md 共 4 个相对路径，文档更新时常断链。建议确认这些文件存在后再发布，或加 CI 断链检测。

🟡 mv 链规则仍列为默认但标注"不安全" — 规则表中 mv a,b; mv c,a → mv c,b 标了"中间寄存器若仍存活则不安全"，陷阱表也再次警告。如果规则无法保证活跃性检查，它就不该出现在"默认规则"列表里——建议要么实现活跃性检查后去掉警告，要么移入"实验性/需手动启用"区块。当前状态会让读者困惑：这条规则到底能放心用吗？

💭 移除了行数信息 — 原头部有 行数：~400，删除后读者失去了对源码规模的预期。如果文件行数变了可以更新数值而非直接去掉。

💭 总结表"合成高冗余对比 约 -25%" — 其他指标都有精确数字（231→230、13→7），这里用"约"显得不严谨。如果能给出具体基线（如 N→M），一致性更好。

📁 docs/topics/archive/topic13_asm_peephole_guide.md

🟡 Schema/tables inconsistent — register_constraints semantics doesn't match its own example tuples. §PeepholeRule Schema defines the check as window[dst_idx].operands[0] == window[src_instr_idx].operands[src_op_idx], but the Default Rules tuples can only be satisfied under a different (4-tuple or dst-op-indexed) reading:

• Rule 1 addi+addi fusion needs window[0].operands[1] == window[1].operands[0] (rs1 of first == rd of second), i.e. (dst=1, src=0, op=1) — the doc says (0,1,0),(0,1,1).
• Rule 4 redundant mv elimination needs window[0].operands[1] == window[1].operands[0] — the doc says (0,1,1), which under the stated semantics reads rd0 == rs2_1 (unrelated to the mv-chain join).

Suggestion: either add a dst_op_idx to the tuple ((dst_instr, dst_op, src_instr, src_op)), or correct the tuples in the table so agents adding new rules don't copy wrong semantics. This is the highest-risk item — Step 1/2/3 of "How to Add a New Rule" will teach the wrong pattern as-is.

🟡 Naming collision risk: Rule 4 is redundant mv elimination and the banned rule is redundant mv pair elimination. Two-word difference, and the top-of-file warning plus the "Removed (unsound)" note make it easy to conflate. Suggest renaming Rule 4 to mv-chain collapse (or similar) so the string is unmistakably different, and grepping for the banned name stays meaningful.

🟡 CLI flag ambiguity: Pitfalls table mentions both --json and --json-output; Verification Commands only shows --json. State the canonical flag and mark the other as an alias (or drop it) — otherwise test_cli_json_report will be interpreted inconsistently.

🟡 "Last verified: 2026-09-13" is a future date. If this is intentional (pre-release marker), fine; otherwise it's misleading and will confuse anyone diffing against today's date.

💭 cd /home/z/ScratchV-main in Verification Commands is a hardcoded personal path. Use cd "$REPO_ROOT" or cd "$(git rev-parse --show-toplevel)".

💭 beq note ops[0,1] ∈ {x0, zero} reads like set-membership notation but means "both ops[0] and ops[1] must be in {x0, zero}". Write it out.

💭 File location: lives under docs/topics/archive/ but has a fresh Last-verified date, active PR link, and a "How to Add a New Rule" workflow — reads as a live maintainer doc, not an archived one. Confirm the archive/ placement is intentional.

💭 Example rule overlap: the "End-to-end agent task" (li rd, 0 → mv rd, x0) is adjacent to Rule 6 (addi-zero to mv). One line clarifying that li zero-imm is not subsumed by Rule 6 (different opcode, distinct rule slot) would prevent an agent from "fixing" Rule 6 instead of adding the new rule.

📁 docs/topics/archive/课题13：窥孔优化器.md

🟡 **Flag rename without alias** — `--peephole` → `--peephole-asm`
  Breaking change if any existing user/script references the old flag.
  Suggestion: Accept `--peephole` as a deprecated alias (log warning) to avoid breakage.

🟡 **Future date** — `2026-08-01`
  Likely a typo for `2025-08-01`. Verify the actual completion date.

🟡 **8 rules vs. "若干"** — Status says "8 条默认规则" but W6–W9 only plan 4 rules.
  Suggestion: Either clarify which 8 rules are default, or change status to reflect actual count at the time of writing.

💭 **Relative link depth** — `[../../../topic13/README.md]`
  Going up 3 from `docs/topics/archive/` lands at project root — correct *if* `topic13/` is a root-level dir. Add a brief comment or footnote noting the expected repo structure, since future reorgs will silently break these links.

💭 **Strikethrough + bold mix** — `~~**禁止**：非真交换~~`
  Renders inconsistently across Markdown parsers (some don't nest bold inside strikethrough). Consider: `~~mv x1,x2; mv x2,x1 → 删除~~` + a separate line `> ⚠️ **禁止**：...`

📁 scratchv/compiler.py

🔴 Potential AttributeError — peephole warning: New message references opt.instructions_saved, opt.instructions_before, opt.instructions_after. If any of these aren't attributes of AsmPeepholeOptimizer (or aren't populated by .optimize()), this raises on every run where changes is non-empty. Verify these exist and are set inside optimize(), or the diagnostic path itself crashes the compile.

🟡 Duplicated linear-scan dispatch — _generate_riscv_linear and _generate_riscv_dag now contain an identical ~10-line block (import → block_from_machine_instrs → LinearScanAllocator → intervals → allocate → emit). Extract a helper, e.g.:

def _emit_via_linear_scan(machine_instrs) -> str:
    from scratchv.backend.regalloc_linear import (
        LinearScanAllocator, block_from_machine_instrs,
    )
    ls = block_from_machine_instrs(machine_instrs)
    lsa = LinearScanAllocator()
    lsa.allocate(lsa.compute_live_intervals(ls))
    return lsa.get_allocated_code(ls)

Otherwise a future fix to the linear path has to be applied in two places (the exact failure mode that produced the original bug this PR fixes).

🟡 Silent coercion of unknown reg_alloc values: mode = ... if self.config.reg_alloc in ("naive","greedy") else "greedy" maps typos like "gread", "greed", or "register" to "greedy" with no log/warning. Either validate the config at parse time (raise ValueError on unknown), or warnings.append(...) when you fall back. Same shape of bug as the one this PR fixes (linear → greedy fallthrough).

🟡 Default change linear → greedy is a behavior break — any downstream config, CI golden output, or user script that omitted reg_alloc will now silently emit different code. Worth calling out in a changelog / commit message, or gating behind a version bump.

💭 The comment "Previously RegisterAllocator ran first with mode='linear', which fell through to greedy" is good — consider also noting how it now falls through (the mode guard) so future readers don't reintroduce the bug.

📁 tests/fixtures/asm_peephole/input_addi_fusion.s

🟡 **Missing paired expected output** — This fixture has no corresponding
`expected_addi_fusion.s` (or equivalent) in the diff. The test is useless
without the expected optimized result (e.g. `addi t0, t0, 8` + `ret`).
Verify the output fixture exists in the same PR.

💭 **Test coverage is minimal** — Only the happy path (positive immediates)
is exercised. Consider adding edge cases in follow-up fixtures:
  - Mixed signs: `addi t0, t0, 3` + `addi t0, t0, -5`
  - Zero immediate: `addi t0, t0, 0` + `addi t0, t0, 5`
  - Sum overflowing int16 imm range: `addi t0, t0, 32767` + `addi t0, t0, 5`

📁 tests/fixtures/asm_peephole/input_addi_overflow.s

🟡 **Suggestion: Ambiguous immediate width** — 2000 + 2000 = 4000, which overflows 12-bit signed imm (max 2047) but fits 16-bit signed imm (max 32767). The filename says "overflow" implying merge should be rejected, but the validity depends entirely on the target ISA's immediate width. Consider choosing values that are unambiguous (e.g., 2047 + 1 = 2048) or adding a comment clarifying the assumed ISA constraint.

🟡 **Suggestion: Add a test comment** — Fixtures benefit from a one-line comment stating the expected optimization result. E.g., `# Should NOT merge: 4000 overflows 12-bit immediate`.

💭 **Nit: Missing expected-output fixture?** — Presumably `output_addi_overflow.s` exists elsewhere. If not, the test has no assertion target.

⚠️ 未审查的文件

• tests/fixtures/asm_peephole/input_beq_zero.s
• tests/fixtures/asm_peephole/input_hex_fusion.s
• tests/fixtures/asm_peephole/input_li_addi.s
• tests/fixtures/asm_peephole/input_mv_chain.s
• tests/fixtures/asm_peephole/input_no_change.s
• tests/fixtures/asm_peephole/input_nop_mv_self.s
• tests/test_asm_peephole.py
• tests/test_asm_peephole_blackbox.py
• tests/test_asm_peephole_integration.py
• tests/test_asm_peephole_stress.py
• topic13/README.md