Skip to content

test: add PR #37 register allocation regressions - #49

Open
EthanZero2Hero wants to merge 1 commit into
ScratchV-Compiler:mainfrom
EthanZero2Hero:ci/pr37-regression
Open

test: add PR #37 register allocation regressions#49
EthanZero2Hero wants to merge 1 commit into
ScratchV-Compiler:mainfrom
EthanZero2Hero:ci/pr37-regression

Conversation

@EthanZero2Hero

Copy link
Copy Markdown

Summary

Validation

The regression cases were validated against the fetched PR #37 v1.5 implementation. CI will run:

python3.12 -m pytest tests/test_pr37_regression.py -v --tb=short

@github-actions

Copy link
Copy Markdown

🤖 AI Code Review

共审查 2 个变更文件

📁 .github/workflows/ci.yml

🔴 Hardcoded python version — Line 48: python3.12 硬编码,与工作流其他步骤可能不一致(例如 matrix 或系统默认版本)。建议使用 python 或从环境变量/矩阵中获取版本。

🟡 Missing test file validation — 未确认 tests/test_pr37_regression.py 是否存在,若文件缺失则 CI 会失败。建议添加 if: steps.check-test-file.outputs.exists == 'true' 或提前检查文件。

🟡 Step name not reusable — 名称包含 “PR #37”,但此步骤将永久保留在 CI 中,不适用于未来其他 PR。建议改为 “Run register-allocation regression tests”。

💭 No shared pytest configuration — 直接调用 pytest 可能跳过项目级别的 pyproject.tomlpytest.ini 配置(如插件、标记、超时)。建议省略参数,让测试继承项目默认配置。

💭 Step placement — 新步骤位于 Generate test visualization page 之前,但后者仅限 main 分支,而新步骤无条件运行。若本意在 main 上才运行,应添加 if: github.ref == 'refs/heads/main' 或保留为全局(当前逻辑一致)。


📁 tests/test_pr37_regression.py

🟡 Suggestion: Use explicit assertions instead of relying on StopIteration — In test_spilled_vreg_redefinition_is_written_back_before_reload, if redefine or writeback are not found, next() raises StopIteration with a cryptic error message. Consider using pytest.fail() with a descriptive message or assert with a condition.

🟡 Suggestion: Tighten regex to avoid potential false positives — The regex (?<![A-Za-z0-9_])v[0-9]+(?![A-Za-z0-9_]) is used to check for leaked vregs. It correctly excludes comments, but if a physical register were named like v0 (unlikely), it would be flagged. Consider using a more specific pattern like \bv[0-9]+\b if the assembler supports word boundaries.

💭 Nit: Test relies on hardcoded comment strings"store redefined v0" and "reload v0" are implementation details. If the allocator's comment format changes (e.g., "store v0 (redefined)"), the test will break. Consider matching on "store redefined v0" as a substring or using a more robust heuristic.

💭 Nit: Partial duplication of test blocks_pressure_block() and the block in test_spilled_vreg_redefinition... share similar structure. Consider extracting a helper to reduce duplication, but this is minor.


Cynthia-Xichen pushed a commit to Cynthia-Xichen/ScratchV that referenced this pull request Aug 16, 2026
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.
Cynthia-Xichen pushed a commit to Cynthia-Xichen/ScratchV that referenced this pull request Aug 16, 2026
Run tests/test_pr37_regression.py in CI, from PR ScratchV-Compiler#49 (ci/pr37-regression).
jizhenjun pushed a commit that referenced this pull request Aug 22, 2026
…sis + docs (#37)

* feat: topic17 v1.3 regression allocator + scenario analysis + docs

* fix(regalloc): v1.3.1 code-review fixes (PR #37)

Address verified bugs from PR #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.

* topic17: v1.4 register allocator fixes + scenarios + docs

基于 PR #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 环境无关问题).

* topic17(v1.5): 版本升级 1.4→1.5,落地 7/31 AI 审查复核 + 模块/文档重命名为 v1_5

- 重命名: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

* test: add PR #37 register allocation regression tests

From PR #49 (ci/pr37-regression), add tests/test_pr37_regression.py
covering spill/reload and machine-operand round-trip for the PR #37
register allocator.

All 3 tests pass with the project venv.

* ci: add PR #37 register-allocation regression step

Run tests/test_pr37_regression.py in CI, from PR #49 (ci/pr37-regression).

---------

Co-authored-by: Xi <xi@scratchv.dev>
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