Skip to content

feat(regalloc): support pseudo instructions and accurate spills - #56

Open
yuki-328 wants to merge 6 commits into
ScratchV-Compiler:mainfrom
yuki-328:topic17-pseudo-regalloc
Open

yuki-328 wants to merge 6 commits into
ScratchV-Compiler:mainfrom
yuki-328:topic17-pseudo-regalloc

Conversation

@yuki-328

@yuki-328 yuki-328 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a central machine-instruction semantics table for defs, uses, immediates, control flow, calls, and pseudo-instruction metadata
  • make both linear-scan variants CFG-aware and share an executable spill/reload rewriter
  • fix greedy eviction/reload handling and caller-saved clobbers
  • lower and validate integer pseudos (mv, li, max, bnez, j, local call, labels)
  • report actual spill stores, reload loads, live pressure, and excess pressure in Topic17 benchmarks
  • add P1 implementation and AI self-review reports

Correctness fixes

  • keep distinct spilled sources in distinct physical registers
  • allow a destination to reuse a source only after preserving a still-live old value
  • canonicalize edge-live values across high-pressure CFG joins
  • keep global physical assignments stable across predecessor blocks
  • reject pseudo expansion when it would silently clobber a busy scratch register
  • avoid collisions between generated max labels and user labels
  • fix TinyFive word loads on current NumPy so execution validation reads all four bytes

Validation

  • full suite: 555 passed
  • randomized execution differential: 12 seeds x 2 linear-scan implementations
  • TinyFive checks for pseudo equivalence, RV32 li boundaries, max aliasing, branches, CFG paths, and spill reloads
  • arbitrary virtual-register names are checked for post-allocation leakage
  • CNN benchmark: pressure peak 11 with 19 registers, 0 spill slots/stores/reloads
  • Dense pressure benchmark: pressure peak 29 with 5 registers, 28 slots, 63 stores, 75 reloads

Current boundaries

  • executable proof covers the integer RV32IM pseudo path; floating-point pseudos currently have allocation metadata only
  • max accepts a register RHS or immediate zero
  • call supports local JAL-range targets; external/far relocation is not implemented
  • fixed allocatable physical-register interference for arbitrary hand-written MachineInstr input remains future work

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

🤖 AI Code Review

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

📁 .gitignore

🔴 Bug: 通配符不匹配隐藏文件和子目录benchmark_reports/* 只匹配当前层级的非隐藏项。若有 benchmark_reports/.foobenchmark_reports/sub/file,将不会被忽略。
Suggestion: 使用 benchmark_reports/**(并保留原目录不被排除,以便 negation 生效)。

🟡 设计问题: 用 negation 反白名单单个 review 文档 — 把 topic17_fix_review.md 单独提交进仓库,会让"生成的报告目录"和"要跟踪的文档"混在一起。下次有人加 topic18_fix_review.md 又得改一次 .gitignore,而且容易忘记更新。
Suggestion: 把要跟踪的 review 文档移到专门的目录(例如 docs/reviews/benchmark_docs/),在 .gitignore 里排除整个 benchmark_reports/,保持目录语义清晰。

🟡 可读性/可维护性 — 这条规则对未来的读者不够直观。建议在注释中说明"为什么这个文件例外",否则三个月后没人记得 topic17 是什么、为什么单独保留。
Suggestion: 加一行注释,例如 # keep the review notes; reports themselves are regenerated

💭 Nitsbenchmark_reports/*!benchmark_reports/topic17_fix_review.md 之间的空行不必要;两条应相邻以便阅读。


📁 benchmark_reports/topic17_fix_review.md

Review: benchmark_reports/topic17_fix_review.md

🟡 Internal Inconsistencies

1. Benchmark target vs. algorithm mismatch (Section 5.1 vs 5.2)

  • 5.1 states benchmarks switched to production regalloc_linear.
  • 5.2 CNN row claims "linear/greedy 均 PASS".
  • These contradict — either the benchmark tests both allocators or only linear. If greedy is also tested, the command/config should reflect that.

2. CNN zero-spill anomaly unexplained (Section 5.2)

  • 30 vregs, pressure peak 11, yet 0 spill stores / 0 spill slots.
  • With RV32's ~14 usable GPRs and a peak of 11 live values, spilling should be expected unless the liveness graph is very structured (e.g., mostly sequential use). No justification given.
  • Suggest adding a one-line explanation (e.g., "values have short lifetimes within each layer") or a debug trace reference.

3. Baseline definition ambiguity (Section 2 header vs. table)

  • Header: "记录基线:5de34a1 及其之前已经进入项目的 Topic17 提交"
  • Table includes commits 0394e0a5de34a1 as if they are the baseline and the history under review.
  • Clarify whether 5de34a1 is the starting point (exclusive) or included in scope.

🟡 Missing or Weak Claims

4. "防止假通过" — no reference interpreter identity (Section 5.1)

  • "先由独立解释器计算参考 a0" — which interpreter? Is it reusing an existing one in the codebase, or a new oracle?
  • A reader can't audit the correctness of the "independent" check without knowing what it is.

5. regalloc_rewrite.py unexplained (Section 6)

  • Listed in the backend file inventory but never referenced in any section.
  • Either it's dead code (should note that), or it plays a role that isn't described.

6. Pseudo-instruction semantics integration unclear (Section 3.1 vs 3.2)

  • Section 3.1 says the semantics table covers every MachineOp.
  • Section 3.2 shows assembler-level pseudo-instructions (nop, ret) with no mention of whether they have entries in machine_semantics.py.
  • If assembler-level ops bypass the semantics table, that's a gap worth noting — or clarify they're folded before reaching the table.

7. Previous benchmark results not invalidated (Section 5.1)

  • "simple/dense 不再使用不可编码的 r0rN 假寄存器" implies prior results were based on invalid register names.
  • The document should explicitly state prior benchmark numbers are void, or that this was caught before any published result existed.

💭 Nits

8. "git diff --check:PASS" (Section 7) — only checks whitespace; listing it alongside "完整 pytest" gives misleading confidence. Move it to a "hygiene" sub-bullet or drop it.

9. Section 3.4 says "naive 分配器…为同一条二元指令的不同虚拟源分配不同 scratch register" — this was a fix, but naive allocators should always have done this. Either the bug was subtle (and deserves one line of explanation), or the wording implies the old behavior was standard-compliant when it wasn't.

10. PowerShell backtick continuations (Section 5.2 command block) — fine for PowerShell, but consider noting the shell for cross-platform readers.


Overall

The report is thorough and well-organized for a development-stage benchmark doc. The main concerns are internal contradictions around what the benchmark actually exercises (item 1), unexplained anomalies in the results (item 2), and missing context on what makes the correctness oracles trustworthy (item 4). These should be resolved before this document is used to justify merging.


📁 benchmarks/bench_const_merge.py

🟡 Consistency gap — If other benchmark functions (e.g., bench_merge_* variants) return similar dicts, they likely need this field too. Adding it to only one function creates schema inconsistency across benchmark outputs. Consider extracting this metadata to a shared helper or adding it uniformly.

💭 Downstream consumers — Adding a new key to the returned dict is a minor breaking change for any code doing strict schema validation (e.g., JSON Schema checks or dict key assertions). Confirm no consumers break.


📁 benchmarks/bench_regalloc_spill_compare.py

Code Review: benchmarks/bench_regalloc_spill_compare.py

🔴 Bug: ra incorrectly classified as callee-saved_SAVED_REGS (line ~40): ra is RISC-V caller-saved (x1). Including it means any sd/ld ra, offset(sp) is classified as frame save/restore instead of spill traffic, undercounting spills.
Suggestion: Remove "ra" from the set. Only callee-saved registers belong here.

🟡 Performance: _peak_live is O(range × intervals) — Lines ~168-172: Iterates every integer position and checks every interval. For wide liveness ranges with many intervals, this is effectively quadratic. A sweep-line (sort by start/end, count deltas) would be O(n log n).

🟡 Exception handling too narrow — Line ~244: run_case only catches (ImportError, OSError, RuntimeError, ValueError). llvmlite can raise TypeError, ValueError from IR parsing edge cases, or other uncaught exceptions would abort the entire suite.
Suggestion: Catch Exception here since compile_llvm is wrapped in a try/except by design to isolate per-case failures.

🟡 spill_slots is a unique-offset proxy, not a true slot count — Line ~143: spill_offsets.add(offset) counts distinct offsets. If the allocator reuses an offset for two non-overlapping live ranges, the count is wrong. The compile_scratchv sanity check (spill_slots != allocator.spill_slot_count) mitigates this for ScratchV, but LLVM's count may silently be underreported.

💭 s0/fp redundancy — Line ~40: s0 is a RISC-V pseudonym for fp. Both names in _SAVED_REGS is harmless for robustness (covers both naming conventions in assembly), but a comment would clarify intent.


📁 benchmarks/regalloc_spill_cases/00_low_pressure_chain.dsl

No blockers. Two minor observations:

💭 Nit: Input operands have surprisingly long live rangesa is live from t00 → t07, b from t00 → t08, c from t01 → t09, d from t02 → t06. So there are always 4 long-lived inputs plus 1 short-lived temp = 5 live values at any point. If the target has < 5 registers this won't stay "low pressure" as intended. Verify the register budget matches, or use fewer distinct inputs to keep live count ≤ N-1.

💭 Nit: No companion .expected or assertion file — presumably handled by the test harness, but a brief header comment noting expected behavior (e.g., # Expect: zero spills, 5 regs) would make this case self-documenting for future contributors.


📁 benchmarks/regalloc_spill_cases/01_wide_fanout_32.dsl

🟡 Comment inaccuracy — Line 2: claims "Four recurrence chains" but values cross-mix starting at v02 (mul(a,c)) / v03 (div(b,d)). After v03 there is no clean chain separation — all 32 values are in a single dependency graph. Misleading for anyone debugging unexpected CSE/DCE behavior.

🟡 Division-by-zero risk — Multiple div ops by b, c, d with no guard. If this benchmark runs with arbitrary inputs, these will trap. If it's compile-only, add a comment stating that assumption.

💭 Missing expected-outcome comment — Benchmark cases should document the hypothesis being tested, e.g.:

# Expect: ≥16 spills on 16-register targets; peak live-set = 32 at p-level boundary.

Without this, the test passes or fails but the reader can't tell whether the result matches intent.


📁 benchmarks/regalloc_spill_cases/02_double_use_40.dsl

🟡 Reverse chain inconsistency — Lines 73–113:

r00 = mul(v39, v38) breaks the accumulator pattern (r01–r38 all use add). The forward chain (f00f38) is uniformly add(prev, vN) including the seed. If mul was intentional to vary the op mix, it's undocumented; if not, consider r00 = add(v39, v38) for symmetry. If mul is kept intentionally, add a brief comment.

💭 Title precision — Line 1: "Sustained pressure" is vague. Something like "40 live values across forward + reverse reduction (double use)" would better convey what makes this case distinct from other spill benchmarks in the directory.

Everything else is correct — all 40 v-values are live across the full forward-to-reverse span, no dead code, no undefined ops, and the structure cleanly models the intended scenario.


📁 benchmarks/regalloc_spill_cases/03_lifetime_holes_36.dsl

🟡 Observation: 54 live values span the middle, not 36 — The header comment mentions "36 anchors" but the e00e17 block (18 values) is also live through the g-chain since er consumes them after q. This means the allocator must keep 54 values resident during the quiet middle region (36 v + 18 e + up to 20 g), which is significantly higher pressure than the comment suggests. Consider updating the comment to clarify the total live set, or restructuring e to be computed after the g-chain if the intent is to test 36-only pressure.

💭 Nit: Naming asymmetry between forward and reverse chains — The forward reduction uses e## for paired sums and er## for the final chain; the reverse chain uses q## for the reduction but no intermediate naming for its result. Consistent naming (e.g., q##qr##) would make the pattern easier to scan at a glance.

No correctness bugs — all references resolve, the chain is complete (v00–v35 and e00–e17 fully consumed), and the return path is valid. The test structure correctly exercises live-range splitting: the allocator should be able to evict v/e during the g-chain and reload for q/er.


📁 benchmarks/regalloc_spill_cases/04_hot_cold_48.dsl

🟡 Terminology mismatch — "cold" values are actually long-lived — The comment says "48 cold values wait," but v00–v47 are consumed sequentially in the output chain (out00–out47), giving each a live range spanning ~80 instructions. These are high-pressure values, not cold in the traditional register-allocation sense. "Cold" typically implies short liveness or infrequent use; here use frequency is low (once each) but liveness is extremely long. A reader familiar with RA terminology may be confused.

Suggestion: Rename to "long-lived / single-use values" or add a parenthetical clarifying that "cold" means infrequent use despite long liveness.


🟡 Division-by-zero safety — 12 div ops (v03, v07, v11, …, v47) divide by operands that include runtime inputs (a, b, c, d) and intermediate values with no zero guard. If this DSL has a runtime interpreter or JIT backend, undefined/exception behavior is possible.

Suggestion: Add a comment noting the benchmark assumes non-zero divisors, or use add/mul substitutions if runtime execution is in scope.


💭 No documented register budget — The benchmark's spill decisions are entirely register-count-dependent (at peak: ~48 v-values + 1 h-value + 4 inputs ≈ 53 live). Without stating the target register count (or ISA), results aren't reproducible across environments.

Suggestion: Add a # expected_pressure: 53 live at peak or similar annotation so consumers can calibrate.


💭 Output chain is fully serialout00→out01→…→out47 forms a zero-ILP chain. This is intentional (prevents scheduling reordering), but worth a one-line comment so readers don't mistake it for an oversight and "fix" it.


📁 benchmarks/regalloc_spill_cases/README.md

🟡 Magic number without source — "19 default integer registers" will confuse anyone unfamiliar with ScratchV's register file. List which registers are excluded (reserved, callee-saved, etc.) or link to the allocator config so the count is verifiable.

🟡 Mixed RV32/RV64 instructions in one category — "anchored sw/lw/fsw/flw/sd/ld/fsd/fld stack accesses" lists both 32-bit and 64-bit scalar access sizes. If the benchmark targets a single ABI, remove the non-target instructions; if it covers both, state which is which.

🟡 Ambiguous column header — "Expected ScratchV boundary" is unclear. "Expected spill behavior" or "Spill threshold" would be more self-explanatory for new contributors.

💭 Reproducibility gaps — No mention of required LLVM version, RISC-V target ABI (RV32/RV64), or host OS. Benchmark results are version-sensitive; even one line of "Tested with LLVM 18.x on Linux/RV64GC" would help.

💭 No output format example — Users running --json-output /tmp/spills.json have no idea what the JSON schema looks like. A 3–5 line snippet would save a round trip.



⚠️ 未审查的文件

  • benchmarks/test_regalloc/init.py
  • benchmarks/test_regalloc/bench_cnn.py
  • benchmarks/test_regalloc/bench_dense.py
  • benchmarks/test_regalloc/bench_pseudo.py
  • benchmarks/test_regalloc/bench_regalloc_linear.py
  • benchmarks/test_regalloc/bench_simple.py
  • benchmarks/test_regalloc/bench_utils.py
  • benchmarks/test_regalloc/regalloc.md
  • docs/topic17_AI自审报告.md
  • docs/topic17_P1实现报告.md
  • docs/topic17_benchmark文档.md
  • scratchv/backend/abi_frame.py
  • scratchv/backend/const_merge.py
  • scratchv/backend/inst_select_ext.py
  • scratchv/backend/instruction_select.py
  • scratchv/backend/machine_semantics.py
  • scratchv/backend/regalloc_cfg.py
  • scratchv/backend/regalloc_linear.py
  • scratchv/backend/regalloc_linear_v1_5.py
  • scratchv/backend/regalloc_metrics.py

yuki-328 and others added 5 commits September 5, 2026 22:59
Integrate the provided benchmark patch with the canonical 19-register bank and preserve the pseudo-instruction lowering helpers.

Co-authored-by: KangjieZhang <KangjieZhang1112@gmail.com>
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