Impl/topic15

[FeelTheBeats]

No description provided.

[github-actions]

🤖 AI Code Review

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

📁 .github/workflows/ci.yml

Code Review: CI Workflow Update

🔴 Bug: Potential test duplication — Lines 110-117: These 4 test files (test_inliner.py, test_ir_call.py, test_backend_call.py, test_topic15_inline_case_report.py) may already be collected by an earlier pytest step (e.g., a catch-all tests/ run). If so, this new step re-runs them, doubling CI time with no benefit.
Suggestion: Verify these files aren't already covered by a broader test step. If not, this is fine.

🟡 Suggestion: Missing timeout-minutes on benchmark step — Lines 228-235: run_topic15_inline_case.py has no explicit timeout. If the inliner encounters a pathological case (deep recursion, exponential expansion), it could hang the CI job indefinitely. Existing benchmark steps have the same gap, so this is a pre-existing issue, but since you're touching this section anyway:

        timeout-minutes: 10

🟡 Suggestion: No if: failure() guard on visualization step — Line 118 (existing, not introduced here): The new test step runs unconditionally, which is correct. Just flagging that the existing visualization step (which follows immediately after) is gated by github.ref == 'refs/heads/main', meaning on PRs the inliner_report.md file may be generated but never displayed in $GITHUB_STEP_SUMMARY. This is fine for PRs but means inliner report visibility is PR-only for the JSON artifact, not the markdown summary. Consider whether PR reviewers need the markdown report in the summary too.

