topic14-docs

[yuki-328]

No description provided.

[github-actions]

🤖 AI Code Review

共审查 10 个变更文件

📁 benchmarks/bench_const_merge.py

🔴 Bug: Infinite loop in _gen_synthetic_asm — Lines 67-90: The if/elif block has no else branch. When random.random() returns a value greater than redundant_lui_ratio + lui_ratio (possible when the sum < 1.0), no branch executes and i never increments, causing an infinite loop.
Suggestion: Add an else branch that generates a single "normal" instruction and increments i by 1.

🟡 Fragile import path — Lines 14-16: sys.path.insert(0, PROJ_DIR) overrides standard imports and breaks if the script is moved or imported as a module. Consider using from ..backend import ... (relative import) or installing the package with pip install -e ..

🟡 Misleading output in density test — Lines 179-180: The printed ratio=0.1 only reflects lui_ratio; the fixed redundant_lui_ratio is not shown. This can confuse readers.
Suggestion: Print both densities, e.g. pair_ratio=0.1, redundant_ratio=0.1.

💭 Unclear docstring — Line 49: redundant_lui_ratio description says "redundant LUI pattern" but doesn't explain the pattern (two consecutive LUI to same register with an unrelated instruction in between). Clarify for future maintainers.

💭 Redundant changes_list computation — Line 119: r[1].total_changes is accessed inside a loop, but stats from merge_constants_detailed is assumed to have this attribute. Ensure the attribute name matches the actual return type.

---

📁 benchmarks/run_benchmark.py

🔴 错误处理缺失 — 第 208-215 行：merge_constants_detailed(asm_str) 可能抛出异常（如 ASM 解析失败），导致整个基准测试崩溃。建议用 try-except 包裹，捕获异常并记录到 result.error，或跳过优化统计。

🟡 导入位置不一致 — 第 6 行顶部的 from scratchv.backend._asm_parser import 与第 200 行函数内部的 from scratchv.backend.const_merge import merge_constants_detailed 不统一。建议将 merge_constants_detailed 的导入也放在文件顶部，避免延迟导入带来的可读性损失和潜在循环依赖（若无特别原因）。

🟡 未使用的字段 — 第 65-71 行新增的 machine_instructions_before/after、code_size_before/after、output_equal 被初始化为 None，但在 run_benchmark 中从未被赋值。这些字段会始终为 None，可能造成数据误解。建议要么移除，要么在后续实现中赋值，或添加注释说明待实现。

💭 函数命名 — _count_parsed_asm_instructions 可简化为 _count_instructions（已明确参数类型为 list[ParsedAsmLine]），减少冗余。_count_asm_instructions 同理。

---

📁 benchmarks/test_benchmark.py

🔴 Bug: Incorrect test case — Line 44, parameter (0.6, 0.5): both values are valid probabilities (0–1), so _gen_synthetic_asm should not raise ValueError. Either remove this test case or change it to an invalid combination (e.g., (1.1, 0.0) is already covered).
🟡 Fragile assertion — Lines 179–183: reduction == tracked_changes assumes the only changes to instruction count are from merging and LUI removal. Any other optimization (e.g., dead code elimination, constant folding) will break this test. Consider relaxing to reduction >= tracked_changes or asserting only when other optimizations are disabled.
💭 Private import — Line 26: from benchmarks.bench_const_merge import _gen_synthetic_asm imports a private function. While acceptable in tests, consider renaming the function to public if it's intended for testing.

---

📁 docs/课题14-常量加载合并优化-开发文档初稿.md

🔴 关键矛盾：合并与删除顺序描述可能误导
第2.2节核心处理流程写“删除冗余lui → 合并lui+addi”。但实际实现中，若先删除冗余，可能错失合并机会（如lui t0,0x12345; lui t0,0x12345; addi t0,t0,0x678，删除第二个lui后暴露相邻对）。虽然固定点迭代会补偿，但建议明确迭代顺序，或改为“先合并，再删除冗余”以更直观匹配常见模式。

