feat(scheduler): 使用 llvm-mca 评估指令调度并完善 CNN 验证

[Mastttttter]

依赖 #73，承接 #59，请先合并 #73。

Summary

• 使用 LLVM MCA 18.1.3 / SiFive E76 评估调度收益，移除自建周期模型。
• 完善调度校验、CNN A/B 评测、文档及 CI 接入。

Benchmark

benchmarks/bench_cnn_schedule.py 对仓库 models/graph/cnn.onnx 的 standalone 编译结果自动评测：

指标	调度前	调度后
局部完成周期合计	1116	927
最大并行指令数	2	2
零发射周期（气泡）	230	151
关键路径长度	N/A	N/A

模型周期减少 16.94%

Test plan

• 全量测试：1255 passed，无跳过。
• 三组完整 CNN QEMU 执行，调度前后状态一致。
• 物理寄存器数保持 30，无新增栈访存；CFG 活跃峰值 24→25。

[github-actions]

🤖 AI Code Review

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

📁 .github/workflows/ci.yml

Code Review: .github/workflows/ci.yml

---

🔴 Runner image ubuntu-24.04 may not exist on GitHub-hosted runners — Lines 17, 193.
If this image isn't yet available for self-hosted label resolution on GitHub, both jobs fail immediately with "Unable to find a runner". Verify the label is supported; if still on preview, prefer ubuntu-22.04 or keep ubuntu-latest.

---

🔴 Hard fail when models/graph/cnn.onnx is missing — Lines ~355-360.
Old logic auto-generated a minimal CNN; new logic test -f "$MODEL" aborts. If the .onnx file isn't committed to the repo (or was deleted), the entire benchmark job dies at that step. Confirm the file is tracked, or restore a fallback.

---

🟡 Subsequent steps don't stop on failure — CNN standalone step onward (~lines 360–375).
By default GitHub runs later steps even after a failure. If onnx_to_riscv_standalone fails, "Topic 18 CNN scheduling benefit" and "CNN assembly beautifier" will run against missing/stale artifacts, producing confusing errors. Add if: success() on dependent steps, or wrap them in a single run: block.

---

🟡 Missing set -euo pipefail — Topic 18 scheduling case report, synthetic benchmark, and CNN scheduling benefit steps (~lines 323, 332, 364).
These new run: blocks execute multi-line scripts without explicit fail-fast. A mid-pipeline failure (e.g., bench_inst_scheduler exits 0 but --json write fails) would be silently swallowed.

---

💭 Duplicated RISC-V tool setup — Appears in both test (line ~91) and benchmark (line ~267) jobs.
Consider extracting to a composite action (actions/composite) to avoid drift — they already differ subtly (benchmark version installs llvm-18 without specifying package pins).

---

📁 .gitignore

🔴 **Likely wrong pattern** — `/:memory:.ses` matches a literal file named `:memory:.ses` at the repo root. `:memory:` is an SQLite in-memory DB URI convention, not a real filename — did you capture this pattern correctly from the tool's actual output?

---

📁 CHANGELOG.md

Review

🟡 Breaking changes buried in prose — The SchedInst immutability change, build_dag single-region enforcement, and machine_instrs_from_scheduled lossy-conversion rejection are all breaking API changes. They should be in a dedicated ### Breaking subsection with a BREAKING CHANGE: prefix so consumers can grep for them.

🟡 "Future work" doesn't belong in a changelog — The last bullet (Structured post-RA/pre-emission scheduling and Fast/BURR strategy selection remain future work) documents what hasn't shipped. Changelogs record completed work; future plans belong in the roadmap or docs/topic-18/README.md (which you already link). Move it there and optionally link from the changelog.

🟡 Test methodology isn't user-facing — The bullet starting "Validate the tracked CNN through standalone scheduling A/B…" reads like a QA checklist, not a changelog entry. Either remove it or compress to a single factual statement like "Added scheduling A/B and full QEMU execution validation to CI."

🟡 Tooling-internal vs user-facing change boundary is blurry — "Remove the custom timing model, parameter table and extractor" and "restore section-stack state" are implementation details. Unless the removed components were public API, keep them brief or move to an ### Internal subsection.

💭 "Unreleased" lacks a target version — Other sections use ## [0.3.0]. Consider adding a provisional version tag (e.g., ## [0.4.0] — Unreleased) to set expectations about the next release cadence.

---

📁 benchmarks/audit_inst_scheduler.py

🟡 Import side-effect risk — If bench_cnn_schedule performs heavy work (e.g., loading models, running benchmarks) at module level, this shim pulls that in even when users only want to import the module. Consider lazy-importing main inside if __name__ == "__main__".

