feat: 完善窥孔优化器 Benchmark（基于 PR #39）

[Sunwoods123]

本次工作基于 PR #39，围绕“窥孔优化器 Benchmark”方向进行了补充。完善了 benchmarks/bench_asm_peephole.py，复用统一汇编解析器，提供确定性的测试样例、指令数量统计、规则命中次数、指令削减率、运行时间和输入哈希等指标；新增 benchmarks/compare_peephole.py，用于在相同样例上对比开启和关闭窥孔优化器时的结果，并生成 JSON 数据报告和 HTML 可视化报告。同时补充了 tests/test_bench_asm_peephole.py 和 tests/test_compare_peephole.py，更新了相关设计文档、开发文档和 topic13/README.md。当前默认基准包含 14 个样例，测试结果显示指令数由 31 条减少到 22 条，减少 9 条，削减率约为 29.0%。本次未修改窥孔优化器核心实现。

[github-actions]

🤖 AI Code Review

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

📁 benchmarks/compare_peephole.py

Code Review: benchmarks/compare_peephole.py

🔴 Bug: Integer counts rendered with 3 decimal places in HTML

_bar_row always formats value with {value:,.3f}, so instruction counts like 12 show as "12.000" and rule matches as "3.000" — confusing and incorrect for integer quantities.

Fix: Add a value_fmt parameter or detect integer values:

fmt = "{:,}" if value == int(value) else "{:,.3f}"
f'<td class="bar-value">{fmt.format(value)}{_escape(suffix)}</td>'

🔴 Bug: maximum_matches fallback is dead code

Line: max([int(rule_matches.get(name, 0)) for name in PR39_RULES] or [1])

[0, 0, 0] is a non-empty list → truthy → or [1] never triggers. When all counts are 0, maximum_matches = 0, then _bar_row's maximum <= 0 guard catches it, so no crash — but the or [1] is misleading dead code that won't do what it appears to.

Fix: Use a clearer pattern:

matches_list = [int(rule_matches.get(name, 0)) for name in PR39_RULES]
maximum_matches = max(matches_list) if max(matches_list) > 0 else 1

🟡 Misleading zero timing for peephole_off

The peephole_off block reports "elapsed_ms_median": 0.0, implying a measurement was taken and yielded 0ms. In reality no timing occurs for the off path.

Fix: Use null or omit the field:

"peephole_off": {
    "enabled": False,
    "instructions": before,
    "changes": 0,
    "elapsed_ms_median": None,  # no measurement taken
},

And update _print_summary / HTML to handle the None case.

🟡 # flake8: noqa disables all linting file-wide

This silences every warning (unused imports, undefined names, etc.) for the entire file. Use targeted ignores only where needed:

# flake8: noqa: F401, E501  # specific rules only

🟡 No validation that rule_matches keys match PR39_RULES

If optimizer.total_matches contains unexpected keys (new rules added later, or typos), they're silently ignored. Consider logging a warning or asserting key alignment during development.

💭 _optimize_with_timing returns last-iteration state only

The loop runs repeats times for timing, but output/changes/rule_matches come from the final iteration. Since input is identical each time this is correct — but a brief comment noting this intent would prevent future confusion.

💭 html.escape called on already-escaped data in some paths

