Skip to content

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

Open
Cynthia-Xichen wants to merge 6 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 6 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

共审查 8 个变更文件

📁 .github/workflows/ci.yml

🟡 非通用命名 — 步骤名和测试文件均包含 PR #37 编号,维护时易混淆。建议改为 Register-allocation regression teststests/test_register_allocation_regression.py

🟡 硬编码 Python 版本 — 使用 python3.12 而非 CI 变量,若其他步骤采用矩阵或变量,将导致版本不一致。建议引用 ${{ matrix.python-version }}python

💭 缺少条件运行 — 此步骤会每次 CI 都执行,而回归测试通常只在特定变更时触发。考虑添加 if: github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'regression') 等条件。

💭 考虑合并到主测试 — 若回归测试内容不多,可考虑直接加入 pytest 主命令,避免额外步骤开销。


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

🔴 示例代码与新版分配器不一致 — 2.1节示例代码 from scratchv.backend.regalloc_linear import LinearScanAllocator 导入的是基线版本,但文档后半部分(v1.3.1)推荐使用 regalloc_linear_v1_3。用户可能混淆。
🟡 术语不统一 — 7.5 Bug A 使用 self.alloc_map,而 7.6 Fix 3 使用 rename 字典。建议统一命名或说明两者关系(如 renamealloc_map 的别名),避免歧义。

🟡 report() 输出示例缺失 — 2.1节提到 print(allocator.report()),但未给出输出格式。建议在附录中贴出一段示例输出,便于用户理解 peak_activepeak_real_pressure 等新字段的含义。

💭 基线版本状态未明确 — 3.1文件清单只列出新增文件,未说明 regalloc_linear.py 是否保留。建议注明基线版仍可用但已冻结,新版为 regalloc_linear_v1_3.py

💭 Fix 2 描述措辞易误解 — 7.6 表格中“写入其值会污染仍在存活的 vreg”,实际是读取被污染寄存器的值作为 sw 源,导致栈中存入错误数据。建议改为“使用被占用的 phys_regs[0] 作为 sw 源寄存器,会读取错误值并写入栈中”。

🟡 3.3 异常处理 checklist 未完成 — 所有 [ ] 未勾选,但文档本身是设计文档,这种状态合理,不过建议在发布前全部复核或标记为“待定”,避免误认为未完成。

💭 7.7 优化方向缺少优先级/时间预期 — 列出 5 个方向但未标注优先级或预计工作量,建议补充(如 P0/P1/P2),便于后续排期。

总体:文档内容详实,修复记录清晰,但示例代码和术语一致性需要修正,以免误导用户。


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

🔴 正文与最新实现不一致 — 2.2 节算法流程仍保留 self-spill 分支(spill() 返回 None 时标记 SPILL_),但 v1.3.1 已废除 self-spill,改为统一 evict 最晚活跃区间。2.3 节 Spill Code 生成中也保留“自身溢出”描述,与当前实现矛盾。
建议:将正文(2.2、2.3、3.2 Step 3/4)更新为 v1.3.1 最终逻辑,附录只保留历史演进。

🟡 “self-spill”术语混淆 — 5.5 节 Fix 2 说明不再 self-spill,但 Fix 3 仍使用 SPILL_ 标记(被 evict 的 victim),且 2.3 节继续使用“自身溢出”概念。读者易误以为当前区间仍可能被标记为 SPILL_
建议:统一术语,明确所有溢出均通过 evict 实现,SPILL_ 仅表示被 evict 的 victim 的降级标记。

🟡 _evict_for_reload 方法未在正文中定义 — 5.5 节 Fix 3 突然引用该方法,但正文 2.2/2.3/3.2 均未介绍其行为。
建议:在 2.3 或 3.2 中补充 _evict_for_reload_pick_reload_reg 的说明,确保文档自包含。

💭 代码片段与最终实现不一致 — 3.2 Step 3 核心分配循环代码仍保留 self.spill(interval, active, free_regs) 返回 None 的分支,与 v1.3.1 修复后 spill() 不再返回 None 矛盾。
建议:更新代码片段,移除 self-spill 分支,体现 evict 唯一路径。

