Skip to content

docs(topic13): peephole optimizer docs and tests - #39

Open
zying333 wants to merge 1 commit into
ScratchV-Compiler:mainfrom
zying333:docs/topic13-peephole
Open

docs(topic13): peephole optimizer docs and tests#39
zying333 wants to merge 1 commit into
ScratchV-Compiler:mainfrom
zying333:docs/topic13-peephole

Conversation

@zying333

@zying333 zying333 commented Aug 1, 2026

Copy link
Copy Markdown

Summary

  • 汇编窥孔:8 条默认规则(已移除不健全的假交换删除)
  • report 打印指令前后数量与节省数
  • 文档:topic13 README / 设计文档 / 开发文档

Test plan

  • pytest tests/test_asm_peephole*.py → 84 passed

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

🤖 AI Code Review

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

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

🔴 技术债遗漏:x0/zero 别名未全量统一 — 第8节指出“beq规则接受x0/zero;其他规则字符串比较”,但未提及 li rd, 0mv rd, x0 的等价性。若 addi+addi 规则中遇到 addi x0, x0, 0 等,可能因字符串比较错过优化或导致错误。建议在规则引擎中统一别名映射,或至少记录该限制的明确影响范围。

🟡 第2.3节“非目标”与“已纳入目标”自相矛盾 — 非目标列表未包含“溢出拒绝、标签阻挡、假交换保留”,但下文又强调“勿再当作非目标”。建议将已实现的目标移出非目标列表,或改为“已解决的历史非目标”单独说明,避免读者困惑。

🟡 第4.1节 PeepholeRule.register_constraints 描述模糊(dst_idx, src_instr_idx, src_op_idx) 未说明索引的基准(是窗口内指令序号还是全局序号?),且未给出具体示例。建议补充一条示例,如 (0, 1, 1) 表示第0条指令的rd必须等于第1条指令的第1个源操作数,并解释整数索引如何映射到操作数列表。

💭 第5.2节匹配逻辑第4点不够完整 — 只说明“窗口内第2条及以后若带label,拒绝匹配”,但未提及窗口首条标签的处理。后续第5.3节提到“窗口首条标签转移到替换结果”,但此处未联动。建议在匹配逻辑中明确首条标签允许保留,并补充“标签安全”检查的完整规则。

💭 第6节规则目录表格格式不一致 — Rule 4的约束列以文本描述代替结构化表达式,而其余规则均为简洁的符号约束(如“同rd”)。建议统一为类似 (rs_of_second == rd_of_first) AND NOT (swap pattern) 的符号形式,或补全表格的约束列定义。

💭 第7.7节性能基准表中“变更次数”与“行数减少”关系不明 — 两列数值几乎相同,但“变更次数”可能包含非删除的替换(如 addi->mv),而“行数减少”仅统计净减少行数。建议在表格下方增加脚注说明两者定义,或合并为一列“净减少行数”。


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

🔴 日期错误 — 状态行 2026-08-01 是未来日期,应为 2025-08-01 或当前实际完成日期。

🟡 用词不规范addi+addi fusion 表格中“和须 ∈ [-2048,2047]” 的“和须”应为“和必须”或“和需”。建议改为“和必须在区间 [-2048, 2047] 内”。

🟡 措辞不精准 — 坑列表“mv 链活跃性”中“不健全”应改为“不安全”或“不正确”,因为 a 后续仍使用时优化会改变语义,不仅是健全性问题。

💭 表格对齐 — 表格中“和须”后缺少空格,[-2048,2047] 建议加空格,如 [-2048, 2047],与常见表示一致。


📁 docs/topics/archive/topic13_asm_peephole_guide.md

🔴 日期错误Last verified: 2026-08-01 是未来日期,应修正为实际验证日期或移除该行。

🟡 report() 方法缺失:文档称 opt.report() 可用,但 Public API Contract 中未列出该方法。若代码已实现,应补充到 API 部分;否则删除该引用。

🟡 测试计数过时风险:文档声称 83/83 测试通过且默认规则 8 条。若规则或测试数量变更,需同步更新,否则误导读者。

🟡 规则4 swap 排除逻辑不明确redundant mv elimination 排除了 swap 形状,但文档未说明实现方式(如检查 window[0].operands[1] == window[1].operands[0])。建议补充匹配逻辑或约束条件。

💭 死代码信息应移至内部注释_split_operands 被列为“dead code; ignore or cleanup PR”,这类信息更适合放在代码 TODO 或开发者注释中,而非面向用户的文档。


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

🟡 清晰度:在“详细任务”第1点中,“addi x1, x1, 1; addi x1, x1, 1addi x1, x1, 2(和须落入 12 位有符号立即数)”的“和”表述模糊。建议改为“两次立即数之和须在 12 位有符号范围内”。

