Skip to content

feat: topic17 v1.3 regression allocator optimization + scenario analysis + docs - #37

Open
Cynthia-Xichen wants to merge 3 commits into
ScratchV-Compiler:mainfrom
Cynthia-Xichen:feat/topic17-v1.3-clean
Open

feat: topic17 v1.3 regression allocator optimization + scenario analysis + docs#37
Cynthia-Xichen wants to merge 3 commits into
ScratchV-Compiler:mainfrom
Cynthia-Xichen:feat/topic17-v1.3-clean

Conversation

@Cynthia-Xichen

Copy link
Copy Markdown

Summary

topic17 v1.3 回归分配器优化 + 瓶颈场景分析 + 文档

Changes

  • regalloc_linear_v1.3.py: 回归分配器 v1.3 版本,优化了 eviction/scratch/pressure 策略
  • topic17_bottleneck_scenarios_v1.3.py: 瓶颈场景分析框架 v1.3
  • 开发文档: topic17 v1.3 开发过程记录
  • 设计文档: topic17 v1.3 设计方案说明

Test Plan

  • 单元测试通过
  • 回归分析覆盖

Closes #

@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown

🤖 AI Code Review

共审查 6 个变更文件

📁 docs/topic17_v1.3开发文档.md

🔴 设计假设未明确 — 2.2 未声明基本块内单一定义假设。7.6 虽提及 multi-def 不在本次范围,但设计文档主体应明确列出,避免读者误解算法适用范围。

🟡 API 示例混淆版本 — 2.1 示例导入 regalloc_linear(基线版),但 v1.3.1 改进版在 regalloc_linear_v1_3。用户可能误用旧版,建议在示例中直接使用 v1_3 的 import 或添加注释说明对应版本。

🟡 spill code 生成描述不完整 — 2.2 步骤 5 只说“插入 sw/lw”,未说明临时寄存器(scratch)的选择策略。自溢和 reload 的 scratch 选取是正确性关键,建议补充或引用 7.3/7.5 的修复。

💭 物理寄存器池定义缺失 — 文档未列出哪些寄存器属于池(如是否排除 x0、x1、x2?)。3.3 检查项提到保留 x0/ra,但未在池定义中明确,建议在 2.2 或附录中给出寄存器列表。

💭 日期笔误 — 文档中日期为 2026 年(如 2026-07-13),疑似未来年份,若为真实项目请修正。


📁 docs/topic17_v1.3设计文档.md

🔴 代码示例错误:setdefault 应为 setdefault
第 178 行 all_uses.setdefault(d, set()).add(i) 中 Python dict 的正确方法是 setdefault(缺少字母 f)。此错误若直接复制到代码中会导致 AttributeError,建议全局修正。

🔴 算法描述与 v1.3.1 修复矛盾
2.2 节仍保留自我溢出(self-spill)分支(spill() 返回 None 时标记 SPILL_),但 5.5 节 Fix 2 说明已改为“统一 evict 结束最晚的活跃区间”,不再有自我溢出。请更新 2.2 节算法流程和伪代码以匹配 v1.3.1 的最终实现,消除读者混淆。

🟡 拼写歧义:scartchv 应为 scratchv
3.1 节表格中 scartchv/backend/regalloc_linear.py 及类似位置(第 160 行、第 341 行等)出现 scartchv,应为项目名 scratchv。建议全文统一替换。

🟡 版本演进逻辑不清晰
v1.1 修复中已说“当前区间被自身溢出时改为返回 None,不分配寄存器”,但 v1.3.1 又修复“self-spill 写死 phys_regs[0]”。若 v1.3 重新引入了该问题,请在附录中明确说明原因,否则会使读者怀疑版本历史的一致性。

💭 性能优化建议
allocate()free_regs.pop(0) 时间复杂度 O(n),当寄存器池较大时可能影响性能。若顺序无关,可考虑 pop() 从末尾弹出(O(1))或使用 collections.deque。此建议不影响正确性,作为可选项。


📁 docs/topic17_v1.4开发文档.md

好的,以下是代码审查结果。

🔴 文档与实现不一致:compute_live_intervals 复杂度描述
文档说“单遍扫描 O(N+V)”,但实际算法(v1.4 代码)在 compute_live_intervals 中仍然有两层循环:外层遍历指令,内层对每个指令的 defines/uses 集合做成员检查。虽然名义上是“单遍”,但复杂度实为 O(N * avg_defs+uses),在密集指令(如多源 add)中可能退化。建议在文档中如实描述为 O(N + total_operands),避免误导。