🟡 状态清空条件表述模糊
“对已跟踪寄存器产生定义的普通指令使该寄存器状态失效；这不妨碍规则A对相邻lui+addi进行整体匹配”——规则A仅检查相邻指令，不依赖状态，因此“不妨碍”是正确的，但措辞易误解为“失效不影响规则A（即规则A仍可匹配中间有clobber的序列）”。建议改为：“规则A（lui+addi合并）仅基于相邻指令，不依赖寄存器状态，因此状态失效不影响其匹配”。

🟡 内部设计数据结构未明确key规范化
“使用dict[str, int]跟踪各物理寄存器的最后一次lui值”未说明key是否已规范化。建议补充：“key为规范化后的寄存器名（如x5），确保ABI别名（t0）统一处理”。

💭 外部接口中merge_constants_detailed未说明是否已实现
文档标记为“可选新增”，但未提及当前提交是否已包含。建议在实现状态或步骤7中明确此接口的存在性，避免下游开发者混淆。

💭 步骤9的A/B表格中machine_instructions_before/after与code_size_before/after的N/A条件
文档已说明工具链不可用则标记N/A，但未注明若工具链可用但对象文件存在差异（如li被展开）时，机器指令数仍可能不变。建议在表注中补充：“对于li伪指令，反汇编后可能恢复为lui+addi，此时机器指令数不变，属于正常现象”。

💭 异常处理列表第6项-0x1可解析
当前RISC-V汇编器通常接受负立即数，但-0x1在GNU汇编中可能被解析为0xFFFFFFFF（32位），而RV32I的addi只接受12位立即数，实际需截断。建议明确：“-0x1应解析为12位有符号-1，即0xFFF，而不是32位补码”。

文档整体逻辑清晰，覆盖全面，以上为可优化点。

---

📁 docs/课题14-常量加载合并优化-技术设计文档初稿.md

这份设计文档整体质量较高，思路清晰、边界明确，尤其对“伪指令减少≠机器指令减少”的区分非常到位。以下按优先级列出问题。

---

🔴 文档内部矛盾：规则 A/B 的执行顺序不一致

• 5.1 架构图按“规则 A：lui+addi 合并 → 规则 B：块内冗余 lui 消除”排列；
• 6.5 与 0.1 明确说明“先删除冗余 lui，再合并 lui+addi”。

两种顺序会导致不同的最优结果（如 6.5 示例所示）。5.1 的图会误导读者，需按实际执行顺序修正为“规则 B → 规则 A”。

---

🔴 规则 B 缺少 CSR 指令分类

6.4 只写了“遇到会定义某寄存器的指令：清除该寄存器状态”，但未明确 CSR 指令（csrrw、csrrwi、csrrsi、csrrci 均写 rd）是否属于“会定义某寄存器”的类别。若解析器不认识 CSR 指令而走“未知指令→清空全部”，则行为保守正确；但若已知却未清除目标寄存器状态，会导致错误删除。

建议在 6.4 中明确：

• 所有带 rd 的 CSR 指令应清除 canonical(rd) 的状态；
• 若解析器无法可靠识别某指令写集合，一律清空全部状态（已有此条，但需与 CSR 分类衔接）。

---

🟡 lui 立即数接受范围表述模糊

6.1 说“接受有符号 20 位写法或 0..0xFFFFF 的编码写法，超出范围时拒绝优化”，但未给出具体数值边界。建议明确：

# 接受范围：[-2^19, 2^19-1] 或 [0, 0xFFFFF]
# 例如 -1 应被接受并规范化为 0xFFFFF

否则“有符号 20 位”可被理解为 [-2^20, 2^20-1]，将产生错误。

---

🟡 “函数调用”定义不明确

6.4 和 八、 中多次提到“跨调用”清空状态，但未列举哪些指令算作调用。建议明确至少包括：jal、jalr、call、tail。若解析器按操作码识别，需列出完整清单。

---