💭 格式:规则列表中的“addi x, x, 0 / nop / mv x, x”使用斜杠分隔,易混淆。建议改用逗号或分项列出,如“addi x, x, 0nopmv x, x → 删除”。

💭 日期验证:状态日期为 2026-08-01,若为计划日期请明确标注,若为实际完成日期则无问题。

💭 路径检查:完成目录链接 ../../../topic13/README.md 相对于当前文件(docs/topics/archive/),解析后为 docs/topic13/README.md,建议确认目标目录是否存在。


📁 scratchv/backend/asm_peephole.py

🔴 Bug: Potential IndexError in swap check — Line 387: window[1].operands[0] and window[1].operands[1] accessed without checking len(window[1].operands) >= 2. If window[1] has fewer than 2 operands (e.g., nop or a malformed line), this will crash. Add a guard.

🟡 Suggestion: Missing imm validation for addi-zero rules — Lines 409–410 and 415–417: _parse_imm(ops[2]) is called after checking len(ops) < 3, but ops[2] may be a label or register (e.g., addi x1, x2, foo). _parse_imm returns None for non-integers, and the rule would be skipped. That’s correct, but consider adding a comment to clarify that the pattern expects a numeric immediate, else the rule is silently skipped.

🟡 Suggestion: _fits_simm12 should guard against integer overflow — Python int is unbounded, so _fits_simm12 is fine, but if the sum exceeds Python int range (unlikely), it’s still safe. No action needed.

💭 Nit: Redundant check in _match_rule for addi-zero self elimination — The register constraint (0,0,1) already ensures rd == rs. The explicit ops[0] == ops[1] check is redundant but harmless. Could be removed for clarity.

💭 Nit: _apply_replacement for deletion rules preserves label but loses comments — Comments from the original instruction are dropped. If this is intentional, it’s fine; otherwise, consider preserving comments from the first instruction.

🟡 Suggestion: report() duplicates "Total changes" line — Line 670 adds a legacy line, but line 648 already outputs rule applications. This is okay for backward compatibility, but consider documenting it.

Otherwise, the changes are sound: improved safety with immediate range checks, label preservation, and new rules are well-implemented. The move to explicit _parse_imm avoids silent failures.


📁 scratchv/compiler.py

🔴 潜在逻辑错误: 双重寄存器分配 — 新 _generate_riscv_linear 中,当 reg_alloc == "linear" 时,先调用 RegisterAllocator(machine_instrs, mode="linear") 进行分配(第406–407行),随后又用 LinearScanAllocator 在已分配的结果 allocated 上再次分配(第412–416行)。这可能导致寄存器被错误覆盖或指令被二次处理,产生不可用的汇编。建议:在线性分支中移除 RegisterAllocator 调用,或确保 RegisterAllocator 在线性模式下是空操作(no-op)。

🟡 默认值变更可能影响兼容性reg_alloc 默认值从 "linear" 改为 "greedy",若外部代码或测试依赖默认线性扫描,需要更新或适配。

💭 警告信息缺少属性安全校验_run_asm_passes 中使用了 opt.instructions_saved 等属性,若 AsmPeepholeOptimizer 未能设置这些属性(如优化未运行),会触发 AttributeError。建议添加属性存在性检查或提供默认值。


📁 tests/fixtures/asm_peephole/input_addi_fusion.s

🔴 Missing global declaration — 入口符号 main 未声明为 .globl,可能导致链接器无法识别。建议添加 .globl main

💭 缺少尾部换行 — 文件最后一行 (ret) 后无换行符,可能在部分工具链中引发警告。建议确保文件以换行结尾。


📁 tests/fixtures/asm_peephole/input_addi_overflow.s

💭 建议添加注释 — 此测试用例用于验证 peephole 优化器不会将两个 addi 合并为 addi t0, t0, 4000(因为4000超出12位有符号立即数范围)。添加简短注释可帮助其他开发者理解测试意图,提升可维护性。


📁 tests/fixtures/asm_peephole/input_beq_zero.s

💭 缺少注释说明测试目的 — 当前文件没有注释,未来的维护者可能不清楚这个测试用例意图(测试 beq x0, x0 总是跳转的特殊情况)。建议在文件开头添加一行注释,如 # Test that beq with x0, x0 (always equal) is handled correctly.


📁 tests/fixtures/asm_peephole/input_hex_fusion.s

🟡 Suggestion: 增加边界测试覆盖 — 当前测试只覆盖了两个正立即数(0x10 和 0x20)合并为 0x30 的场景。建议增加负立即数(如 -0x10-0x20)、立即数相加后溢出 12-bit 范围(如 0x8000x800,合并后为 0x1000 超出范围,可能触发其他优化路径或报错)、以及目标寄存器不同(如 addi t0, t0, 0x10; addi t1, t0, 0x20)等场景,以更全面地验证 hex fusion peephole 优化的正确性和边界处理。



⚠️ 未审查的文件

  • 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.

1 participant