Values flow through compare_cases → generate_html_report where _escape is applied. This is safe (double-escaping only affects < > & ", and data like hashes/descriptions don't contain these). Not a bug, but worth a comment that escaping is expected to be idempotent.

---

Priority summary: The 3-decimal formatting bug will produce visibly wrong output in the HTML report. The dead-code and misleading zero-timing are correctness/documentation issues. The rest are polish.

---

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

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

---

🟡 结构矛盾：§9.1 已实现规则列在"建议新增"下

§9.1 表格第一行 addi-zero / nop / mv-self — ✅ 已实现（Rule 5–8） 位于"建议新增规则"章节，读者会困惑这些是否还要做。建议移除该行或将其移至 §6 规则目录作为补充引用。

---

🟡 §2.3 非目标章节混入已纳入目标

"已纳入目标（勿再当作非目标）"段落放在非目标章节内，语义冲突。建议单独起一个 ### 2.4 已修正/纳入目标 小节，或直接删除（这些信息已在 §6、§8 重复出现）。

---

🟡 测试计数表述不一致

• 文档开头：94 项相关测试通过
• §7.2 命令注释：优化器回归测试（84 项）
• §7.4 表格：84 PASSED + 10 PASSED = 94
• §11 清单：84 个优化器回归 + 10 个 benchmark 测试

§7.1 的"五类测试概览"表格没有给出每类具体数量，导致读者难以验证总数。建议在表格中增加"测试数"列，或在开头统一用 84 + 10 = 94 的格式。

---

💭 代码围栏格式不统一

§7.5 使用 ~~~bash 而 §7.2、§7.5 末尾、§12 末尾使用 ~~~。建议统一为 ``` 围栏（Markdown 中两种等价，但风格应一致）。

---

💭 §8 技术债表混合了已解决和未解决项

✅ 已修复 和 ✅ 已移除 的行（立即数溢出、假交换删除）仍然占据"技术债"表格，与未解决项并列。建议拆为"已解决"和"未解决"两张表，或添加"状态"列（open/closed）以便筛选。

---

💭 §7.6 Benchmark 口径描述与数据表混排

统计口径表后紧跟 bash 命令，再接规则覆盖套件描述段落，层次不够清晰。建议用子标题分离：### 统计口径、### 运行命令、### 规则覆盖套件说明。

---

总结

文档整体质量高、技术内容准确。主要问题是结构层面的组织——已实现内容混入"建议/非目标"章节，以及测试计数分散表述。无 🔴 blocker。

---

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

🔴 No blockers found — the diff is a documentation update with a correct and important bug fix (removing the false-swap elimination rule).

---

🟡 Suggestion: mv-chain liveness pitfall is understated — the pitfalls table says "中间 a 后续仍使用时可能不健全" but doesn't flag that this is a soundness hole in the current 8-rule set, not just a theoretical concern. If a generated assembly ever has mv a,b; mv c,a where a is later read, the optimizer will silently produce wrong code. Consider either (a) adding a guard in the rule implementation (liveness check for a post-window), or (b) adding a prominent ⚠️ to the rule table row so implementers know to be cautious.

🟡 Suggestion: benchmark precision looks like a leak — -29.032% in the summary table suggests this came from a single local run rather than a documented benchmark methodology. If these numbers are meant to be canonical, they should match the exact JSON produced by bench_asm_peephole.py; if not, consider marking them as "示例数据（本地冒烟）" to avoid readers treating them as guarantees.

💭 Nit: private API in docs — _asm_parser.parse_asm uses an underscore-prefixed (private) name in the benchmark description. Either re-export it as a public API or rephrase to "the shared asm parser" to avoid teaching readers to reach into internals.

💭 Nit: code fence delimiter inconsistency — the new Benchmark section uses ~~~bash while the rest of the file (and the existing code blocks) uses triple backticks. Consider standardizing on one style for consistency with the linter/previewer.

💭 Nit: dropped line count metadata — the original header had **行数**：~400 which was removed. If the file has changed significantly in size, consider re-adding an approximate count for readers who want a quick complexity estimate.

---

📁 scratchv/compiler.py

🔴 Bug: Double register allocation when reg_alloc="linear" — Lines 406–417: RegisterAllocator(machine_instrs, mode=self.config.reg_alloc) runs with mode="linear", then its output is fed into LinearScanAllocator. If RegisterAllocator's "linear" mode is itself a linear-scan implementation, you're now allocating twice — the second pass sees pre-allocated registers/spills and may emit invalid code or no-op silently. Verify whether the pre-RegisterAllocator run is intentional (as a canonicalizer) or accidental. If it's a pre-pass, pin it to a fixed mode (e.g. mode="none" or a dedicated pre-pass class) rather than honoring user config.

🔴 API mismatch risk on get_allocated_code — Line 418: lsa.get_allocated_code(ls_insts). Prior code used lsa.emit(ls_insts). Confirm the signature — if it actually expects intervals (the live-interval data you just computed) rather than ls_insts, this either silently produces wrong asm or throws. Worth a one-line doc/type-check on the allocator's public surface.

🟡 Breaking default change — Line 60: reg_alloc default flipped from "linear" to "greedy". Any existing user or downstream test relying on the old default will now see different codegen and possibly different numeric results (relevant given rtol is configured). Add a changelog note, or gate behind an explicit opt-in.

🟡 Optimizer stats coupled via ad-hoc attributes — Lines 454–457: opt.instructions_saved / _before / _after are read as attributes. If these are internal state rather than a defined interface, a future refactor silently breaks the warning. Consider returning a small OptimizationResult(changes, before, after) namedtuple from opt.optimize() instead of mutating instance state.

🟡 Pass ordering ambiguity — Greedy now always runs before the linear-scan branch, but does not run when reg_alloc="linear" in the old design. If greedy is a "pre-alloc" that the linear scanner depends on, that dependency should be documented in the function docstring or enforced in RegisterAllocator itself — currently it's implicit in the call site.

💭 Comment clarity — Line 409 # Optional: use linear-scan instead is misleading; the branch is now a mandatory early-return after pre-allocation, not an opt-in alternative. Suggest: # Alternative backend: full linear-scan (uses allocated output as input).

💭 Warning verbosity — New warning embeds three numeric fields per peephole pass. If many such warnings accumulate in warnings, consider a structured log entry or aggregation (e.g., one summary warning at the end) to avoid log spam in CI.

💭 Test coverage — None of the diff touches tests. At minimum: (a) assert that reg_alloc="linear" produces the same result as before this change (regression), (b) assert reg_alloc="greedy" default still passes the existing golden outputs, (c) a unit test that RegisterAllocator in "linear" mode is idempotent or explicitly not double-runnable.

---

📁 tests/fixtures/asm_peephole/input_addi_fusion.s

🟡 Missing description — No comment explaining the expected optimization.
Suggestion: Add a header comment, e.g. # Expect fusion: addi t0,t0,3 + addi t0,t0,5 → addi t0,t0,8.
Fixtures are read in isolation; a future reader won't know the intended transformation without context.

🟡 No edge-case companion — This only covers the happy path.
Suggestion: Consider adding companion fixtures for:

• Imm32 overflow (addi t0,t0,65536 + addi t0,t0,3) — should not fuse.
• Interleaved register use (addi t0,t0,3 / addi t1,t1,1 / addi t0,t0,5) — should not fuse.
• Signed immediates near boundary (addi t0,t0,2047 + addi t0,t0,1).

Without these, a broken peephole pass could ship undetected.

💭 No .globl / linkage — If the test harness links the output, main won't be findable.
Suggestion: Add .globl main if the framework links these fixtures; otherwise add a comment noting it's assemble-only.

---

📁 tests/fixtures/asm_peephole/input_addi_overflow.s

No issues with the code itself — clean, focused test fixture.

🟡 Suggestion — 确认有对应的 expected output 文件（如 output_addi_overflow.s），且期望值明确验证：两条 addi 不会被合并为单条 addi t0, t0, 4000（4000 超出 12-bit signed immediate 范围 −2048~2047）。如果 expected 文件缺失或内容不精确，测试可能无法捕获错误的优化行为。

---

📁 tests/fixtures/asm_peephole/input_beq_zero.s

🔴 Always-taken branch — expected output missing from this review? — The fixture encodes beq x0, x0, target (always-taken, register ≠ itself is impossible). Confirm the corresponding expected-output fixture (e.g. output_beq_zero.s) exists with b target. Without it, this test is incomplete.

🟡 Missing complementary case — Consider adding a sibling fixture for bne x0, x0, target (always-not-taken), which is a different peephole pattern (branch elimination / dead-code removal). The two cases exercise different optimization paths.

💭 No .global / symbol declaration — If the test runner expects a well-formed ELF, add .global main. Otherwise the fixture works fine as raw input text — just confirm the test harness doesn't assemble/link.

---

📁 tests/fixtures/asm_peephole/input_hex_fusion.s

🟡 **Coverage Gap: No negative tests** — This fixture only covers the happy path (fusable case).
Consider adding companion fixtures for:
  - Non-fusable pair (intervening instruction between the two `addi`s)
  - Different destination registers (`addi t0,...` + `addi t1,...`)
  - Immediate overflow (e.g., `0x7F0` + `0x10` exceeds 12-bit signed range)

🟡 **Missing `.global main`** — Line 2: entry point is not exported. If the test harness assembles + links this to a standalone ELF, the linker may not emit `main` into the symbol table. Add `.global main` before `main:`.

💭 **Overflow boundary not documented** — The peephole rule must know whether `0x10 + 0x20` is within `imm[11:0]` bounds. A brief comment (e.g., `# imm sum = 0x30, within range`) makes the test's intent self-documenting for future contributors.

---

📁 tests/fixtures/asm_peephole/input_li_addi.s

🟡 **Test Coverage Gap** — Only covers the happy path (fold `li` + `addi` → `li`).
Suggestion: Add companion fixtures for:
- Non-foldable case (`addi` on a register not set by prior `li`)
- Immediate value overflow (>17-bit range for `li` / >16-bit for `addi`)
- Negative immediate results (`li t0, -20; addi t0, t0, 3`)

💭 **Naming** — File name `input_li_addi.s` is fine but consider `input_li_addi_fold.s` to clarify intent vs. a sibling `input_li_addi_no_fold.s`.

💭 **Missing `expected` output reference** — Ensure the test harness pairs this with an expected output file (e.g., `expected_li_addi.s` containing `li t0, 15`). Without it, the fixture alone cannot validate the optimization.

---

📁 tests/fixtures/asm_peephole/input_mv_chain.s

🔴 Missing expected-output fixture — A peephole test typically needs a paired output_mv_chain.s (or equivalent) that asserts the optimized result. Without it, there's no assertion — CI would silently pass on a broken optimizer.
Suggestion: Add the corresponding expected output file (e.g., where mv t2, t0 is folded to mv t2, t1 and the redundant mv t0, t1 is eliminated or rewritten).

🟡 No test harness reference — The fixture exists in isolation. Ensure the test runner (e.g., tests/run_asm_peephole_tests.sh or a language-specific harness) actually loads this fixture and compares against the expected output. If the runner isn't wired up, this fixture is dead code.

---

---

⚠️ 未审查的文件

• 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
• tests/test_bench_asm_peephole.py
• tests/test_compare_peephole.py
• topic13/README.md