🟡 Fix 7 的判据修改说明不充分
文档说“redefine 判据由 rename[d].startswith("SPILL_") 改为 d in self._spilled”,但未解释为什么 startswith 会漏检。实际上,rename[d] 在第一次重定义后会被改为物理寄存器,因此后续重定义时 startswith 返回 False,导致漏检。文档应加入此根因分析,帮助读者理解 bug 的深层原因。

🟡 设计边界“multi-def”可能在 v1.4 中被意外突破
文档声明“非本次 bug,真实管线不产生多定义输入”,但 Fix 7 处理了“同一 spilled vreg 多次重定义”。这本质上就是 multi-def,只是范围限定在 _spilled 集合中。如果未来真实管线因某种原因产生了多定义非 spilled vreg,当前代码仍然会漏写回。建议在文档中明确标注此边界,并指出当前修复仅覆盖 spilled vreg 的 multi-def。

💭 Flattened 压力测试场景描述可优化
文档中 A01/A02/A03 等场景的“非法多源 add”解释为“单指令超过全部 27 个物理寄存器的操作数”,但实际一个 RISC-V 指令最多 3 个操作数。应改为“场景构建器生成了操作数数量超过硬件限制的 add 指令(如 100 个源)”,并说明重构为累加链保留了压力剖面。

💭 “验证结果”部分缺少基准测试数据
文档提到“语义仿真器:A01-A04...全部 0 错误”,但未给出具体数值(如多少次仿真、多少条指令)。建议补充简要的仿真规模统计,如“每个场景执行 1000 拍,检查 5000 条指令的寄存器值”,使验证结果更可信。


📁 docs/topic17_v1.4设计文档.md

🔴 不一致:compute_live_intervals 冗余分支 — 文档 V1.1 声称已消除 define+use 同指令的冗余分支,但代码块第 107-109 行仍保留 if d in inst.uses: 分支。该分支与后续 for u in inst.uses 重复处理,导致 all_uses[d] 中同一指令号被重复添加(虽 set 去重,但逻辑冗余且表明代码与文档描述不符)。

🔴 缺失核心函数:spill() 无实现allocate 第 134 行调用 self.spill(interval, active, free_regs),但文档未给出 spill 方法定义。溢出策略、eviction、self-spill 分支均依赖该函数,缺少实现细节导致设计不可评估。

🔴 数据结构不一致:spill_code 未定义get_allocated_code 第 149 行依赖 self.spill_code,但文档未说明该属性的类型、初始化位置及填充方式。同样 self._spill_slotsself._spilled 均未定义,导致代码片段无法编译。

🟡 active 列表未保持排序 — 文档宣称“按 end 升序维护 active 列表”,但 allocate 中仅使用 active.append() 追加,未做插入排序或后续排序。溢出策略需遍历找最大 end,排序非必须,但描述与实现矛盾,易误导读者。

🟡 _pick_scratch_evict_for_reload 未展示 — 文档多次提及这两个函数的关键修复(v1.3.1 Fix 2、Fix 3),但未给出签名或实现逻辑,无法验证其正确性(如 _pick_scratch 如何避让 live_regs_evict_for_reload 如何降级而非删除)。

🟡 allocatespill 返回 None 分支未处理 — 第 139 行 if spill is not None 存在 else 分支标记为 SPILL_,但文档未说明 spill 在何种条件下返回 None(自身溢出)。该逻辑依赖 spill() 内部实现,但未给出。

💭 compute_live_intervalslast_use 更新逻辑last_use[u] = max(last_use.get(u, -1), i + 1)u 使用 i+1 作为 end,而 last_use 初始化为 -1,当 u 在指令 i 首次使用时,end 为 i+1,正确。但 for d in inst.definesif d in inst.uses 分支也更新 last_use[d],与 for u 重复,建议删除该分支。

💭 get_allocated_coderename 字典修改可能影响后续指令 — 第 166 行 rename[u] = scratch 直接修改 rename 字典,但 renamedict(self.alloc_map) 的浅拷贝,修改 rename 不影响 alloc_map,但会影响后续指令的映射。若同一 vreg 在后续指令中再次作为 use,仍会使用该 scratch 寄存器(可能被第二次 reload 覆盖),但文档未说明 _pick_scratch 是否保证同一 vreg 复用同一 scratch(v1.3 提到 _scratch_cache 但未展示实现)。


📁 scratchv/backend/regalloc_linear_v1_4.py

🔴 Bug: _pick_scratch may silently corrupt data — Line ~720: when all physical registers are busy and the cached scratch is also busy, the function falls back to self.phys_regs[0] without checking if it is occupied. This overwrites a live value. Should raise RuntimeError like _evict_for_reload.

