Impl/topic10

[FeelTheBeats]

No description provided.

[github-actions]

🤖 AI Code Review

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

📁 .github/workflows/ci.yml

Code Review: .github/workflows/ci.yml

🔴 Potential job ordering dependency — The test tests/test_loop_unroll_case_report.py (line ~110) runs in the test job, while benchmarks/run_topic10_unroll_case.py (line ~228) produces loop_unroll_report.{json,md} in the benchmark job. If the test reads/validates those artifacts, it will fail with a missing-file error. Confirm test_loop_unroll_case_report.py is self-contained (generates its own data) or add the dependency via needs.

🟡 Unnecessary mkdir -p — Line ~226: The preceding const-merge benchmark step (and likely others) already ensures benchmark_reports/ exists. Unless this step can run standalone (e.g., job re-ordering in the future), drop it. If defensive coding is intentional, add a brief comment explaining why.

💭 Test file existence — Verify tests/test_loop_unroll_case_report.py actually exists in the repo; the CI will fail with a clear error, but catching it in review is cheaper.

---

📁 benchmarks/cases/topic10_unroll_feature.dsl

🔴 Bug: acc read before initialization — Line 5: acc = add(acc, t) uses acc on its first iteration with no prior declaration or assignment. If the intended starting value is 0, this must be explicit (acc = 0 before the loop) — otherwise the benchmark is either testing implicit zero-init (undocumented behavior) or contains a correctness hole that unrolling could expose differently than the folded form.

🟡 Loop bounds semantics unclear — Line 4: for i = 0, 4 — is the upper bound exclusive (4 iterations, i∈{0,1,2,3}) or inclusive (5 iterations)? This directly affects the expected result (10 vs 15) and whether the unrolled IR matches the folded IR. The comment references a specific "shape" but the bounds aren't pinned down in the test itself.

