Skip to content

feat(peephole): reuse shared _asm_parser and harden match guards (topic13 track A) - #43

Open
zinoe-1 wants to merge 2 commits into
ScratchV-Compiler:mainfrom
zinoe-1:feat/topic13-peephole-parser-reuse
Open

feat(peephole): reuse shared _asm_parser and harden match guards (topic13 track A)#43
zinoe-1 wants to merge 2 commits into
ScratchV-Compiler:mainfrom
zinoe-1:feat/topic13-peephole-parser-reuse

Conversation

@zinoe-1

@zinoe-1 zinoe-1 commented Aug 4, 2026

Copy link
Copy Markdown

Summary

  • Reuse scratchv/backend/_asm_parser.py inside asm_peephole (remove duplicate parser)
  • Harden redundant mv elimination swap check (operand length guards)
  • Normalize x0/zero aliases in register constraint matching
  • Add track-A design/dev docs(作者:孟子旭)+ regression tests
  • Fixture cleanup: .globl main + intent comments

Baseline

Based on PR #39 (docs/topic13-peephole / d62acdf). This branch includes that tip plus track-A follow-ups. Please review in relation to #39.

Test plan

  • pytest tests/test_asm_peephole*.py → 88 passed

zying333 and others added 2 commits August 1, 2026 16:19
Topic 13 track A follow-up on PR ScratchV-Compiler#39: drop duplicate asm parser,
normalize x0/zero aliases, guard short-operand mv swap checks, and
add design/dev docs plus regression tests.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

🤖 AI Code Review

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

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

🟡 文档表述突兀 — §2.3 非目标中,“已纳入目标(勿再当作非目标)” 这更像是对之前 review 的回应,而非设计文档的正式描述。建议改为更自然的表述,如“已解决:addi+addi 的 simm12 溢出拒绝;中窗标签拒绝融合;删除时保留标签;假交换对不删除。” 或直接移除括号内的提醒。

🟡 规则目录表模式列可更精确 — §6 表 Rule 3 模式列仅写 beq,但实际只匹配 rs1, rs2 为 x0 或 zero 的情况。建议在模式列或说明中明确,以避免误解。

💭 性能基准表单位 — §7.6 表格中耗时列标题为“耗时(ms)”,但数值如 0.6 等,建议确认单位是否一致(如是否应为 ms?0.6ms 合理)。若为 ms 则无问题。

💭 文档中重复强调“假交换删除已移除” — 该点已在 §6、§7.4、§8 等多处出现,可适当合并,但保持清晰度亦可接受。


📁 docs/topics/13-窥孔优化器-赛道A-开发文档.md

🔴 文档缺少关键测试用例 — 第 4.1 节只列出 3 个新增场景,但第 3.3 节提到的边界条件(如 x0/zero 混用在 beq 中的约束)未在测试计划中体现。建议补充:beq x0, zero, label 等价匹配、li rd,0 不等价折叠的已知限制不作为测试遗漏。

🟡 实现步骤 3 描述不完整 — 第 3.2 节只说“别名 + IndexError”,未说明 _regs_equal 具体如何实现(例如是否同时处理 x0zero 的多种形式)。建议增加一行描述:_regs_equal 将寄存器名统一规范化为小写后再比较,且对 x0/zero 做别名映射。

🟡 验收标准缺少 CI 集成验证 — 第 4.3 节只要求 pytest 全绿,但未提及在 CI 环境中运行 --peephole-asm 的 end-to-end 测试(如 compiler 模块调用)。建议增加一条:python -m scratchv.backend.asm_peephole fixture/bad_input.s 不崩溃。

💭 第 5 节风险项第三点建议 — “共享 lines_to_asm 格式细微差异” 风险缓解措施可更具体:建议添加一条黑盒测试,对比新旧 parser 输出的文本在语义上等价(例如通过汇编器重汇编后相同)。

💭 第 2.2 节内部设计层次可简化 — 当前列出 3 步流程,但未提到 _canon_reg 等辅助函数何时被调用。可考虑在第 2.2 节末尾加一句:_canon_reg 用于规则匹配时统一寄存器表示,降低维护成本。


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

🔴 依赖冲突风险 — 文档声称“基线即为 PR #39 tip”,但若 PR #39 尚未合并,主分支无法直接应用本 PR。建议明确说明本 PR 是否包含 #39 的变更(即 squash 后一起提交),否则需在 PR 描述中注明合并顺序。