💭 Nit: Comment language consistency — Lines 228, 231: The new comments use Chinese (# ── 3.1.2 课题15...), while the surrounding comments in the same section are also Chinese (# ── 3.2 DSL 用例编译...), so this is actually consistent. No action needed.

Summary: The only blocker-level concern is verifying no test duplication. The timeout suggestion is worth applying as a hardening measure. Everything else is solid — the pattern (test → benchmark → summary) matches existing workflow structure cleanly.

---

📁 benchmarks/cases/topic15_inline_feature.py

🟡 Suggestion: build_rejected_program 命名参数缺少类型声明 — Line 82–84

loopy 的 a 参数声明为 FLOAT32，但 build_program 中的 x 也是 FLOAT32。两者一致，但如果未来 IRBuilder 支持多精度或不同 callee 需不同类型，这两个函数之间缺乏约束。考虑用类型常量统一引用，或在 docstring 中明确"所有值均为 FLOAT32"作为契约。

💭 Nit: for _ in range(6) 意图可再明确 — Line 76

该循环纯粹是为了凑够指令数（store + 6 const + add + ret = 9）。考虑换行注释或常量名 PAD_INSTRS = 6 增强可读性，避免未来维护者误以为这里应有语义循环体。

PAD_INSTRS = 6  # pad to reach max_instrs threshold + 1
for _ in range(PAD_INSTRS):
    b.load_const(1)

💭 Nit: 变量名 b — 全文件

b = IRBuilder() 在两处使用，而 b 也是 build_rejected_program 内部 make_const 结果的惯用缩写。当前不冲突，但建议改用 builder 或 ib 避免认知歧义。

---

除此之外代码正确、结构清晰、文档与实现匹配，无明显缺陷。

---

📁 benchmarks/run_topic15_inline_case.py

🔴 Bug: ir_grows_by_cloned_bodies invariant is off by the removed CALLs — evaluate(), checks dict:
on["ir_instructions"] - off["ir_instructions"] == on["clones"] * off["callee_body_size"] ignores that each inlined site deletes one OpCode.CALL. Since inlined_removes_all_calls asserts call_count 2→0, the true delta is clones * body_size - call_count_off. As written this check is always False, so hard_failures is never empty and main() always exits 1 (the script can never report PASS).
Suggestion: ... - off["call_count"] == on["clones"] * off["callee_body_size"], and run once to confirm.

🟡 Fragile coupling to warning count/wording — rejected_warnings_recorded:
Requires exactly REJECTED_CALL_SITES warnings, plus one containing body_too_large and one loop_body_unsupported, all starting inliner: skip. If the inliner dedupes per-callee, or emits one warning per rejection reason per site, this hard check fails on behavior that is otherwise correct. Consider asserting "≥1 warning per distinct refusal reason and 0 calls inlined" instead of an exact count.

🟡 Magic constants duplicated across modules — ELIGIBLE_CALLEE, ELIGIBLE_CALL_SITES, REJECTED_CALL_SITES, REJECTED_MAX_INSTRS are hard-coded here and independently inside topic15_inline_feature.py. Change the case and the hard checks silently go stale (or, as above, start failing). Prefer importing them from the case module.

🟡 Ugly failure mode when the case module is malformed — load_case_module validates only importability; a module missing build_program / build_rejected_program surfaces as a bare AttributeError deep in evaluate(). Validate the two callables and parser.error() / RuntimeError with the expected attribute names.

🟡 A/B "inliner off" column reports vacuous zeros — _measure_uninlined never runs the inliner, yet the Markdown table prints clones=0, rejected=0, rounds=0 with +0 deltas. Readers may read "inliner off inlined 0 sites" as a measurement. Use n/a for those cells (as already done for pass time).

🟡 Determinism is only within-process — fingerprints are compared across repeats in one process; program.dump() stability across processes (and whether it embeds object ids/addresses, which the docstring only asserts) is untested. The report wording "deterministic" overstates the evidence.

💭 print(render_markdown(report)) re-renders the whole report; render once, write both files, then print.

💭 DEFAULT_JSON/DEFAULT_MARKDOWN are CWD-relative while DEFAULT_CASE is __file__-relative, so output location drifts with invocation directory. Pin both to the same base.

💭 assert program is not None and runner is not None — narrowing assert is stripped under -O; use an explicit if raise.

💭 f"{on['pass_time_ms']:.4f}" raises TypeError if a future refactor leaves it None; guard like the JSON path does.

💭 by_index = {d["index"]: d for d in ...} silently collapses duplicate clone indices, which would then defeat clone_names_do_not_collide rather than detecting it.

💭 failed = sorted(...) prints failures alphabetically, breaking the ordering of hard_checks in the Markdown checklist.

💭 _is_cont_block excludes _cont blocks from clone grouping, so definitions inside continuation blocks are never checked for namespace collision by dest_names (only _duplicate_defined_names catches those).

💭 Uninlined verifier ERROR count is never asserted (only post-inline is required clean), so a future verifier regression on the residual-CALL path goes unnoticed despite being documented in HONESTY.

---

📁 scratchv/backend/instruction_select.py

🟡 Bug: Caller-saved registers not preserved around call — Lines ~264-267: JAL overwrites ra, but arguments staged into a0-a7 are also caller-saved. If the caller's prologue doesn't spill/restore ra and a0-a7, the generated code will corrupt state. This is likely a design gap — verify that _select_prologue/epilogue covers this when allow_uninlined_calls=True.

🟡 Bug: Error message references non-existent parameter — Line ~258: "set minimal_call_codegen=True" — the actual parameter is allow_uninlined_calls. This will confuse users who follow the advice.

raise UnsupportedCallError(
    f"CALL {callee} in function "
    f"'{self._current_function_name}': ABI support "
    f"(prologue/epilogue, stack args) is not implemented; "
    f"enable inlining (--inline) or set minimal_call_codegen=True")  # ← wrong name

Suggestion: Change to allow_uninlined_calls=True.

🟡 Bug: Parallel-safe staging is correct but fragile against future changes — Lines ~262-267: The two-pass staging (temp → a-regs) correctly prevents mv a1, a0 from clobbering a0 before mv a0, X reads it. This is sound. However, the temp names are bare strings (_call_arg{i}_{counter}) — if the register allocator ever switches from string-keyed vregs to numeric IDs, this silently breaks. Consider adding a brief docstring warning or an assertion.

🟡 Perf: Global call counter grows unboundedly — Line ~263: _call_counter never resets, so temp names get longer with every call in the program. Cosmetic, but a per-function counter would keep labels shorter and debugging easier.

💭 Nit: _call_counter is incremented after being used in temp names? No — it's incremented before the loop (self._call_counter += 1), so the first call produces _call_arg0_1. This is fine, just note the counter starts at 0 and the first real value is 1.

💭 Nit: No validation that callee is a non-empty, valid symbol name before generating JAL ra, <callee>. If instr.target is None or empty, JAL gets garbage. A guard like assert callee or explicit check would help.

---

📁 scratchv/backend/llvm_codegen.py

🟡 Confusing error when instr.target is None — Line 180:
If CALL is emitted without a target (e.g. an unresolved reference or a malformed IR node), the message reads CALL None in function .... Consider:

f"CALL {instr.target or '<unknown>'} in function ..."

💭 Defense-at-depth placement — The guard fires at instruction-emit time, which is correct, but a caller might hit this deep in compilation with no easy way to know why. If there's a function-entry or IR-validation stage upstream, a pre-check there could give earlier/better diagnostics. Not blocking — current placement is fine.

That's it. The change is clean: early-return before the generic dispatch, clear error message with actionable advice (inline or switch backend), and consistent with the RISC-V backend's behavior.

---

📁 scratchv/compiler.py

🔴 Bug: phys_regs may not exclude argument registers — Line ~470: phys_regs = list(ALL_REGS). The comment says to "reserve a0..a7 for the CALL staging sequence," but if ALL_REGS includes a0–a7, the allocator can still map vregs onto those registers, defeating the purpose. Verify ALL_REGS actually excludes x10–x17 (a0–a7). If it doesn't, this needs to be ALL_REGS - set(ARG_REGS) or equivalent.

🟡 _program_has_call: use == not is — Line ~557: ins.opcode is OpCode.CALL. Identity comparison works for singletons/enums but is fragile if OpCode ever uses value-based comparison or has non-singleton instances. Use == for robustness.

🟡 _program_has_call: no early termination — Full scan of all blocks even after first match. For large programs this is wasteful. Consider:

for func in program.functions:
    for block in func.blocks:
        for ins in block.instructions:
            if ins.opcode == OpCode.CALL:
                return True
return False

🟡 Warning string is informal — Line ~306: "inliner requires --optimize basic|all; skipped" reads like a CLI hint, not a compiler diagnostic. Consider: "--inline was requested but --optimize is 'none'; inlining pass skipped".

💭 Lazy imports inside _run_optimizations and _generate_riscv_linear — Works, but if scratchv.optimizer.inliner or scratchv.backend.machine_types are always available, hoisting to module level would simplify debugging and avoid repeated import overhead. Only keep lazy if these are truly optional or circular-dependency-sensitive.

---

📁 scratchv/ir/builder.py

🟡 Silent type mismatch risk — dtype=DataType.FLOAT32 as default with has_ret=True. If the callee returns an int or vector but the caller omits dtype, the IR silently declares a FLOAT32 result. Consider either:

• Requiring dtype when has_ret=True (raise ValueError if default is used), or
• Documenting loudly that callers must pass dtype for non-float32 callees.

🟡 Fragile return-value contract — has_ret silently controls both whether a Value is created and what's returned. A misspelling (has_ret=True on a void callee) won't error at construction; it'll produce a dangling SSA value in the IR. Consider an overload-based design or a two-method split (call / call_void) to make misuse a type error rather than a logic error.

💭 args or [] swallows empty lists identically to None — Functionally correct but ambiguous at call sites: call("foo", []) and call("foo", None) are indistinguishable. If that ever matters (e.g., distinguishing "no args" from "arg list was empty"), switch to list(args) if args is not None else [].

---

📁 scratchv/ir/types.py

🔴 Opcode added but downstream handlers likely not updated — CALL is a new enum member. Any exhaustive match/if-elif chains on OpCode (lowering, verifier, printer, interpreter, JIT, serialization) will now silently hit a fallthrough/else branch instead of raising. Grep for OpCode. and match op across the codebase and confirm each site handles CALL. If your style is "fail fast on unknown op," this is a correctness risk, not a nit.

🟡 Future-dated comment — # ── Topic 15: interprocedural (2026-09-14) ──. Either a typo (2024/2025?) or a real project milestone you want preserved. If it's a tracking marker for internal roadmap items, fine; if it was meant as a commit date, it's misleading for anyone reading blame/history.

🟡 Inconsistent comparison style — is_call() uses self is OpCode.CALL while is_arith() / is_control_flow() use self in (...). For a single value both are correct, but mixing idioms in one method family invites "which one is right?" comments later. Pick one — either make single-opcode checks use in (OpCode.CALL,) for symmetry, or document that is is intentional for the common single-case fast path. Note also self is OpCode.CALL relies on Enum identity, which is guaranteed, so it's not a bug — just a style call.

💭 Missing docstring parity — is_call() has a docstring, but is_arith() and is_control_flow() don't. Either add short docstrings to all three for consistency, or drop the one-liner here to match.

💭 No test for the new opcode — even a trivial assert OpCode.CALL.is_call() and not OpCode.ADD.is_call() would lock in the contract and catch regressions if someone later refactors these helpers into a table/dict.

The enum addition itself is clean and minimal; the real question is whether the rest of the pipeline was updated in the same changeset. If yes, ignore the first point. If no, that's the blocker to resolve before merge.

---

📁 scratchv/main.py

🔴 Warnings only shown on error path — The new warning loop sits inside the else branch after if not result.errors. On successful compiles with warnings, they're silently swallowed. If inliner emits "skipped X calls" or similar notes, users won't see them on happy paths.
Suggestion: Move the warning loop above the if not result.errors block (or duplicate in success path).

🟡 --inline requires --optimize per help text but nothing enforces it — User can pass --inline alone, config.inline=True, and the inliner never runs (or runs silently). Either drop the "requires" phrasing from help, or add a validation check that exits with a clear message.

🟡 --inline-max-instrs accepts 0 / negative — type=int alone doesn't bound the value. An inliner with threshold ≤0 either never inlines (confusing) or has undefined behavior. Consider choices / a validator callback asserting > 0.

🟡 --minimal-call-codegen produces non-executable output with no runtime warning — Help says "not executable" but nothing warns the user at emit time. Consider printing a stderr note when this flag is active so downstream tooling / CI can detect it.

💭 Arg→Config mapping is positional-blind — Fine today, but if CompilerConfig fields get reordered or renamed, these kwargs silently break. Minor; only flagging since this pattern is repeated for every new flag.

💭 Nit: The four new inline-related args would read better grouped under a comment block like the existing "Cycle estimation" section for consistency.

---

📁 scratchv/optimizer/__init__.py

💭 **API surface:** Line 9/11 — `InlinerConfig` is exported in `__all__`.
   If users only pass it to `Inliner` internally, consider keeping it out of `__all__`
   to signal it's not a primary API. If intentional, no action needed.

No other issues. Import/export are consistent, no circular import risk at this layer.

---

---

⚠️ 未审查的文件

• scratchv/optimizer/dead_code.py
• scratchv/optimizer/inline.py
• scratchv/optimizer/inliner.py
• scratchv/optimizer/licm.py
• tests/test_backend_call.py
• tests/test_inliner.py
• tests/test_ir_call.py
• tests/test_topic15_inline_case_report.py