🟡 No expected output — As a benchmark case, there's no assertion of the expected acc value. If the harness relies solely on IR-level comparison (per the header comment), that's fine — but a numeric oracle in the file (e.g. # expected: 10) would make regressions self-documenting and catch harness-level bugs.

💭 Comment density — The header comment is thorough, which is good for a benchmark spec. Consider also annotating the expected unroll factor (e.g. # unroll-by: 4 or # full-unroll) so the test's intent is machine-checkable, not just human-readable.

---

📁 docs/topics/10-循环展开优化-开发文档.md

Code Review: docs/topics/10-循环展开优化-开发文档.md

---

🔴 Bug: Test count inconsistency — §5 lists ~10 test cases across 5 classes, but the implementation section reports "23 用例" and "36 例". A reader cannot tell which is authoritative. The table should be updated or marked as "initial plan" with a reference to the final count.

🔴 Bug: Contradictory dynamic instruction counts — §10.2 states 21 → 12, 31 → 27, 36 → 30 while the implementation section reports 30 → 15, 36 → 28, 41 → 32, 51 → 42. Both are presented as factual. Either update §10.2 with actual measurements or clearly label the original table as "theoretical" and explain the encoder-discrepancy in §10.2 itself, not just the implementation section.

🟡 Pseudocode error: iv_after filter — §3.4: any(op.name == iv.name for i in instrs if i is not region) — i is an Instruction, region is a list[Instruction]. i is not region is always True (identity check between instruction and list). Should be i not in region (or better, pre-compute outside = [x for x in instrs if x not in region]).

🟡 Missing internal method signatures — §2.1 says "命名固定，便于测试与 debug" and lists _find_pairs, _select_plan, _copy_region, _fresh_name. But _apply_unroll, _redirect_after_values, _make_remainder_loop, _make_binding, _collect_names, _value_like are used in §3–4 without being listed. If the claim is "fixed names for testing", list all of them.

🟡 Rollback scope inconsistency — §4.7 says "保存 list(block.instructions) 快照", implying only instruction-list ordering is restored. §2.2 says "指令列表、attrs 与 operands 一并还原". In-place mutations to attrs and operands won't be caught by a list copy. Clarify that deep copies (or pre-mutation snapshots of attrs/operands) are required.

🟡 dynamic_saving ≥ 2 threshold undocumented in contract — §8.2 references "C12 盈利下限 dynamic_saving ≥ 2" as a skip condition, but this threshold appears nowhere in §2 (interface) or §3.3 (_select_plan algorithm). A reader implementing from §2–3 alone would miss this check.

🟡 CLI count mismatch — §6.2 says "六个开关" but §2.4 defines 7 add_argument calls (--loop-unroll, --no-loop-unroll, plus 5 --unroll-*). §10.1 checklist says "七个开关". Pick one count and be consistent; note that --loop-unroll/--no-loop-unroll share a single dest if you want to justify "six".

💭 FOR.attrs key format ambiguous — §3.4 shows FOR.attrs = {0, q, 1, "unrolled": U}. In Python this mixes set-literal syntax with dict syntax. Specify the actual key names (e.g., {"start": 0, "end": q, "step": 1, "unrolled": U} or whatever the IR convention is).

💭 body_defs excludes iv but should — §3.4: body_defs = {i.dest.name for i in region if i.dest}. If the iv is never redefined in the body, it's not in body_defs and won't be renamed during copy — correct. But the iv_in_body flag controls whether bindings are generated; if iv is used (not redefined), iv_in_body=True triggers binding setup, yet iv itself is not in body_defs so its references are rewritten via the bind parameter in _copy_region. This works, but the interaction is subtle and could use a comment in §4.3.

💭 Section numbering drift — The implementation section at the bottom starts with "## 实现结果" but has no section number. §十 (10) is the last numbered section. Consider making it §十一 or an unnumbered appendix to avoid confusion with §10.2/10.3 references.

---

📁 docs/topics/10-循环展开优化.md

Review: docs/topics/10-循环展开优化.md

🔴 **Factual inconsistency: "14个固定原因键"** — Line ~120: 只列了 9 个键（bad_attrs / step_not_one / body_too_large / multi_def / carried_value / no_factor / growth_limit / unprofitable / unpaired），"等"不等于 14。要么补齐、要么改成实际数字。读者对照代码时会困惑。

🟡 **隐含假设未显式声明：trip count 必须是编译期常量** — 全文默认 `N` 已知（`N <= full_threshold`、因子枚举都基于编译期值），但未显式说明 `end` 为运行时值时 pass 的行为（跳过？崩溃？）。这属于 correctness 边界条件，应在概述或 核心设计表中明确。

🟡 **PARTIAL_EPILOGUE 不支持 carried value 是重大功能限制** — 常见坑 #7 说余数循环 `r>1` 不能携带值（如 `acc = add(acc, t)`），意味着最常见的累加器模式在此模式完全失效。这个限制应提升到概述或核心设计表中显著标注，而不是埋在常见坑。用户读到这里会以为所有模式都可用。

🟡 **嵌套循环：外层因内层 PARTIAL 而被跳过** — 常见坑 #6 说外层需确认内层标记已消失。但内层 PARTIAL 时标记仍存（带 `unrolled` attr），外层被 `nested_loop` 跳过。这可能导致"内层部分展开 → 外层永远不展开"的优化盲点。建议在核心设计表「配对扫描」行补充说明：内层 PARTIAL 后外层如何处理。

🟡 **"循环后引用重定向"只在任务列表出现一次，无实现说明** — 这是正确性关键路径：完全展开后，FOR/ENDFOR 被删除，原 `iv` 和携带值在循环外的引用必须重定向到末副本新名。但全文没有任何解释。对比 iv 重写和轮转命名都有段落说明，此处的缺失让读者无法评估该逻辑的正确性。

💭 **CLI 数据表 3 条指令缺乏上下文** — 013/014/019 展开后仅 3 条静态指令，即便有脚注说明，读者仍会疑惑。建议在表头或脚注补一句："数值为全程序 `--count-instr` 输出，循环体被 LICM 提空后仅残留 prologue/epilogue"。

💭 **"评审 F1 / F2 / §8.2" 等交叉引用缺少上下文链接** — 如果读者只看本文件，这些引用无法定位。建议至少加相对路径（如 `见开发文档 §8.2 (docs/dev/...)`）。

---

📁 docs/topics/INDEX.md

No issues found. Change is clean — topic 10 moved from 规划中 → 中级，counts updated consistently (9→10, 3→2), and numerical ordering within the table is preserved.

---

📁 scratchv/compiler.py

🟡 Confusing stats injection — _run_optimizations, after pm.run():

if unroll is not None:
    result.stats["loop-unroll"] = unroll.stats

On the success path this is a no-op — the _PassAdapter already extracts unroll.stats into all_stats["loop-unroll"] via p.name. On the failure/stop path it injects partial stats from the failed pass. The two behaviors differ silently. Either remove this block entirely (the adapter handles both cases), or add a comment explaining the failure-case intent.

🟡 Stats contract with LoopUnroll is unenforced — _PassAdapter.run() reads getattr(self._legacy, "stats", {}) and the message formatter assumes keys loops_unrolled, full_unrolls, partial_unrolls, partial_epilogues, instructions_before, instructions_after. If LoopUnroll doesn't populate .stats during .run(), the message silently degrades to "N change(s)" and stats is {}. Add an assertion or test that LoopUnroll.run() sets .stats with the expected schema.

🟡 Defaults sit exactly at the boundary — unroll_body_limit=64, unroll_max_factor=8, unroll_max_growth=512: a 64-instruction body × factor 8 = 512 growth, exactly allowed. Any margin error in instruction counting would flip eligibility. Consider adding a comment documenting the relationship (e.g., body_limit × max_factor ≤ max_growth) so future default changes don't accidentally decouple them.

💭 Docstring formatting — The new unroll_* entries use multi-line continuation indentation while all existing entries are single-line. Minor inconsistency; consider either one-line summaries or converting existing entries too.

💭 Message formatter is a bit long — The conditional message in _PassAdapter.run() is a 10-line f-string inside a method that's otherwise one-liners. If more passes need rich messages, consider extracting a format_message(stats) helper on the adapter or the legacy pass itself.

---

📁 scratchv/main.py

🔴 No input validation on numeric args — --unroll-factor, --unroll-full-threshold, --unroll-body-limit, --unroll-max-growth accept any int including 0 or negative values. Downstream unrolling logic almost certainly assumes positive values (division, loop trip counts). Add type=positive_int or post-parse validation:

def positive_int(v: str) -> int:
    n = int(v)
    if n <= 0:
        raise argparse.ArgumentTypeError(f"must be > 0, got {n}")
    return n

🟡 Missing invariant between thresholds — --unroll-full-threshold defaults to 8 and --unroll-factor also defaults to 8. If a user passes --unroll-full-threshold 16 --unroll-factor 4, full-unroll threshold exceeds the max partial factor — likely a logic bug or at minimum confusing. Add a check:

if args.unroll_full_threshold > args.unroll_factor:
    parser.error("--unroll-full-threshold must be <= --unroll-factor")

🟡 --unroll-epilogue lacks a --no-unroll-epilogue counterpart — Every other toggle has an explicit off-switch (--loop-unroll / --no-loop-unroll). For consistency and scriptability, add the mirror flag.

🟡 --loop-unroll help text leaks internal rationale — "while the greedy register allocator spills without reloading" is an implementation detail, not a user-facing description. Suggest: "Enable IR loop unrolling at --optimize all. Disabled by default."

💭 Config field name mismatch — CLI dest is unroll_factor but config field is unroll_max_factor. Works, but the mismatch is a maintenance trap. Consider renaming the CLI to --unroll-max-factor or the config to unroll_factor for consistency.

---

📁 scratchv/optimizer/__init__.py

🟡 Public API surface — UnrollPlan is exported in __all__. If it's an internal plan type (not meant for external consumers to construct or depend on), consider keeping it out of the public API. Exporting it implies a stability guarantee.

---

📁 scratchv/pass_interface.py

🟡 Line 50: Overly permissive value type — dict[str, Any] allows arbitrary objects, undermining type safety.

Suggestion: If the keys are metric names and values are numeric, use dict[str, float] or dict[str, int]. If you need mixed numeric types, consider dict[str, Union[int, float]]. Reserve Any for cases where you genuinely can't constrain the type.

💭 Line 44: "Optional" in docstring is misleading — The field defaults to an empty dict, so it's never None. "Optional" implies Optional[...] semantics, but callers can't distinguish "pass didn't provide stats" from "pass provided empty stats." If that distinction matters, use Optional[dict[...]] = None. If not, remove "Optional" from the docstring.

---

📁 tests/golden/013_for_sum.s.golden

🔴 Bug: Loop body is empty — code doesn't actually compute a sum — Lines 7–10: The loop increments t3 from 0 to 4 but never adds anything to t2. For a test named for_sum, the loop body should contain an accumulation (e.g., add t2, t2, <expr>). This golden file locks in broken behavior — future compiler regressions won't be caught because the expected output is wrong.

🔴 Wrong semantics for the sum result — Line 5: t2 = t0 + t1 is computed once before the loop. If the intent is sum = t0 + t1 after 4 iterations, the current code just returns that static value regardless of loop behavior. Verify the source IR and confirm what the expected sum actually is.

🟡 Label .entry is non-idiomatic — Line 5: An unnamed main: already serves as the entry. The extra .entry label adds noise and will confuse anyone comparing against other golden files that don't use this convention.

🟡 No .file or .section directives — Consider adding .file "013_for_sum.s" for symbolizer/debugger tooling consistency with other golden files in the suite.

💭 Comments could be more informative — # loop inc doesn't explain what's being incremented or why. For golden files that document expected behavior, # increment loop counter or similar would aid readability.

---

---

⚠️ 未审查的文件

• tests/golden/014_for_dot.s.golden
• tests/golden/019_nested_loop.s.golden
• tests/test_loop_unroll.py
• tests/test_loop_unroll_case_report.py