feat: unify CFG infrastructure for IR and Machine IRCodex/cfg docs

[muykokoro]

• 统一 scratchv/analysis/cfg.py 与 scratchv/ir/cfg.py 的 CFG 核心
• 新增 IR/Machine CFG adapter
• 新增 liveness、dataflow、constant propagation 和 CFG validation
• MachineInstr 增加结构化 target 字段并同步后端使用
• 378 tests passed

[github-actions]

🤖 AI Code Review

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

📁 benchmarks/test_regalloc/bench_cnn.py

Looks good overall — the target or comment fallback is applied consistently across all 6 branch/jump sites and the labels dict construction matches. One minor concern:

💭 Nit: or semantics with empty strings — If instruction.target can be "" (empty string) rather than None, the or would silently fall through to comment. If that's possible, consider instruction.target if instruction.target is not None else instruction.comment for explicit None-checking. If target is guaranteed to be None or a non-empty string, current code is fine.

Otherwise, clean and well-scoped change.

---

📁 docs/CFG_Design.md

Review: docs/CFG_Design.md (v1.0 → v2.0)

🔴 Bug: CFGNode.instructions 默认值类型矛盾 — §3.2:

instructions: Any = 0          # builder 产物为 list[Instruction|MachineInstr]

默认值 0 是 int，但字段语义是指令列表。下游消费者（liveness、usedef、DOT）会对 instructions 做迭代/索引。建议默认 field(default_factory=list)，或者如果确实需要计数兼容，用 list[Instruction] | int 联合类型并加 guard。

🔴 Bug: NaturalLoop 类型凭空消失 — 旧版 §3.5 定义了完整 dataclass，新版全文无此类型。但 §4.5 仍在描述循环检测算法（回边、body、嵌套、parent/children/nesting_depth）。文档声称"已实现"，但核心产出类型未定义。需要补充 dataclass 或引用实际模块中的定义。

🟡 支配集公式对无前驱节点未定义 — §4.4:

Dom(n) = {n} ∪ ⋂{Dom(p) | p ∈ predecessors(n)}

当 n ≠ entry 且 predecessors(n) 为空（不可达块）时，交集为空集，结果变成 Dom(n) = {n}。这不正确——不可达块的支配集应特殊处理。建议明确说明不可达块如何处理（跳过？设为全集？依赖 unreachable 检测先剔除？）。

🟡 FOR/ENDFOR 规范化的责任边界不明确 — §4.3 展示了 desugar 结果但未说明执行时机和所有者。是 IRCFGAdapter.blocks() 在调用时 desugar？还是 builder 内部？还是 adapter 的构造函数？这直接影响调用链理解和测试边界。

🟡 eliminate_unreachable 接口去向后无替代说明 — 旧版 §5 接口契约包含此函数，新版仅在核心方法中提到 unreachable_blocks（返回集合）。返回集合 vs 原地删除是不同语义。需要说明下游消费者如何消费 unreachable_blocks 结果。

🟡 旧版审查清单（§8）被移除 — 该表对 reviewer 有直接价值（"验证方法"列指定了具体的验证动作）。新版"验收标准"是项目级 checklist，不覆盖审查者逐条验证的需求。建议保留或合并。

💭 Adapter Protocol 类型过松 — §4.1 所有方法参数均为 Any，Protocol 的静态检查价值近乎为零。建议至少用 Protocol[InstrT, BlockT] 泛型参数化，或在文档中约束实际实现类型。

💭 Shim 文件无退役时间线 — §2.2 列出 scratchv/ir/cfg.py 和 scratchv/analysis/cfg_builder.py 为"旧路径兼容 shim"，但未说明 deprecation 计划。作为已实现版本的文档，建议标注预期移除版本或 issue link。

💭 可变性语义回归缺失 — 旧版已知限制中明确指出"instructions 是引用非拷贝，外部修改原 IR 会影响 CFG"。新版 Adapter 模式引入间接引用后，此问题可能更严重（adapter 可延迟读取）。建议在新版"已知限制"中恢复此条目。