🟡 未讨论 lui x0, imm 的边界情况

lui x0, imm 是合法的无操作指令。若合并规则匹配到 lui x0, imm + addi x0, x0, 0，输出 li x0, imm 可能展开为 lui x0, imm（无操作）或 addi x0, x0, imm（无操作），语义上安全但无意义。建议至少注明“若目标寄存器为 x0，放弃优化或保留原样”，避免流水线中引入无关变更。

---

🟡 10.4 中 li 展开描述不精确

原文：

允许出现 asm_instructions_after < asm_instructions_before 且 machine_instructions_after == machine_instructions_before

在 RV32 下，li ra, imm 的展开结果取决于 imm 的位宽：

• 若 imm 可 12 位有符号编码，可能展开为 addi ra, x0, imm（单条）；
• 否则展开为 lui + addi（两条）。

因此 machine_instructions_after 可能小于 machine_instructions_before（当 lui+addi 被合并为单条 addi 时）。建议改为：

允许出现 machine_instructions_after <= machine_instructions_before，且 lui+addi -> li 可能重新展开为两条真实指令，因此不保证机器指令减少。

---

🟡 未提及自修改代码（self-modifying code）边界

若输入汇编包含运行时修改 .text 段的代码（罕见但在嵌入式场景存在），任何汇编级重写都可能改变程序行为。文档已声明“保守安全”，但建议在 八、正确性与安全约束 中加一条免责声明：

本优化不考虑自修改代码；若程序在运行时修改指令流，优化结果不保证语义等价。

---

💭 测试用例 4 可补充反向变体

用例 4 验证了“别名写目标寄存器时不删除”，很好。建议补充反向变体：

lui  t0, 0x10000
addi x5, x6, 1      # 写 t0 但通过 x6 读，不破坏 t0 状态
lui  t0, 0x10000

期望：第二条 lui 可删除（x6 与 t0 不是同一寄存器）。该用例可验证“只写不读”时的状态保留逻辑。

---

💭 8.2 中“未处理短路”可补充

6.3 中提到“中间最多只有空行或纯注释，没有标签或其他有效指令”，但若中间有 .section、.align 等汇编伪操作，规则 A 是否合并？当前文字“其他有效指令”可能包含伪操作，建议明确“伪操作视为边界，不跨过”。

---

总结：文档在语义模型、性能口径、测试设计上非常扎实，主要问题集中在规则 B 的边界条件明确性和文档内部一致性。建议按优先级处理上述 4 个 🔴/🟡 项后，再进入实现阶段。

---

📁 scratchv/backend/_asm_parser.py

🔴 无阻塞问题 — 代码逻辑正确，映射表与 RISC-V psABI 一致，正则匹配 x0-x31 无误。

🟡 缺少测试 — canonical_reg 是供数据流分析使用的关键函数，但未添加任何单元测试。建议覆盖：别名映射（'t0'→'x5'、'fp'→'x8'、'zero'→'x0'）、边界值（'x0'、'x31'）以及未知操作数（如 'abc'）返回小写的预期行为。

💭 正则边界 — x([0-9]|[12][0-9]|3[01]) 不匹配前导零（如 x01），但标准 RISC-V 寄存器名无前导零，可接受。若将来需要容忍非标准写法，可放宽为 x0*([0-9]|[12][0-9]|3[01])，但当前无此需求。

💭 函数命名 — canonical_reg 可能过于宽泛，暗示也适用于浮点寄存器，但实际仅处理整数。建议命名 canonical_int_reg 或明确 docstring 说明作用范围，避免后续误用。

---

📁 scratchv/backend/const_merge.py

🔴 Bug: 负立即数合并错误 — _parse_addi_imm 返回有符号值，但 _sign_extend_12 期望无符号值。例如 addi x1, x1, -1 时 _parse_addi_imm("-1") 返回 -1，_sign_extend_12(-1) 返回 -1，导致 (imm_hi << 12) + (-1) 计算错误。
建议: _parse_addi_imm 返回 value & 0xFFF，或调用处显式掩码。

🟡 潜在崩溃: 空行解析 — 兼容包装器 _parse_asm 对单行调用 parse_asm(raw)[0]，但若 parse_asm 对空行返回空列表会触发 IndexError。原代码能为空行创建 AsmInst。
建议: 改为 parsed = parse_asm(raw); line = parsed[0] if parsed else ParsedAsmLine(raw="", ...) 或直接复用 _asm_parser 的解析逻辑。

🟡 性能: 迭代次数上限过大 — max_iterations 默认 max(1, len(lines))，但通常一两次即可收敛。虽然会提前终止，但大文件可能浪费少量循环。
建议: 固定上限为 10 或根据变更次数动态限制。

💭 语义变化: 八进制立即数支持 — _parse_imm 现在接受 0o 前缀，原代码不支持。可能引入意外行为，但风险低。
建议: 确认是否需要；若需严格符合汇编语法，可限制为十进制和十六进制。

💭 注释行行号保留 — 移除冗余 lui 时生成的注释行保留了原始 lui 的行号，可能误导调试。
建议: 考虑使用 -1 或 lineno 取为 line.lineno 但标记为注释行，当前行为可接受。

---

📁 scratchv/compiler.py

🟡 潜在运行时错误：缺失属性检查 — 如果 merge_constants_detailed 返回的 stats 对象缺少 merged_pairs 或 redundant_lui_removed 属性，将引发 AttributeError。建议：

• 确认函数签名和返回类型始终一致（例如使用 typing.NamedTuple 或 dataclass）。
• 或添加防御性检查：getattr(stats, 'merged_pairs', 0) 并在更新日志时回退到简单版本。

💭 可读性 — 长字符串用 f-string 跨行拼接，可考虑用 f"""...""" 或提取为变量，避免行内换行。

---

📁 tests/test_backend.py

🔴 测试依赖私有方法 — _run_asm_passes 以下划线开头，应避免直接测试内部实现。建议通过公共接口（如 CompilerDriver.compile）进行验证。

🟡 断言字符串过于脆弱 — 警告消息 "Const merge: 1 changes" 和 "1 pairs" 完全硬编码，若格式或措辞调整则测试失效。建议使用 assert "Const merge" in warnings[0] 等部分匹配。

🟡 缺少分支覆盖 — 仅测试 const_merge=True 的路径，未覆盖 const_merge=False 或其他配置变化，应补充。

🟡 测试名称冗长 — test_const_merge_config_runs_post_codegen_pass 可简化为 test_const_merge_enabled，保持清晰。

💭 导入顺序 — 新导入 CompilerConfig, CompilerDriver 应放在 from scratchv.backend.asm_emit import AsmEmitter 之后（按字母顺序），但当前位置在 DSLParser 之后，虽不影响功能，可调整以保持一致性。

---

📁 tests/test_const_merge.py

🔴 Bug: 错误的目标寄存器 — test_comment_and_blank_between_pair_are_preserved 第 1 行：addi x5, t0, 2 的目标寄存器是 x5（即 t0 的别名），合并后应为 li x5, 4098，但断言 "li t0, 4098"。要么测试有误，要么实现错误地保留了 lui 的目标寄存器。

💭 关注点分离 — test_register_canonicalization 测试的是 _asm_parser 模块的 canonical_reg 函数，建议移入 tests/test_asm_parser.py（若存在），避免跨模块测试。

💭 空格敏感断言 — test_label_on_lui_is_preserved_when_pair_merges 断言 "L0: li t0, 4098"（两个空格），实际输出可能为 "L0: li t0, 4098"（一个空格）。建议使用 in 或 substring 匹配，避免因空白格式变动而误报。

🟡 测试名称可读性 — test_alias_clobber_keeps_later_lui 中 add x5, a0, a1 实际上 clobber t0（因为 x5 是 t0 的别名）。建议添加注释或修改名称，明确说明 t0 与 x5 是同一寄存器。

---