💭 raise SystemExit(main()) — Works fine if main() returns None/int, but a guard (if main() then raise) or a comment noting the expected return contract would make the intent clearer to future maintainers.

That's it — clean minimal shim, good docstring, correct __name__ guard.

---

📁 benchmarks/bench_cnn_schedule.py

🔴 Bug: KeyError not caught in candidate matching — audit_side_effects, line ~165:

candidate.append(available[after_lines[inst.id]].popleft())

If after_lines[inst.id] isn't a key in available, this raises KeyError, but only IndexError and ValueError are caught. Add KeyError to the except clause.

🔴 Bug: .pop("timing") without default — analyze_assembly, line ~222:

report.pop("timing")

If run_case ever omits the "timing" key (error path, different code version), this raises KeyError and crashes the entire benchmark instead of producing a failure report. Use report.pop("timing", None).

🟡 False-positive stack pointer classification — assembly_resources, line ~89:

if inst.operands and register_name(inst.operands[0]) == "x2" and not store:

not store is true for loads too. A load like ld x2, 0(x1) (unusual but valid) would be classified as a stack pointer write. Condition should be not store and not load to match the intent stated in the comment: "opaque instructions which name sp as their first operand."

🟡 Dead parameter — run_benchmark(execute=False) default is unreachable: main always passes execute=True with no CLI flag. Either add --no-execute to the parser or remove the parameter default so the contract is explicit.

🟡 Potential crash on empty region — audit_side_effects, line ~176:

live_out = set().union(*(inst.defines for inst in body))

If a region body is empty (e.g., a zero-instruction region with original_cycles set), set().union(*[]) raises TypeError. Guard with if not body: continue.

🟡 Python 3.9+ dependency — line ~373:

model_path.is_relative_to(ROOT)

Path.is_relative_to requires Python 3.9+. The from __future__ import annotations doesn't cover this. Add a version guard or use model_path.relative_to(ROOT) in a try/except, or document the minimum Python version.

💭 Serialization cost — assembly_resources returns stack_access_sequence and stack_pointer_operations as full lists of (opcode, tuple(operands)). For large programs these can be substantial JSON payloads. Consider truncating or omitting from the JSON output if only the comparison results matter.

💭 Long markdown function — ~120 lines mixing data extraction, formatting, and business logic. Hard to test independently. Consider extracting table-building helpers.

---

📁 benchmarks/bench_inst_scheduler.py

🔴 Dead-code with broken semantics: bench_build_dag — main() no longer calls it, but it still exists and constructs SchedInst without defines/uses. If those fields default to empty sets, build_dag produces a dependency-free DAG and the function's num_nodes result is meaningless. Either delete it or restore dependency info.

🟡 SchedInst(index, op, operands, raw_line=...) — positional construction is fragile. The old code used keywords (id=, opcode=, operands=, defines=, uses=). If field order ever changes, or defines/uses don't have defaults, this silently corrupts data. Prefer keyword args for anything beyond triviality.

🟡 Tight coupling to stats["regions"] shape — sum(row["status"] == "no_improvement" for row in stats["regions"]) will raise KeyError if the field is renamed or optional. applied_regions/skipped are already top-level; consider getting "unchanged" from the library too rather than re-deriving from the region list.

🟡 Benchmarked time no longer measures scheduling — schedule_assembly(source, config) runs each iteration with the same source string, so in-process caching (acknowledged in the docstring) means the measured mean_s reflects the cached/parse path, not real scheduling cost. The old code timed build_dag + schedule directly. If the intent is end-to-end latency this is fine, but rename the metric (e.g., pipeline_mean_s) so readers don't equate it with scheduler cost.

🟡 Hardcoded "课题 18" in _markdown — this title is wrong outside one specific assignment and mixes Chinese into an otherwise English-file repo. Pull from an argument or a constant at module top.

🟡 No --seed CLI flag — seed is fixed at 42 inside _gen_instructions and stamped into row["seed"] after the fact. If a user wants a different seed they must edit the source. Add a CLI arg and thread it through.

🟡 stats taken from the last iteration only — result = schedule_assembly(...) inside the loop, then stats = result.stats after. If schedule_assembly is even slightly nondeterministic (hash randomization, dict ordering affecting region selection), the reported cycle counts won't correspond to the timed runs. Capture stats from a dedicated call, or assert stability across repeats.

💭 # flake8: noqa removed — the file still has long f-string print headers and mixed formats; confirm flake8 is clean before dropping the suppression.

💭 argparse(description=__doc__) — __doc__ includes the "Run: python -m ..." usage line, which will duplicate the auto-generated usage in --help. Use a short summary string or strip the docstring.

💭 SchedInst field id → index — if this is an actual field rename in the class, the benchmark won't catch it at import time; a quick grep to confirm the constructor signature matches is worth it.

💭 N/A formatting — before = str(row["orig_cycles"]) if row["modeled"] else "N/A" mixes int and str then formats with :>8; works, but a small _fmt(v) helper would keep the print block readable as it grows.

---

📁 benchmarks/cases/inst_scheduler_feature.asm

🔴 Ambiguous semantics — The comment says "Move the independent addi ahead of the store wait" but the file shows addi after sw. Is this the pre-schedule input the scheduler should transform, or the post-schedule expected output? As written, a reader can't tell which. Either rename the file/label (e.g. scheduler_feature_input / ..._expected) or add a one-line header: # PRE-SCHEDULED: scheduler must hoist addi above sw.

🟡 Pipeline config is dangling — "LLVM MCA sifive-e76 7 -> 5" names a target but the pipeline spec isn't referenced. If the benchmark suite has a canonical pipeline definition (JSON / YAML / MCA script), link it. Otherwise "7 → 5" is unverifiable and future refactors will silently break it.

🟡 No assertion of the store effect — Expected output lists memory[a0+4]=16 but nothing in this file encodes it. If the runner does the check, that's fine — but a brief comment (# runner verifies mem[a0+4]==16) would make the contract explicit, since a store-only case would otherwise look like a load-only case to a future reader.

🟡 Hazard model not stated — The benchmark only makes sense if sw and addi contend for a resource (single ALU, or store-to-store forwarding, or a non-pipelined LSU). On a typical 5-stage RV32 with separate ALU/LSU, addi never waits on sw and the 7→5 claim is false. Add one line stating the assumed pipeline resource conflict, or the test is meaningless for any scheduler not matching that exact config.

🟡 add t1, t0, t2 is not independent of the load — It does have a RAW dep on lw t0. So only addi is truly movable. The comment says "the independent addi" which is correct, but worth noting the case only isolates one scheduling decision — make sure the harness doesn't conflate this with "stores are cheap."

💭 No .globl / .type on scheduler_feature — If the runner resolves the symbol by name via ELF, a plain local label may not be visible. Add .globl scheduler_feature + .type scheduler_feature, @function for portability.