---

📁 docs/CFG_Dev.md

🔴 Missing eliminate_unreachable — 旧 Module Map 第 13 行明确列出 eliminate_unreachable（原地修改 DFS 消除不可达块）。新版 §2.1 完全消失，也没有注明移除或迁移到何处。如有调用方依赖该 API，静默删除是破坏性变更。

🟡 CFGNode.instructions 语义变更未显式标注 — 旧版："instructions 是对原 IR 列表子序列的引用（非拷贝）"；新版 §3.1："instructions 是 builder 新建列表，元素为原指令对象；不直接引用输入块列表。" 这是 breaking change：旧代码若在 CFG 构建后修改原 IR 列表会影响 CFG，新代码不会。建议在 §3.1 不变量旁加一条 changelog 备注或迁移说明。

🟡 CFGAdapter 协议无形式定义 — §3.1 的 build_cfg(adapter) 和 §2.2 的 IRCFGAdapter/MachineCFGAdapter 暗示 adapter 是核心扩展点，但全文未给出 CFGAdapter protocol 的方法签名（get_instructions()? get_blocks()? get_edge_meta()?）。新 contributor 无法知道需要实现什么。建议在 §2.2 上方或 §3 开头加一个 class CFGAdapter(Protocol) 的签名片段。

🟡 操作级陷阱丢失 — 旧版 "常见陷阱" 包含 "LABEL 被当成普通指令加入块"、"DOT 引号未转义"、"visited 用 list 而非 set" 等实操问题。新版替换为架构级陷阱（adapter 规范化、CALL terminator 等），但上述操作性问题在 IR adapter 和 DOT 输出中依然适用。建议保留为子表或脚注，避免新开发者重复踩坑。

🟡 §3.3 analyze_liveness 中 live_before / live_after 定义不够精确 — "先定义后使用不加入 uses[B]" 只描述了 phi 函数规则，但未说明 live_before 是 uses[B] ∪ (live_out[B] - defs[B]) 的哪一侧。测试断言（§5 "liveness live_in/live_out/live_before/live_after 精确集合"）依赖这四个概念的精确定义。建议在 §3.3 加一行形式化定义：live_before[v] = (use_before[inst]) ∪ (live_out[B] \ defs_before)。

💭 BOM 移除 — 第一行 -# → +# 正确，跨平台编辑器和 pytest 不会因 BOM 报错。

💭 §2.3 分析模块的模块名列表 — liveness.py、dataflow.py、usedef.py、cfg_validation.py 均为新文件。如果这些尚未创建或还在 review 中，"状态: 已实现"（第 4 行）可能超前。建议确认所有引用文件均已合入后再标记已实现。

---

📁 pyproject.toml

💭 建议加 --strict-markers — 没有此标志，注册 marker 只是声明性的，使用未注册 marker 仅产生 warning 而非 error，注册的意义大打折扣。考虑在 [tool.pytest.ini_options] 下添加：

addopts = "--strict-markers"

---

📁 scratchv/analysis/__init__.py

🔴 Breaks backward-compatible import path — Line 3: imports now come from scratchv.analysis.cfg, but old code doing from scratchv.analysis.cfg_builder import CFGBuilder will break.
Suggestion: Either keep cfg_builder.py as a deprecated shim that re-exports from the new location, or confirm no consumers use the direct submodule path.

🟡 God-module __init__.py — 7 submodules flattened into one __all__ with 35+ symbols. This couples every consumer to every analysis pass, forces eager loading of all dependencies, and makes the public API boundary ambiguous.
Suggestion: Keep __init__.py thin — export only the top-level entry points (e.g., CFGBuilder, IRVerifier, analyze_liveness, run_dataflow). Let consumers import submodules directly: from scratchv.analysis.liveness import analyze_liveness.