💭 版本号混乱 — 文档标题“v1.3 设计文档”,但内容涉及 v1.3.1,且文件名 topic17_v1.3设计文档.md 未更新。
建议:统一为 v1.3.1 版本,文件名同步更新。

🟡 活区间计算示例中 v6 的 end 与 uses 矛盾 — 5.1 节示例中 v6 的 uses 为 {},但 end 为 3(指令 2 之后)。按定义,无使用的区间长度应为 start+1,此处却为 3,与 2.1 节定义不符。
建议:修正 uses 为 {2} 或 end 改为 3(指令 2 定义了 v6,但无使用则 end=3?实际上指令 2 定义 v6 后无后续使用,区间应为 [2,3) 半开区间,end=3 合理,但 uses 应为空集?需明确:定义点本身也算使用吗?通常定义点不算使用。若 uses 为空,则 end 应为 2+1=3,正确。但示例中 uses 写为 {},可能引起误解。建议注释说明。

💭 5.5 节 Fix 2 场景中变量名多下划线v1_new 应为 v_new


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

🔴 API 示例与实际模块不一致 — 2.1 节示例使用 scratchv.backend.regalloc_linear,但文档说明实际模块为 regalloc_linear_v1_5。示例直接复制将无法运行,应更新为匹配实际模块名。

🔴 版本历史文件命名前后矛盾 — 7.6 节(v1.3.1)称“文件已重命名为 regalloc_linear_v1_5.py”,但 v1.3.1 阶段重命名目标应为 v1_3,此处写为 v1_5 是笔误;7.7 节 v1.4 重命名描述与其冲突,需统一修正。

🟡 核心算法流程缺少 spill code 插入位置说明 — 2.2 节算法步骤 5 仅说“为被溢出 vreg 的所有定义后插入 sw、所有使用前插入 lw”,但未明确 sw 插入在定义指令之后、lw 插入在使用指令之前,建议补充示例或明确位置规则。

🟡 风险缓解措施“栈槽分配冲突”描述不完整 — 当前方案“每个 vreg 独占一个槽位”是对的,但未提及未来 Opt 2(栈槽复用)可能改进,易让读者误以为这是最终设计,建议补充说明“当前为独占分配,栈槽复用是后续优化方向”。

💭 拼写错误 — 多处将 redundant 误写为 redundant(如 7.8 节 redundant_sw、验证结果 “redund: D04=1”),正确拼写为 redundant,建议全文统一修正。

💭 时间线逻辑矛盾 — 6. 进度跟踪中“编码实现”计划 2026-07-20,但“代码优化”完成于 2026-07-16,早于编码开始日期,时间顺序不合理,应核实。

💭 文档结构过于冗长 — 7.5~7.8 节大量历史版本修复细节(约 150 行)挤占核心设计阅读空间,建议将版本演进记录移至附录或单独文档,主文档保留最新设计说明及关键修复要点即可。


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

🔴 **矛盾:v1.1 优化记录与当前实现冲突**  
§5.2 第3条声称“`compute_live_intervals` 复杂度从 O(V*N) 改为了 O(N+V) 单遍扫描”,但 §2.2 Step 2 的代码和复杂度说明明确当前实现是 O(V·N)。要么 v1.1 优化被后续版本回退且未记录,要么文档矛盾。建议补充版本演化说明,避免读者困惑。

🟡 **逻辑不一致:溢出策略描述重复**  
§2.2 算法流程后已详细解释“选择 end 最大的区间溢出”,但 Step 3 的代码注释后又完整重复一遍相同理由。建议删除冗余,或仅保留一次。

🟡 **代码示例与注释不匹配**  
§2.2 Step 2 中 `compute_live_intervals` 代码的注释写“独立的 define-and-use 分支是冗余的(已删除)”,但实际代码仍保留 `if vreg in inst.defines` 和 `if vreg in inst.uses` 两个分支,只是去掉了合并写法。注释“已删除”会误导读者以为该分支不存在。

💭 **数据结构类型标注不标准**  
§2.2 Step 4 中 `_reloads` 类型写为 `list[(vreg, slot)]`,Python 实际应为 `list[tuple[str, int]]`。建议修正,避免与类型检查工具冲突。

💭 **“代码审查记录”不应塞入设计文档**  
§5.6.1 第四点“`setdefault` 审查条为误报”与文档技术内容无关,属于代码审查对话记录,建议移至开发日志或移除。

🟡 **未解释关键函数作用**  
文档大量引用 `_pick_scratch`、`_pick_reload_reg`、`_evict_for_reload` 等内部函数,但 §2.2 核心流程中并未定义它们的语义和调用时机。读者需在修正说明中拼凑,建议在 Step 4 中简要说明各函数职责。

💭 **版本号混乱,v1.5 变更不明确**  
标题为 v1.5,§5.6.1 列出 v1.5 的 AI 审查复核结果,但 §2.2 的修正说明(如复杂度、自溢分支)标注为 v1.4 修正。读者无法区分哪些是 v1.5 新增、哪些是 v1.4 原有。建议在 §2.2 各小节用脚注或标记明确版本归属。

📁 scratchv/backend/regalloc_linear_v1_5.py

🔴 Bug: multiple reloads to the same register corrupts data_pick_reload_reg reuses reuse_reg even when that register is still live for a previous reload. If an instruction needs two reloaded vregs, the second lw overwrites the first, so the instruction sees only the last one. Fix: each reload must get a unique register (or guarantee the reused register is not simultaneously used as a source).

🟡 Unsafe fallback in _pick_scratch — When all registers are busy, it falls back to phys_regs[0] which may be live, corrupting a value. This is documented as “pressure dump only”, but for executable code it’s a correctness hole. Should raise or guarantee a free register via eviction.

🟡 _pick_reload_reg logic may return an occupied register — The used set is built from rename but reuse_reg is returned without checking if it’s in used. Even if it’s live, the comment assumes it’s safe, but the value may still be needed by the instruction. This is the same root cause as the first bug.

🔴 Missing handling when spill returns None — In allocate line if spill is not None: … else: … the else branch does nothing, leaving the current interval unallocated. Although spill never returns None in practice (active non‑empty when free_regs empty), the code is fragile and will silently produce unresolved vreg names if it ever does.

🟡 compute_live_intervals does not handle redefinitions of the same vreg — If a vreg is defined multiple times, start stays at the first definition, and end includes all uses, giving a wrong interval for the later definitions. The docstring should state the assumption (single definition per block) or handle it correctly.

💭 Active list not sorted by end_expire_old_intervals and spill both scan all active intervals, which is O(n²) per allocation. For large register files this is a minor performance issue, but sorting by end would enable O(1) expire and faster spill selection.


📁 scratchv/backend/topic17_bottleneck_scenarios_v1_5.py

🔴 Bugs in scenario construction – duplicate id + _renumber alters intended pressure profile
Multiple builders (e.g. A01, B01, C01, E01) assign the same id (e.g. 0) to many li instructions to express “simultaneous creation at one program point”. _renumber later spreads these across sequential positions, turning the intended “all live at once” into a staggered start. This fundamentally changes the pressure test and may produce misleading metrics.
Suggestion: If the allocator accepts a batch of instructions with the same id as “one program point”, avoid _renumber and fix the allocator; otherwise rebuild the scenarios with a legal multi‑operand pseudo‑instruction (e.g. a dedicated BUNDLE type) that guarantees simultaneous liveness.

🔴 Uses set deduplication hides repeated use in same instruction
In C04, add v_out, v{POOL}, v{POOL} puts v{POOL} only once in uses. The allocator will emit a single reload, but the real assembly would need two source operands – the spilled vreg would be loaded only once, leading to a wrong result.
Suggestion: Either model the repeated use with two distinct entries in uses (e.g. uses={v{POOL}, v{POOL} won’t work because it’s a set) or restructure the test to use two different vregs that happen to hold the same value.

🟡 Reliance on private allocator attributes
run_scenario accesses alloc._reloads, alloc._evictions, alloc._spill_slots, alloc.spill_code. These are implementation details of LinearScanAllocator and may change without notice.
Suggestion: Expose the needed counters via public methods (e.g. get_store_lines(), get_reload_map()) or document the dependency tightly.

🟡 _all_spill_lines merges stores in wrong order
It first appends all spill_code entries for a given position, then all eviction entries. The comment says the two classes “may interleave”, but the implementation always groups them. This can cause false neg/pos in redundant‑store detection (e.g. an eviction store followed by a self‑spill at the same slot would not be seen as consecutive).
Suggestion: Merge the two lists sorted by their original line index (if available) or by the order they appear in the emitted assembly.

🟡 B02_long_chain redefines v0 200 times – allocator may not handle redefinition well
Each addi defines v0 again, creating 200 overlapping intervals for the same virtual register. The allocator’s compute_live_intervals may treat each definition as a new interval, but the test expects a single long‑lived vreg. This could cause excessive spill/reload not representative of a real chain.
Suggestion: Use a fresh vreg per step (e.g. v0, v1, …) and chain them to get a true long‑lived anti‑dependency chain.

💭 print_result may truncate flag list
flags_s = ','.join(f[:3]) limits to 3 flags, discarding others. The summary table could miss important warnings.
Suggestion: Print all flags; if the line is too long, wrap or use a shorter abbreviation.

💭 F04_large_random_block uses closure over rng – fine but could be made explicit
rng is defined outside build, but inside build it’s used as a closure variable. This works, but moving rng inside build with a local random.Random(12345) makes the seeding clearer.

💭 Minor: addi instruction format in comments
Some comments show addi with 3‑operand syntax (e.g. addi v0, v0, 1), but the actual code uses addi with a list of operands. No functional issue, but the comments could be misleading to readers unfamiliar with the internal representation.


📁 tests/test_pr37_regression.py

🟡 Fragile assertion relies on exact comment strings — Lines 72–73, 76:
"store redefined v0" and "reload v0" appear in comments. Renaming or removing those comments (e.g., moving to a debug mode) breaks the test silently.
Suggestion: Assert on instruction types or operand patterns instead of hardcoded diagnostic strings.

🟡 Imprecise match for redefinition line — Line 72:
if ", 9" in line could match other instructions (e.g., add v1, v0, 9). Currently unique, but fragile.
Suggestion: Check the opcode and defined register, e.g., if line.startswith("li") and "v0" in line.split("#")[0].

💭 Duplicate allocator creation — Lines 38, 67, 97:
LinearScanAllocator(phys_regs=["t0", "t1"]) repeated. Minor, but could be extracted into a @pytest.fixture to reduce boilerplate.


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.
Xi added 2 commits August 6, 2026 06:10
基于 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 环境无关问题).
- 重命名:regalloc_linear_v1_4.py→v1_5.py,topic17_bottleneck_scenarios_v1.4.py→v1_5.py(.→_ 规范化),文档 topic17_v1.4*.md→v1.5*.md(git mv 保留历史)
- 代码修正(7/31 AI 审查复核):
  * _to_mop 以 _REG_NUMS 精确成员匹配取代前缀误判(a_temp 不再被当作物理寄存器)
  * _all_spill_lines 按位置合并两类 sw,保证 redundant_sw 执行序
  * vreg_leaks 改用词边界正则,消除子串误报
  * spill_code_entries 统一为条目数(消除位置数/条目数混用)
  * report() 补充负偏移说明
  * _pick_scratch 全忙回退补强注释(可燃/合法输入由 _evict_for_reload 保证有闲寄存器,保留回退供压力 dump 场景)
- 文档订正:compute_live_intervals 复杂度 O(V·N)(非 O(N+V))、Step3 自溢分支、Step4 spill_code dict 结构/_pick_scratch 签名;新增 7.8(v1.5)/5.6.1 复核章节
- 验证:23 场景 0 VLEAK/0 ERROR;18 单测 passed;import & py_compile OK
Xi added 2 commits August 16, 2026 09:10
From PR ScratchV-Compiler#49 (ci/pr37-regression), add tests/test_pr37_regression.py
covering spill/reload and machine-operand round-trip for the PR ScratchV-Compiler#37
register allocator.

All 3 tests pass with the project venv.
Run tests/test_pr37_regression.py in CI, from PR ScratchV-Compiler#49 (ci/pr37-regression).
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