💭 No return / register cleanup — Presumably the runner provides the wrapper, but a trailing ret (or a comment # runner supplies epilogue) makes the boundary clear.

💭 Formatting nit — sifive-e76 is not a standard MCA target name (typical: sifive-e76 or e76). Confirm the exact spelling against llvm-mca --version output to avoid copy-paste drift.

---

📁 benchmarks/cnn_schedule_execution.py

🔴 Bug: Assembly injection via unescaped paths — Line 82–83

binary.as_posix() and input_path.as_posix() are interpolated directly into .incbin "..." directives. If either path contains a " character, the assembly silently produces wrong .incbin operands (or a cryptic assembler error). Consider copying the source files to a sanitized temp name (e.g., roundtrip_fixed.bin) before embedding, or at minimum validate with a regex.

---

🟡 KeyError on missing metadata keys — Line 26–28

metadata["workspace_bytes"], metadata["output_elements"], metadata["input_elements"] are accessed without validation. A missing key produces a bare KeyError that doesn't tell the caller which benchmark config was malformed. Add a guard:

for key in ("workspace_bytes", "output_elements", "input_elements"):
    if key not in metadata:
        raise ValueError(f"metadata missing required key: {key!r}")

---

🟡 splitlines()[0] IndexError risk — Line 116–117

If any tool's --version prints empty output (e.g., a wrapper script or unusual build), .splitlines()[0] raises IndexError. Prefer:

ver = subprocess.check_output([exe, "--version"], text=True, stderr=subprocess.DEVNULL).strip()
tool_versions[name] = ver.splitlines()[0] if ver else "<unknown>"

---

🟡 Subprocess failures omit stderr from error messages — Lines 17–18, 104–105

check=True with capture_output=True swallows the tool's stderr into CalledProcessError.stderr, but it's never surfaced. When clang or lld fails, the user gets a bare CalledProcessError with no diagnostic. Wrap or augment:

try:
    subprocess.run(..., capture_output=True, check=True, timeout=30)
except subprocess.CalledProcessError as e:
    raise RuntimeError(f"{e.cmd[0]} failed: {e.stderr.decode(errors='replace')}")

---

💭 total and stores recomputed every phase iteration — Line 54–55

These values are identical for both phases. Computing them once before the for phase, binary loop would make the intent clearer.

💭 Storing x0 — Line 54

sw x0, -128(sp) is a no-op (x0 is hardwired to zero). Consider starting the range at 1 to save one instruction, though the impact is negligible in a benchmark harness.

---

📁 benchmarks/run_inst_scheduler_case.py

Code Review: benchmarks/run_inst_scheduler_case.py

---

🟡 Private API coupling — baseline._run_asm_passes(source, []) / driver._run_asm_passes(...): These are private methods. The cross-check after != direct.asm_text mitigates drift, but a future refactor could silently break this harness with no compile-time warning. Consider adding a # pragma: allow-secret comment or a thin public wrapper.

🟡 warnings only reflects last iteration — The warnings list is reinitialized inside the timing loop, so the report only captures warnings from the final run. If earlier iterations produce different warnings (e.g., cache cold-start), they're silently dropped. Accumulate or assert stability:

all_warnings = []
for _ in range(repeats):
    stats, w = {}, []
    after = driver._run_asm_passes(source, w, stats)
    all_warnings.extend(w)

🟡 Exact-string comparison for assembly equality — before != source, before != after, after != direct.asm_text all use byte-exact string comparison. If _run_asm_passes normalizes trailing whitespace/newlines on one path but not another, these checks produce false positives. Consider .rstrip() normalization, or at least document that the contract is byte-exact.

🟡 Bare except Exception in main obscures root cause — The catch-all writes a report but prints the same markdown to stdout. When the harness fails during development, the traceback is lost. Consider except Exception as exc: traceback.print_exc() before building the error report, so interactive runs still surface the stack.

💭 Redundant encode-decode cycle — source = case_path.read_bytes().decode("utf-8") followed by hashlib.sha256(source.encode("utf-8")). Hash the raw bytes directly:

raw = case_path.read_bytes()
source = raw.decode("utf-8")
# later: hashlib.sha256(raw).hexdigest()

💭 repeats=1 stdev is silently 0 — The guard if repeats > 1 else 0 is correct but could confuse a reader who expects NaN or a field omitted. A one-line comment explaining the choice would help.

---

Overall the validation in _execute is thorough and defense-in-depth is well-designed — the directive whitelist, straight-line check, memory bounds, strict=True simulator run, and post-run PC/instr_count verification form a solid safety net. The _markdown report is detailed without being bloated.

---

📁 benchmarks/schedule_analysis.py

Code Review

🟡 Silent target drop — Line 40: if target < len(insts) silently ignores branch targets pointing past the last instruction. A label placed after all instructions gets index == len(insts) and is assigned as a valid target, but then dropped here. This can produce incorrect liveness results instead of flagging a malformed CFG.

Suggestion: Replace with:

if target >= len(insts):
    return {"status": "not_modeled", "reason": f"branch target out of range: {inst.effects.target}"}

🟡 Boolean |= produces int — Line 47: changed |= entry != live_in[i] or .... Since bool subclasses int, after the first iteration changed becomes int(1)/int(0) rather than bool. The while changed loop still works, but this violates the declared bool type and could surprise future readers or type checkers.

Suggestion:

if entry != live_in[i] or out != live_out[i]:
    changed = True

🟡 Fixpoint loop has no iteration cap — Lines 44–49: Convergence is theoretically guaranteed (finite lattice, monotone), but a defensive cap (e.g., while changed and iterations <= len(insts) + 1) prevents infinite loops from coding errors in successor construction.

💭 Variable shadowing — index is used for label positioning (line 15–23) and then reused in for index, inst in enumerate(insts) (line 31). Consider renaming the first to label_idx for clarity.

💭 Minor: set().union(*(live_in[j] for j in successors[i])) could be set().union(*(live_in[j] for j in successors[i])) → a simple loop or frozenset().union(...) would be more explicit, but this is purely stylistic.

---

---

⚠️ 未审查的文件

• docs/topic-18/18-指令调度器Review迭代报告.md
• docs/topic-18/18-指令调度器SPEC-Review.md
• docs/topic-18/18-指令调度器代码说明.md
• docs/topic-18/18-指令调度器设计文档.md
• docs/topic-18/README.md
• docs/topic-18/cnn-scheduling-results.json
• docs/topic-18/todolist.md
• scratchv/backend/init.py
• scratchv/backend/inst_scheduler.py
• scratchv/backend/instruction_select.py
• scratchv/backend/llvm_mca.py
• scratchv/backend/machine_types.py
• scratchv/backend/regalloc_linear.py
• scratchv/backend/schedule_semantics.py
• scratchv/backend/schedule_verify.py
• scratchv/compiler.py
• scratchv/main.py
• scratchv/standalone/onnx_to_riscv_standalone.py
• scratchv_dag/selection_dag.py
• tests/test_inst_scheduler.py