🔴 Bug: machine_instrs_from_block misidentifies physical registers — Line ~770: _to_mop uses prefix matching (s.startswith("a") etc.) to decide if an operand is a physical register. A virtual register name like a_temp (after stripping %) would be incorrectly classified as a physical register. Use _REG_NUMS or a set of known physical regs instead.

🟡 Suggestion: compute_live_intervals ignores multiple definitions — If a vreg is defined more than once in the block, start is only set by the first definition. The interval then spans from first definition to last use, which may hide later redefinitions. Document that each vreg must be defined once, or handle redefinitions correctly.

🟡 Suggestion: _pick_scratch should fail safely when no register is free — The final fallback to phys_regs[0] (even when busy) is unsafe. Prefer raising RuntimeError to prevent silent data corruption, similar to _evict_for_reload.

🟡 Suggestion: get_allocated_code is too long — This method (~120 lines) handles reloads, evictions, scratches, and spill stores. Consider splitting into _emit_reloads, _emit_scratch_stores, and _emit_evictions for readability.

💭 Nit: farthest typo — In _evict_for_reload and spill, the variable name farthest is used; standard English uses furthest. Minor, but inconsistent.

💭 Nit: report displays negative offsets without context — Spill slot offsets are negative (e.g., sp+-4). It’s correct (stack grows down), but consider adding a note like (sp + offset) to clarify direction.


📁 scratchv/backend/topic17_bottleneck_scenarios_v1.4.py

🔴 Bug: _renumber 破坏同时创建语义 — 场景如 A01、A02、A03、B01、D01、E01、E03 通过重复 id(如所有 li 使用 id=0)表达“所有 vreg 在同一位置同时创建”的极端压力。但 _renumber 将每个指令重编为唯一顺序 id,使这些 vreg 不再同时活跃,完全改变了测试意图。建议: 要么放弃 _renumber,改用分配器本身支持重复 id(同一位置多个指令);要么让场景构建器使用正确的唯一 id,但通过将多个 li 放在同一个 pos 来保留同时性。

🟡 误报:vreg_leaks 检测使用子串匹配 — 第 98 行 if v in asm: 会匹配子串,例如 v0v0_2 中,导致大量误报。建议: 改用正则 \b 边界或拆分空白操作数列表来精确匹配 vreg 名称。

💭 redundant_sw 检测顺序不保证执行顺序_all_spill_lines 先遍历 spill_code_evictions,但两类溢出可能交错,使 sw_history 判定冗余不准确。建议: 按指令 id 合并顺序后再扫描,或至少文档说明局限性。

💭 spill_code_entries 指标不一致 — 前两项 len(alloc.spill_code) + len(alloc._evictions) 是位置数,第三项 sum(len(v) for v in alloc._reloads.values()) 是 reload 条目数,混合比较无意义。建议: 统一为条目数(如 sum(len(v) for v in alloc.spill_code.values()) + ...)或位置数。


Address verified bugs from PR ScratchV-Compiler#37 AI review:
- rename regalloc_linear_v1.3.py -> regalloc_linear_v1_3.py (importable)
- fix self-spill clobbering phys_regs[0] (evict farthest instead)
- fix _evict_for_reload leaking vregs (SPILL_ demotion, not del rename)
- remove redundant define+use branch in compute_live_intervals
- hoist machine_types import to module level
- rewrite scenario runner for v1.3 dict spill_code API + unique-inst ids

Verified: 23/23 scenarios pass (no leak, no reg conflict), 18 unit tests pass.
基于 PR ScratchV-Compiler#37 AI 代码审查再核对, 修复 v1.3.1 未覆盖的真实问题:

分配器 (regalloc_linear_v1_4.py):
- Fix 7: 重定义写回路径补全. spilled vreg 被纯重定义或同一 vreg
  多次重定义时, 新值未写回栈 (redefine 判据由 rename[d] 前缀改为
  d in self._spilled), 后续 reload 读到栈上旧值.
- Fix 8: _pick_scratch 增加 busy 参数, 避开同指令内 reload 寄存器冲突.
- Fix 9: _evict_for_reload 回退改为复用同指令 reload 寄存器, 不再
  静默覆盖存活寄存器.

场景 (topic17_bottleneck_scenarios_v1.4.py):
- Fix 10: A01/A02/A03/D01/E03 多源 add 非法指令重构为合法累加链.
- Fix 11: D04 螺旋交织 use-before-def 非法输入修复 (99 处), 消除
  SPILL_vXX 泄漏进汇编.

文档: 新建 topic17_v1.4开发文档/设计文档, 同步整理 v1.4 修复清单.

验证: 23 场景全通过 (redund D04=1/F04=644 与基线一致), 语义仿真
A-F 全 0 错误, pytest 342 passed (2 失败为 tinyfive 环境无关问题).
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