🟡 Mixes abstraction levels in __all__ — Internal types (BlockId, InstructionId, ValueId, Direction, OVERDEFINED) are exported alongside user-facing APIs. Consumers can't distinguish stable public API from implementation details that may change.
Suggestion: Only export what consumers actually need to call or subclass. Internal ID types and constants should stay private to their submodules.

🟡 No lazy imports — All submodules (cfg, adapters, liveness, dataflow, usedef, cfg_validation, ir_verifier) are loaded on import scratchv.analysis even if only one is needed. For a compiler analysis framework, the transitive import cost can be significant.
Suggestion: Use __getattr__ (PEP 562) for lazy module loading:

def __getattr__(name):
    if name in _lazy_exports:
        mod = importlib.import_module(_lazy_exports[name])
        return getattr(mod, name)
    raise AttributeError(name)

🟡 Missing CFGBuilder deprecation signaling — CFGBuilder is retained for compatibility but there's no deprecation warning or docstring noting that build_cfg is the preferred replacement. Silent coexistence of old and new APIs causes confusion.
Suggestion: Add a warnings.warn deprecation in CFGBuilder.__init__ or at least document the preferred path.

💭 CFG is both a class and conceptually a protocol — CFG appears in __all__ but consumers importing from cfg also get ControlFlowGraph. The relationship between CFG and ControlFlowGraph is unclear from the public API. Consider aliasing explicitly or dropping one.

---

📁 scratchv/analysis/adapters.py

🔴 Bug: Reverse FOR loops use wrong comparison op — _normalize_for_endfor always emits cmp_op=">=" regardless of step sign. A loop like for i = 10 downto 0 step -1 would immediately exit. Compare step < 0 and flip to < (or the IR's equivalent).

🔴 Bug: Synthetic names can collide with user identifiers — for_hdr{n}, for_body{n}, for_exit{n}, for_end_{n}, for_step_{n} are minted without any uniqueness check. If the source IR already contains a label/value with one of those names, CFG edges silently cross-link and use-def analysis corrupts. Namespace them with a function-scoped counter or use a reserved prefix the IR parser rejects.

🔴 Bug: _br_if encodes targets as a comma-joined string — target=f"{true_target},{false_target}" then re-split in branch_targets. Any label containing , breaks parsing; also there's no way for branch_targets to distinguish "true target is empty" from "no target". Consider attrs={"true_target":..., "false_target":...} or a tuple.

🟡 Interface inconsistency: IRCFGAdapter lacks clobbers — Machine adapter exposes it, IR doesn't. If CFGAdapter protocol requires it, IR adapter will AttributeError; if it's optional, document that. Align the two adapters or make it optional explicitly.

🟡 Dead branch in IRCFGAdapter.has_fallthrough — Since _br_if always emits 2 targets, len(branch_targets(instr)) == 1 is unreachable. Either remove the branch or document that IR BR_IF may legitimately have a single target with implicit fallthrough (and add a test).

🟡 Missing return annotations on uses, defs, edge_uses, phi_defs, clobbers across both adapters. Under from __future__ import annotations these fall back to Any, hiding real bugs in the use-def contract.

🟡 _strip_function_label heuristic is fragile — Drops exactly one leading non-. LABEL. Breaks if the selector emits multiple function labels, or if a legitimate block label doesn't start with . (project convention isn't declared anywhere in this file). Prefer an explicit marker (e.g., Function carries its first block name, or the selector tags function labels).

🟡 branch_targets falls back to get_machine_semantics(...).target_from_comment — LABEL parsing elsewhere uses .target or .comment. Two different conventions for the same field across one file is a maintenance trap; unify.

🟡 end_value / step_value shadow user-visible values — _const_int(f"for_end_{n}", end) creates a new Value even when the FOR's attrs["end"] was originally a real SSA value. Use-def on the original SSA value won't see this redefinition, breaking later passes that expect SSA uniqueness.

💭 Nit: loop_stack: list[dict[str, Any]] — a small NamedTuple or dataclass (iv, step, header, exit_label) would make the FOR/ENDFOR matching self-documenting and let the linter catch field typos.