🟡 测试设计不完整 — 仅列出用例描述,未给出具体输入/输出示例或断言。建议补充每个用例的预期结果(如 mv t0 畸形行应返回原行不优化,且不抛异常),以便 reviewer 验证。

🟡 未知的 _canon_reg 函数 — 第 2.2 节提到 _canon_reg,但文档中未说明其定义位置或行为。建议补充说明该函数位于 _asm_parser 中,或明确其规范化规则(如 zerox0,但 x0 不变)。

🟡 “刻意不做” 的边界 — 第 2.3 节声明 li rd, 0mv rd, x0 不做模式统一,但未说明原因。如果后续优化器规则中有 li rd, 0 的消除,可能会与本次映射冲突。建议添加一句“已知限制,未来可在规则层面扩展”。

💭 架构图使用 ASCII 箭头 — 可读性尚可,但建议在渲染的 Markdown 中改用 UML 或 Mermaid 图表(若工具链支持),以提升视觉清晰度。

💭 风险评估中“directive 表示变化”描述模糊 — “去点号”具体指什么?旧 parser 用 line.startswith('.') 判断,新 parser 用 is_directive 属性,但属性值是否包含点号?建议明确新旧表示差异(如旧:.text 为 directive,新:text 也是 directive?)。


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

🔴 事实错误:mv 交换的语义描述不准确 — 文档中“已移除的错误规则”说明“结果是两个寄存器都变成原来的 y”是错误的。实际执行 mv x,y; mv y,x 后,x 变为原 yy 变为原 x,并非两者都等于原 y。建议改为:“这两条指令实现的是交换,而不是空操作,删除会改变语义。”

🔴 笔误:addi+addi fusion 的注释 — “和须 ∈ [-2048,2047]”中的“和须”应为“和必须在”。建议改为“和必须在 [-2048,2047] 范围内”。

🟡 陷阱描述不够精确 — “mv 链活跃性”说明“在中间 a 后续仍使用时可能不健全”。建议明确:只有当 a 后续不再被使用时,mv a,b; mv c,a → mv c,b 才是安全的。否则替换会丢失 a 的原值。

💭 格式小问题 — “已移除的错误规则”用 > 引用,但下一行直接跟 这两条**不是**真交换,建议保持引用块内一致。


📁 docs/topics/archive/topic13_asm_peephole_guide.md

🔴 Date error — Line 19: Last verified: 2026-08-01. This is a future date; likely a typo or placeholder. Update to a real date or remove.

🟡 Misleading _LINE_RE description — Module map says _LINE_RE/_parse_line are "Single-line parse", but the actual regex is used inside _parse_line. The doc implies they are separate symbols; consider clarifying that _LINE_RE is a module-level regex constant used by _parse_line, not an independent parsing entry point.

🟡 {rs1} template placeholder listed but not used for addi+addi fusion — Replacement templates table says {rs1} is set by addi+addi fusion, but the actual replacement template in the code is addi {rd} {rs1} {imm_sum}. Confirm that {rs1} is indeed the operand from the first instruction (second operand of addi). The documentation should note that {rs1} is the first source operand of the first addi (window[0].operands[1]).

🟡 Missing constraint meaning for rule 4register_constraints: (0,1,1) for redundant mv elimination is not explained. The table says "excludes swap shape", but the constraint itself is cryptic. Add a short note: (0,1,1) means window[0].operands[0] == window[1].operands[1].

🟡 Checklist item "Did not break IR peephole" is out of scope — Line 297: - [ ] Did not break IR peephole (optimizer/peephole.py). This is an ASM peephole guide; IR peephole is a separate module. Including this check gives false confidence. Remove or rephrase to "Check that optimizer/peephole.py tests still pass if you touched shared utilities".

💭 _split_operands dead code mention — Pitfalls table lists _split_operands as defined but unused. Good to document, but consider adding a note that it's intentionally left as a hook for future shared parsing.

💭 Pass order in _run_asm_passes — Line 47: peephole → const_merge → schedule → beautify. The order may be correct, but verify that const_merge (constant merging) before scheduling is intentional. If const_merge rewrites li instructions, it might affect peephole later. Consider adding a rationale comment.

💭 Minor: test_mv_chain_unsound_when_mid_live — This test is listed in Test Coverage Matrix, but the doc warns about liveness gap. The test name is accurate; no change needed.


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

💭 格式细节:第4行原文 **难度**:低 末尾有两个空格,建议去除(可能无意添加的 Markdown 换行符)。

