Impl/topic17

[FeelTheBeats]

No description provided.

[github-actions]

🤖 AI Code Review

共审查 10 个变更文件
⚠️ 另有 6 个文件超过上限（最多 10 个）未审查

📁 .github/workflows/ci.yml

🔴 Bug: Duplicate test execution — tests/test_pr37_regression.py is run twice: once in the existing step (around line 107) and again in the new "topic17 register-allocation regressions" step. This wastes CI time and can cause confusing double-failure reports.

Suggestion: Either consolidate both test invocations into a single step, or remove the duplicate from the new step:

- name: Run topic17 register-allocation regressions
  run: |
    python3.12 -m pytest \
      tests/test_regalloc_topic17.py \
      tests/test_topic17_regalloc_case_report.py \
      -v --tb=short

Then merge the old test_pr37_regression.py run into this step instead.

---

🟡 Suggestion: Benchmark step missing ref guard — The "Topic 17 register-allocation case report" step has no if: github.ref == 'refs/heads/main' condition, so it runs on every PR. If this benchmark is non-trivial, it adds CI latency on every branch push for zero benefit (the summary guard if [ -f ... ] will just skip it on PRs anyway since the file won't exist).

Suggestion: Add the same guard used by the visualization step:

- name: Topic 17 register-allocation case report
  if: github.ref == 'refs/heads/main'
  run: |
    ...

Unless the intent is to run benchmarks on PRs too — in which case the summary section should be made consistent (it currently only benefits on main).

---

📁 benchmarks/cases/topic17_regalloc_feature.dsl

🟡 Missing result assertion — No expect/assert k == 30 (or equivalent) in the DSL. If the framework supports a postcondition, add it so the benchmark's correctness contract is self-documenting and enforced. The header comment spells out k = 30, but nothing here verifies it at runtime.

🟡 Comment vs. construct mismatch — "force-spilled across basic blocks" (line 4) implies multiple explicit basic blocks, but the loop body is a single block followed by the back-edge branch. The spill is really "loop-carried save/restore at the loop header," not cross-BB pressure. Reword to avoid conflating two distinct regalloc mechanisms — this file's whole point (per its header) is to test linear scan + frame layout, so precision here matters for reviewers reading the benchmark matrix.

💭 Loop bound readability — for i = 0, 6 is ambiguous to a new reader (inclusive of 6? exclusive?). A one-line # i ∈ {0..5}, 6 iterations under the loop would prevent misreading, especially since the assertion of k = 30 only makes sense if 5 is the last value.

💭 File scope — Filename topic17_regalloc_feature.dsl has no extension hint for the DSL variant (v1.5 linear scan). If other topic-17 cases target different regalloc strategies, consider a suffix like _ls15.dsl so the case matrix is scannable without opening files.

Otherwise the case is minimal and appropriate for a low-pressure baseline — keeping it short is correct.

---

📁 benchmarks/run_topic17_regalloc_case.py

🔴 Bug: max(epilogue) on empty list crashes report generation — ~line 397
If the linear path emits a prologue (addi sp, sp, -16) but no matching epilogue, epilogue is empty and max() raises ValueError. This is exactly the kind of bug the report should surface — instead the script crashes before reporting it.

# Current (crashes when epilogue is empty):
f"prologue addi sp, sp, {min(prologue)}, epilogue addi sp, sp, {max(epilogue)}"

Suggestion: guard both sides independently:

frame_note = (
    f"prologue addi sp, sp, {min(prologue) if prologue else 'n/a'}, "
    f"epilogue addi sp, sp, {max(epilogue) if epilogue else 'n/a'}"
    if (prologue or epilogue) else "no frame adjustment"
)

---

🟡 Hardcoded greedy frame size in markdown table — ~line 401
"Frame size (bytes) | 0 | ..." hardcodes 0 for greedy instead of reading greedy['frame_size']. If greedy ever uses a frame, the report silently lies.

f"| Frame size (bytes) | {greedy['frame_size']} | {linear['frame_size']} | "

---

🟡 _sp_adjustments classifies by sign, not program position — ~line 65
Negative imm → "prologue", positive → "epilogue". This works for simple straight-line code, but misclassifies if there are conditional frames, nested calls, or restoration before the final return. The comment says "prologue" / "epilogue" which implies positional semantics, but the implementation is sign-based.

Consider documenting this is a heuristic, or splitting by instruction offset (e.g., before vs. after the largest negative offset).

---

🟡 Compilation timing discarded on failure — ~line 195
When compile fails, times has a valid measurement but _failed_measurement hardcodes compile_time_ms: 0.0. The actual time-to-failure is useful diagnostic info.

return _failed_measurement(mode, errors or [...])
# → pass times if available, or at least note that timing was lost

---

💭 No error handling on file writes in main — ~line 430
args.json.write_text(...) and args.markdown.write_text(...) will crash on permission errors. Wrap in try/except with a clear message, or let the traceback speak (acceptable for a dev tool).

---

💭 check_hygiene strips comments with # only — ~line 113

body = "\n".join(line.split("#", 1)[0] for line in asm.splitlines())

If the repo's assembler ever supports ; or // comments, SPILL_/vreg markers hidden in those comments would be missed. Low risk given GAS syntax, but worth a comment.

---

📁 benchmarks/test_regalloc/bench_cnn.py

🟡 Private import — _INT_REGS is underscore-prefixed, indicating it's an internal implementation detail. If regalloc_linear later renames/refactors this symbol, the benchmark breaks silently.

Suggestion: If _INT_REGS is only needed for constructing test data, consider exporting a public accessor or duplicating the constant in the test.

---

📁 benchmarks/test_regalloc/bench_dense.py

🔴 Benchmark semantics change undocumented — strict=False alters the allocation algorithm's behavior. If previous results were collected with strict=True (default), all historical comparisons are now invalid. Add a comment explaining why strict=False is the intended mode, or parameterize the benchmark to test both.

🟡 Module rename may break other references — regalloc_linear_v1_5 → regalloc_linear: verify no other benchmarks/tests still import the old path, or that a deprecation shim exists.

🟡 Accessing private attribute — alloc._spill_slots (line ~71) relies on implementation detail. If the internal API changes, this benchmark silently breaks rather than failing. Consider exposing a public counter or at least documenting the coupling.

💭 Directory naming — benchmarks/test_regalloc/ uses a test_ prefix, which pytest will attempt to collect. If not intentionally part of test discovery, rename to benchmarks/bench_regalloc/ to avoid accidental invocation and CI slowdown.

---

📁 benchmarks/test_regalloc/bench_simple.py

No issues. Clean import path update, no behavioral change.

💭 Ensure all other references to regalloc_linear_v1_5 are updated too — if this was a module rename, a stale import elsewhere could silently test the old code path. Run: grep -r "regalloc_linear_v1_5" .

---

📁 scratchv/backend/__init__.py

🟡 Over-exposed internals — LsInstruction, LiveInterval look like implementation details of the linear-scan allocator. If consumers don't construct them directly, they shouldn't be in __all__. Exposing them makes them an implicit API contract.

🟡 No __all__ section for frame_layout — FunctionFrameAllocator and FrameInfo are appended inline rather than grouped under a comment header like the others (# frame layout), breaking the existing pattern.

---

📁 scratchv/backend/machine_types.py

🔴 Breaking change: ALL_REGS semantics silently widened (19 → 27 regs)
The change adds ARG_REGS (a0–a7) to ALL_REGS. If any allocator/consumer still imports ALL_REGS expecting the old 19-reg "safe temporaries + saved" set, it will now hand out argument registers in contexts where that conflicts with calling conventions (e.g., spilling across calls, treating a0 as scratch after a callee). GREEDY_REGS preserves the old set, but this diff doesn't show call-site migrations. Verify all ALL_REGS consumers were audited; if not, this is a correctness regression waiting to happen.

🟡 Memory operand stored as a flat string — structure lost
mem() packs offset(base) into a str in value. Repr is fine, but any later logic that needs to rewrite the offset or base (spilling, index adjustments, relocation) will have to regex-parse "8(sp)" back. Consider value: tuple[int, str] | int or a small dedicated dataclass so the IR stays structured.

🟡 mem() has no input validation
Nothing prevents mem(0, "zero"), negative offsets for loads (legal but worth noting), or non-int offsets (value: str | int allows "8(sp)" directly bypassing mem()). At minimum assert isinstance(offset, int) and consider a whitelist of valid base regs.

🟡 REG_NUMS mixes canonical names with aliases without separation
"x0": 0, "zero": 0 and "s0": 8, "fp": 8 both appear. Fine for lookups, but code that iterates REG_NUMS.keys() (e.g., to enumerate allocatable regs, print a table) will see duplicates. Consider a primary-name dict plus a separate alias map, or document that iteration order/keys is not meaningful.

🟡 No tests shown for allocator-order change
Going from 19 → 27 allocatable regs changes every assignment the linear scan produces. Without a test that pins down expected register assignments (or a golden-output test) on a representative function, regressions will be hard to spot.

💭 Docstring drift
"""A register, immediate, or memory operand.""" is good, but the kind comment list # "reg", "imm", "vreg", "mem" still doesn't mention that "mem" values are str while "imm" are int | str. A one-line note would help the next reader.

💭 Legacy naming
GREEDY_REGS with a comment saying "Frozen" is a code smell that will confuse in a year — someone will wonder why there are two register pools. Consider renaming to something explicit like LEGACY_ALL_REGS and adding a deprecation/migration note, or better, update call sites and delete it.

---

📁 scratchv/backend/regalloc_linear_v1_5.py

🔴 Missing __all__ — Without an explicit __all__, any consumer doing from regalloc_linear_v1_5 import * gets an uncontrolled re-export surface. The * import's scope depends entirely on regalloc_linear's own __all__ (if defined) or its namespace contents (if not). Adding __all__ here pins the guaranteed compatibility contract and makes the removal audit trivial.

__all__ = [
    "LinearScanAllocator", "LiveInterval", "LsInstruction",
    "RegAllocError", "RegisterAliasError", "SpillFallbackError",
    "block_from_machine_instrs", "machine_instrs_from_block",
    "_DEFAULT_PHYS_REGS", "_FP_REGS", "_INT_REGS", "_REG_NUMS",
]

🟡 Behavioral divergence risk — The removed _REG_NUMS, _INT_REGS, _FP_REGS, _DEFAULT_PHYS_REGS definitions are now sourced from regalloc_linear. If those modules ever diverged (e.g., different register subsets), this re-export silently changes allocator behavior. Add an assertion or import-time check:

from scratchv.backend.regalloc_linear import _REG_NUMS as _R  # noqa
assert _R["a0"] == 10 and _R["t6"] == 31  # spot-check key entries

Or better, a test that imports both modules and asserts the register tables are identical.

🟡 No deprecation warning at import time — A DeprecationWarning would catch consumers in the wild who haven't noticed the docstring:

import warnings
warnings.warn(
    "regalloc_linear_v1_5 is deprecated; import from "
    "scratchv.backend.regalloc_linear instead",
    DeprecationWarning, stacklevel=2,
)

💭 No trailing newline — Add a final newline to satisfy POSIX tooling.

💭 Explicit imports + import * is belt-and-suspenders but slightly confusing — Consider dropping import * and keeping only the explicit list (paired with __all__). It makes the re-export surface self-documenting in one place.

---

📁 scratchv/backend/register_alloc.py

🔴 Doc/code mismatch — Line ~2 & ~114: docstring says "temp registers first" but code allocates from GREEDY_REGS (not TEMP_REGS), and there's no ordering/priority logic — it just uses the whole pool. If GREEDY_REGS != TEMP_REGS, the docs are wrong; if they're equal, drop "first" and name it directly.

🟡 Silent behavior change, no test — _reg_pool shrinking from ALL_REGS → GREEDY_REGS is the real semantic change here (in both __init__ and _allocate_greedy). No test added for:

• a vreg that previously fit in a callee-saved slot and now must spill
• interaction with CALL (callee-saved preservation correctness across calls)

Add at least one regression case that exercises the GREEDY_REGS boundary.

🟡 REG_NUMS exported but unused — Line ~22 & ~44: added to imports and __all__ but not referenced in this file. Confirm downstream consumers actually need it here, or it's dead surface area.

🟡 Legacy-warning duplication — The "spill path is not reload-correct, use linear-scan" note appears both in the module docstring (lines ~6-7) and the class docstring (lines ~56-58). Keep it in one place (module-level is right, since it's about the file's strategy choice).

🟡 Broken spill path stays reachable — Greedy still calls _spill_slot() when GREEDY_REGS is exhausted, hitting the known-broken path. Now that the pool is smaller, more inputs will fall into it. Consider either (a) raising an error/abort in greedy mode when spill is needed (since linear-scan is the sanctioned path), or (b) making greedy mode default to linear when spilling is required. Silent wrong-codegen is the worst failure mode.

💭 Naming drift — GREEDY_REGS is only defined in machine_types and re-exported here; its semantic ("the regs the greedy allocator may use") is unclear from the name alone. A comment in machine_types defining it as TEMP_REGS | ... would help future readers understand why the pool changed.

---

---

⚠️ 未审查的文件

• scratchv/backend/topic17_bottleneck_scenarios_v1_5.py
• scratchv/compiler.py
• scratchv/main.py
• tests/test_pr37_regression.py
• tests/test_regalloc_topic17.py
• tests/test_topic17_regalloc_case_report.py