💭 Nit: Empty-block preservation in _partition_*_stream is called out in the docstring but no test exercises it — worth one assertion.

---

📁 scratchv/analysis/cfg.py

Code Review: scratchv/analysis/cfg.py

🔴 Bug: fallthrough condition collision — In build_cfg (~L270): when a BRANCH terminator has targets AND fallthrough, both target index 1 and the fallthrough edge get condition="false". A custom adapter can produce targets=["L1","L2"] with has_fallthrough=True, yielding two ambiguous "false" edges from the same source. Suggest: offset fallthrough condition past existing targets, e.g. f"fallthrough_{target_count}".

🔴 Bug: detect_nested_loops parent is order-dependent — inner.parent = outer.header is overwritten by whichever outer loop is iterated last. With loops A ⊃ B ⊃ C, if A is processed after B in the outer iteration, C.parent becomes A instead of B. Fix: iterate outer loops in decreasing body size, or compute the closest containing loop (smallest superset body).

🟡 Contract violation: CFGBuilder.eliminate_unreachable mutates CFG — The class docstring says "Analyses are intentionally read-only." But this method deletes nodes and filters edges in-place. Either fix the docstring or rename to _eliminate_unreachable_inplace and warn callers.

🟡 Security: incomplete DOT escaping in _dot_escape — Missing |, {, }, ;, <, >. A block named foo;} would break DOT parsing and could inject graphviz directives. At minimum escape these characters.

🟡 Performance: successors/predecessors are O(E) — Linear scan of self.edges on every call. compute_dominators calls predecessors inside a fixed-point loop, making the total complexity O(V²·E). Consider maintaining adjacency lists built once.

🟡 Fragile CFGNode.instructions: Any = 0 — Typed as Any with default 0 (int), but always a list in practice. The scattered isinstance(node.instructions, int) checks are a maintenance trap. Either add a @property that returns [] when instructions is int, or use a union type list[Any] | int.

🟡 partition_basic_blocks_with_names can emit duplicate names — current_name = getattr(instr, "target", None) or f"L_{auto_id}" never increments auto_id when a LABEL lacks a target. Two consecutive label-less LABELs produce ("L_0", ...), ("L_0", ...). Increment auto_id in that branch.

💭 Missing newline at end of file — POSIX requires it; some linters/git-diffs flag this.

💭 Deferred import in CFGBuilder.build — from scratchv.analysis.adapters import IRCFGAdapter inside the method works but signals a circular-dependency smell. Consider extracting IRCFGAdapter to a shared types module if the circular import persists.

---

📁 scratchv/analysis/cfg_builder.py

Review: cfg_builder.py — Shim Replacement

🟡 No deprecation warning — Consumers importing from this legacy path get no signal to migrate. A shim without a warning defeats its own purpose.

Suggestion: Add a PendingDeprecationWarning on module import:

import warnings
warnings.warn(
    "scratchv.analysis.cfg_builder is deprecated; use scratchv.analysis.cfg",
    PendingDeprecationWarning,
    stacklevel=2,
)

🟡 Original CFGBuilder had instance methods now exposed as module-level functions — compute_dominators, compute_dominator_tree, detect_loops, detect_nested_loops were originally called as builder.detect_loops(cfg). The new standalone functions take (cfg) as first arg, but callers using CFGBuilder still call them as methods. Verify the new module's CFGBuilder retains these as methods with matching signatures — if not, this is a silent break.

🟡 Extra symbols leaked into public API — BlockId, InstructionId, ValueId, partition_basic_blocks_with_names, build_cfg_from_instructions, CFGAdapter, verify_cfg were never exported from cfg_builder.py. Now from cfg_builder import * pulls them in. This changes the module's API surface beyond backward compat.

Suggestion: Either split the shim into a __all__ matching only the original exports, or accept the expanded API explicitly in a changelog.

🟡 CFG vs ControlFlowGraph — Both exported. Are these aliases? If so, document which is canonical and mark the other deprecated. If not aliases, the name collision is confusing.

💭 Missing newline at end of file — Trailing \ No newline at end of file. Trivial but trips git diff/linters.

---

📁 scratchv/analysis/cfg_validation.py

🔴 Bug: likely false positive in _check_control_edges — Lines 130-145: flagging JUMP + FALLTHROUGH for a block catches every block containing a conditional branch (BEQ/BNE/BLT/…), because a conditional branch legitimately produces both a taken-edge and a fallthrough-edge. Verify whether EdgeType.JUMP distinguishes unconditional from conditional jumps. If it doesn't, this check is wrong and will produce spurious errors on every non-trivial CFG.

🔴 Bug: silent suppression in _check_terminators — Lines 103-106: instructions = [] if isinstance(node.instructions, int) else list(node.instructions). Treating int as "no instructions" silently disables terminator validation for those nodes. If int is a placeholder for a missing/unresolved block, this hides real defects. Either raise or report a distinct diagnostic instead of no-oping.

🔴 Blocker: dedup key includes free-text message — Lines 175-179: key = (item.code, item.message, item.block, item.edge). Two diagnostics with identical structural identity but slightly different wording would both survive; conversely, two distinct structural issues that happen to share wording could be deduplicated incorrectly. Use (code, block, edge) only.

🟡 Suggestion: redundant check — _check_predecessor_consistency (lines 147-170) re-validates endpoints already checked by _check_edges. Either drop it or document that predecessors()/successors() are independently maintained and can drift from edges.

🟡 Suggestion: incomplete terminator sets — _MACHINE_TERMINATORS is missing BEQZ, BGTZ, BLE, BLEZ, BGT, JALR variants, and _IR_TERMINATORS lacks conditional-return/switch-style terminators. If the IR or ISA extends, this list silently drifts. Consider deriving from a single source of truth.

🟡 Suggestion: no tests — For a module whose whole purpose is producing stable diagnostic codes, there should be table-driven tests covering: empty CFG, missing entry, dangling source, dangling target, terminator-not-last, jump+fallthrough, predecessor drift, and the dedup path. Without them the stable-code contract is unverifiable.

🟡 Suggestion: unused import — Sequence (line 10) is imported but never referenced.

💭 Nit: no newline at end of file — Line 205: add trailing newline.

💭 Nit: fragile attribute probing — _opcode_name (lines 22-29) tries opcode then op, returning str(opcode) as a last resort. str(...) for an enum often yields ClassName.NAME rather than the bare name. Prefer an explicit interface or enum.Enum membership check.

---

📁 scratchv/analysis/ir_verifier.py

🟡 Suggestions

1. Broad except Exception masks adapter bugs — _check_unified_cfg: catching everything converts genuine programming errors in IRCFGAdapter / build_cfg (AttributeError, TypeError, KeyError) into "invalid IR" errors, which a user cannot fix. Real adapter regressions would silently surface as IR diagnostics. Narrow to the exceptions the adapter contractually raises (e.g. CFGBuildError, ValueError), or log traceback alongside the surfaced message so hidden bugs don't rot.

2. Non-error diagnostics are dropped unilaterally — if diagnostic.severity != "error": continue means verify_cfg warnings never reach the IR path. This partially reintroduces exactly the drift the docstring warns against: Machine IR and IR would then disagree on warning-level structural issues. Either forward warnings (if ErrorLevel supports it), or add a test asserting no IR-relevant warnings exist in verify_cfg so the filter can't silently rot.

3. Confirm this is truly the single CFG build site — the "unified" name and the "do not drift apart" comment imply CFG construction lives here. Verify _check_ssa_validity, Check 7 (entry block), and any other pass don't independently reconstruct the CFG; otherwise "unified" is aspirational and the stated drift risk persists. A test asserting one build_cfg call per function would lock this in.

4. Redundant construction cost — build_cfg(IRCFGAdapter(func)) runs per _verify_function invocation. If other checks also build a CFG, consider constructing once in _verify_function and passing the CFG to consumers rather than re-building inside _check_unified_cfg.

💭 Nits

5. Rule naming inconsistent: "cfg-build" vs f"cfg:{diagnostic.code}". Pick one convention (suggest cfg:* throughout) for log filtering.
6. Comment numbering gap: neighbors read # Check 6 / # Check 7; this block has no number. Add # Check 6.5 to keep the sequence readable.
7. Docstring says "delegated to verify_cfg" but the method also performs adapter construction and build_cfg — it delegates validation, not construction. Minor wording fix.

🔴 Blockers: none. No injection, data-loss, race, or broken-contract issues in this diff as shown.

---

---

⚠️ 未审查的文件

• scratchv/analysis/liveness.py
• scratchv/analysis/usedef.py
• scratchv/backend/asm_emit.py
• scratchv/backend/inst_scheduler.py
• scratchv/backend/inst_select_ext.py
• scratchv/backend/instruction_select.py
• scratchv/backend/machine_semantics.py
• scratchv/backend/machine_types.py
• scratchv/backend/regalloc_cfg.py
• scratchv/backend/regalloc_linear.py
• scratchv/backend/regalloc_linear_v1_5.py
• scratchv/backend/register_alloc.py
• scratchv/ir/cfg.py
• tests/test_unified_cfg.py

[yuki-328]

基于 Issue #58 的验收项复查，PR #71 仍建议继续完善：

1. 两个 linear-scan 分配器仍依赖 scratchv/backend/regalloc_cfg.py，没有消费新的统一 CFG/liveness；需要迁移后将旧实现删除或降为兼容层。
2. MachineUseDefProvider 应统一复用 machine_semantics.py。当前 BNEZ(cond) 得到 uses=[]（应包含 cond），SW(value, addr) 只识别 addr（应包含 value 和 addr）；CALL 的 implicit uses/defs/clobbers 也应来自中央语义表。
3. dataflow.py 的 forward solver 在首次输入等于 initial 时会跳过 transfer。最小跨块用例 entry -> b1(x=7) 会得到 out_values["b1"] == {}；应保证每个可达块至少执行一次 transfer，并让 liveness 真正复用通用 worklist，或明确不能复用的契约。
4. 补充跨块 Machine liveness、diamond/join、循环/多回边、Phi edge、CALL live-after、两种分配器接入统一 CFG，以及 spill/reload 后模拟器执行测试。

现有相关测试 214 项通过，但没有覆盖上述问题，因此暂不建议按 Issue #58 完成验收。

[yuki-328]

上次反馈的 Machine uses/defs、跨块 dataflow 和 linear-scan 接入问题已基本修复，相关测试也已补充。

当前建议合并前继续完成一项核心内容：

1. 修复通用 backward dataflow 在 CFG 没有出口块时不执行的问题。当前 worklist 只从无 successor 的块初始化，纯循环 CFG 会直接返回初始结果，需要确保所有块至少被处理一次并补充测试。

完成这一项后，核心功能可以认为达到可合并状态。

若要严格完成 Issue #58 的全部验收标准，还需要二选一处理 liveness 的 worklist 复用问题：要么抽取支持 edge transfer/Phi 的公共 worklist，要么在 Issue 中明确 Phi-aware liveness 使用独立求解器属于允许的设计。

控制流语义仍存在少量重复硬编码，CFG 校验也尚未覆盖唯一 entry、指令唯一归属和 Phi predecessor 一致性。这些可以拆成后续 Issue，但建议在当前 PR 中明确记录，暂时不要将 Issue #58 标记为全部完成。

[yuki-328]

greedy RegisterAllocator 接入统一 CFG/liveness 的修改已提交为补充 PR：muykokoro#1。

包含基本块边界、call clobber 和寄存器淘汰的 live_after 接入，以及对应的集成测试与 TinyFive 模拟器执行测试。验证结果：48 passed；相关 DSL/backend/CNN/metrics 回归为 60 passed, 4 skipped。合并该补充 PR 后，修改会直接进入本 PR 的 codex/cfg-docs 分支。