🟡 链接完整性:第5-7行添加了多个相对路径链接(如 ../../../topic13/README.md)。若文档移动或目录结构变更,这些链接可能失效。建议确认所有目标文件确实存在,或考虑使用绝对路径(如 /docs/...)以提高可移植性。

🟡 接口一致性:第17行将编译开关从 --peephole 改为 --peephole-asm。请检查后端编译器代码、命令行参数文档、用户手册等是否已同步更新,避免接口不匹配。

💭 规则描述清晰度:第12行规则 mv a, b; mv c, amv c, b 标注为“best-effort”,但未明确说明安全条件(中间寄存器 amv c, a 之后不再被使用)。虽然括号内已提及,但建议在主规则描述中直接强调“仅当 a 不再被后续引用时”,避免误用。

💭 删除线语法:第13行使用 ~~\mv x1, x2; mv x2, x1` → 删除~~表示被删规则。反引号内包含反斜杠转义,实际渲染效果可能因 Markdown 解析器而异(部分引擎会将` 视为字面反斜杠)。建议改为 ~~mv x1, x2; mv x2, x1 → 删除~~(去掉反斜杠,反引号直接包裹即可),以确保删除线正确包裹代码片段。


📁 scratchv/backend/asm_peephole.py

🔴 Bug: 删除规则生成的 label 行格式可能不兼容_apply_replacement 中删除规则时返回 AsmLine(raw=f"{lead_label}:", label=lead_label),但 ParsedAsmLine__str__ 可能也会自动添加冒号,导致双重冒号或格式错误。需确认共享模块的序列化行为。

🟡 行为变化:redundant mv elimination 约束从 (1,0,0) 改为 (0,1,1) — 修复了旧 bug(检查了错误的寄存器),但可能改变已有匹配结果,需确保测试覆盖新旧行为。

🟡 解析首尾空白行:_parse_asm(asm_text.strip()) 可能改变语义 — 历史 parser 在 strip() 后仍保留中间空行,但共享模块可能丢弃空行,导致行号偏移或测试失败,需验证。

🟡 依赖 ParsedAsmLine.is_directive 属性_count_opcodes 使用 al.is_directive,但该属性可能未定义,会导致 AttributeError。需确认共享模块中存在此字段。

💭 默认规则集无法禁用单个规则 — 新增规则(如 nop 消除)可能不符合部分用户场景,建议提供禁用选项或文档说明。

💭 报告输出中 report() 的 "Total changes" 行与 main() 中的 summary 行共存 — 虽为向后兼容,但可能被解析脚本误读,需注明。


📁 scratchv/compiler.py

🔴 潜在寄存器分配错误 — 当 reg_alloc == "linear" 时,会先调用 RegisterAllocator(machine_instrs, mode=self.config.reg_alloc)(此时 mode"greedy",因为默认改了),得到已分配的 allocated,再将其传给 LinearScanAllocator 重新分配。线性扫描期望未分配的指令,直接使用已分配结果可能导致寄存器冲突或重复分配。建议:线性扫描路径应直接使用 machine_instrs,跳过 RegisterAllocator

🟡 方法名与默认行为不一致_generate_riscv_linear 方法名暗示线性扫描,但默认 reg_alloc 已是 "greedy",逻辑反而走 AsmEmitter 路径。建议重命名或添加注释消除歧义。

🟡 性能浪费(线性路径) — 线性扫描时先执行了 greedy 分配(alloc.run()),再执行线性扫描,做了两次寄存器分配,浪费计算资源。仅在 reg_alloc == "linear" 时不应执行 RegisterAllocator

🟡 缺少测试覆盖 — 默认 reg_alloc"linear" 改为 "greedy",可能破坏依赖线性分配结果的现有测试。需确认测试是否通过,并补充新默认行为的测试。

💭 警告信息依赖新属性_run_asm_passes 中使用了 opt.instructions_savedopt.instructions_beforeopt.instructions_after,需确保 AsmPeepholeOptimizer 已定义这些属性,否则会在运行时异常。


📁 tests/fixtures/asm_peephole/input_addi_fusion.s

🟡 Suggestion: Add clarifying comment — 文件缺少注释说明测试目的。
建议在文件头部添加类似 # Test that two consecutive addi instructions are fused into one addi with combined immediate. 的注释,提高可读性。


📁 tests/fixtures/asm_peephole/input_addi_overflow.s

💭 测试文件设计正确,未发现任何问题。



⚠️ 未审查的文件

  • 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants