Skip to content

feat(backend): 完善课题 18 安全指令调度与 A/B Benchmark 报告 - #59

Open
Mastttttter wants to merge 7 commits into
ScratchV-Compiler:mainfrom
Mastttttter:main
Open

Mastttttter wants to merge 7 commits into
ScratchV-Compiler:mainfrom
Mastttttter:main

Conversation

@Mastttttter

@Mastttttter Mastttttter commented Sep 14, 2026

Copy link
Copy Markdown

概述

完善课题 18 的局部指令调度器,修复汇编信息丢失、依赖遗漏和周期估算无法反映调度收益的问题。补充调度校验、编译器集成、实际执行验证及 CI Benchmark 报告。

主要改动

  • 复用共享汇编解析器,保留标签、指示行、注释和换行格式,限制指令移动范围。
  • 新增指令语义模块,统一整数及浮点寄存器别名,补齐 RAW、WAR、WAW 依赖,保持访存和浮点状态副作用的顺序。
  • 修正关键路径优先级计算,结合结果就绪时间和功能单元占用安排指令。
  • 新增独立校验器,检查指令完整性、寄存器读写来源和依赖顺序。校验失败时恢复原序;严格模式下终止编译。
  • 接入现有 --schedule 开关,新增 --schedule-strict、--schedule-report,支持结构化统计和 JSON 报告。
  • 将课题文档整理至 docs/topic-18/,纳入设计、实现计划、代码说明、SPEC Review 和 Benchmark 使用说明。

Benchmark 报告

参照 PR #35 的同输入 A/B 报告方式,将固定功能用例、实际编译输出和合成规模测试分别展示。

固定用例与真实执行

同一份汇编分别通过 CompilerDriver 的调度关闭和开启配置,再将前后汇编编码并使用真实 TinyFive 执行。

指标 调度前 调度后
━━━━━━━━━━━━━━━━━━━━━━━━━ ━━━━━━━━ ━━━━━━━━
源汇编指令数 4 4
───────────────────────── ──────── ────────
局部模型周期(估算) 5 4
───────────────────────── ──────── ────────
局部模型停顿(估算) 1 0
───────────────────────── ──────── ────────
编码机器指令数 4 4
───────────────────────── ──────── ────────
代码大小(字节) 16 16
───────────────────────── ──────── ────────
TinyFive 实际执行指令数 4 4

  • compiler_config_schedule=true
  • pipeline_matches_public_pass=true
  • 实际移动 2 条指令
  • backend=tinyfive、fallback=false
  • 全部 32 个整数寄存器及数据区 16 个字的结果一致
  • 模拟器缺失、执行失败或结果不一致时生成 FAIL 报告,并返回非零退出码

模型周期下降表示调度填补了 load-use 等待,不代表硬件实测加速。

CNN 汇编静态 A/B

对现有 CNN 编译步骤产生的同一份汇编切换调度开关,保留零收益及跳过结果。

当前 CNN 列表包含数字分支偏移,触发调度器的整份输入保留规则:

  • 源指令数:876 → 876
  • 比较状态:not_modeled
  • 模型覆盖率:0%,周期显示为 N/A
  • 执行状态:not_run

该结果用于展示当前支持范围,不声称已经完成 CNN 端到端执行验证或取得性能提升。

合成规模测试与 CI

  • 固定随机种子,覆盖 10~5000 条指令。
  • 记录输入哈希、模型周期、移动指令数、区域状态及优化器耗时。
  • 1000 条指令样例的模型估算从 2232 周期降至 1802 周期。
  • 默认跳过超过 1024 条指令的区域,未建模结果显示 N/A。
  • 三组报告均输出 JSON 和 Markdown,由 benchmark-reports artifact 收集,并写入 GitHub Actions Job Summary。

本地验证

  • 全量测试:867 passed,11 条未注册 pytest 标记警告。
  • 调度执行对比覆盖整数、浮点、访存、分支、调用、循环及随机指令块。
  • 固定用例 TinyFive A/B 报告通过,寄存器和内存结果一致。
  • CI YAML 和相关 Shell 步骤语法检查通过。

当前范围

调度在寄存器分配后的汇编阶段进行,仅在合法局部区域内移动指令。候选顺序必须通过校验,且在同一模型下严格减少周期才会被采用。

周期统计是局部静态估算;优化器耗时是主机执行调度 pass 的时间,两者均不作为目标程序硬件运行时间。

@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown

🤖 AI Code Review

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

📁 .github/workflows/ci.yml

🟡 Missing tool verificationllvm-mca-18 is used in the audit step but not checked with command -v like clang, ld.lld, qemu-riscv32. If the package install fails partially, you'll get a confusing runtime error from the Python module instead of a clear diagnostic. Add command -v llvm-mca-18 to the verification block.

💭 No apt cachingapt-get update + install of clang-18/lld-18/llvm-18/qemu-user runs every invocation. Consider actions/cache for /var/cache/apt + /var/lib/apt/lists to cut ~30-60s off each run, or use a self-hosted runner with these pre-installed.

💭 if: always() artifact upload with potentially empty directory — If the test step fails before mkdir -p benchmark_reports, the upload will create an empty artifact. Consider guarding with a file existence check:

with:
  path: |
    benchmark_reports/*.xml
    benchmark_reports/*.json
    benchmark_reports/*.md

Or add if: always() && hashFiles('benchmark_reports/**') to skip when there's nothing to upload.


📁 .gitignore

🔴 Pattern likely unmatchable on Windows:memory:.ses contains colons, which are invalid in Windows filenames. If the source tool writes this on Windows, it will either fail or write to a different filename (e.g. escaped), making this gitignore entry silently ineffective. Confirm the actual filename the tool produces across OSes — if it varies, you need a broader pattern.

🟡 Root-anchored may be too narrow/:memory:.ses only matches at repo root. If the tool can be invoked from a subdirectory (very common for CLIs — sqlite3 :memory: style), the file lands elsewhere and gets committed. Consider :memory:.ses without the leading /, or **/:memory:.ses, unless you've verified the tool always writes to the repo root.

🟡 Vague comment# Local tool session state doesn't identify which tool. Six months from now, nobody will remember whether this is from VS Code, a CLI, or an extension. Suggest: # VS Code (or <tool>): in-memory session state.

💭 No wildcard for the .ses extension — if the tool ever produces variant names (:memory:2.ses, :memory:foo.ses), this won't catch them. A :memory:*.ses pattern is more robust if the naming isn't strictly fixed. Verify by checking the tool's source/docs.

🟢 Good: blank line separator before the new block is consistent with the file's existing style; the entry is scoped to a specific file rather than blanket-ignoring a whole extension, which avoids hiding real artifacts.


📁 CHANGELOG.md

🟡 Breaking changes lack subsection — API migration items (immutable SchedInst, build_dag rejection, lossy conversion rejection) are breaking. Consider a ### Breaking Changes sub-header under Unreleased so consumers can quickly identify migration work.

🟡 Dense multi-clause bullets — Bullets 1, 2, 4, and 7 each bundle 2–3 unrelated changes into one sentence with semicolons. Example: bullet 1 mixes a default-toggle change with a disclaimer about model estimates. Split into separate bullets per discrete change — changelogs should be scannable.

🟡 warningsstats["schedule"]["report"] is a breaking API change — But it's buried mid-sentence in the API migration bullet alongside non-breaking items. Separate it and flag it explicitly (e.g., "Renamed warnings key to stats["schedule"]["report"]").

💭 Future-work bullet mixes scope and reference — "Structured post-RA/pre-emission scheduling and Fast/BURR strategy selection remain future work; see docs/topic-18/README.md." This is design documentation, not a changelog entry. Consider moving it to a See also note or dropping it entirely since the README already documents it.

💭 Header format## Unreleased — Topic 18 review iteration (2026-09-16) — the "Topic 18 review iteration" label is internal process context. External consumers don't know what "Topic 18" is. Consider ## Unreleased — 2026-09-16 or ## [0.4.0] with a placeholder date.


📁 benchmarks/audit_inst_scheduler.py

🔴 Vacuous pass on real regionssummarize() on an empty real_regions yields saved=0, win=0, loss=0, so s["saved"] >= 0 and s["win"] >= s["loss"] is true with zero evidence. If the scheduler applies no region (or every region is rejected), the audit reports passed. Add samples > 0 to the real gate (same for coverage: coverage_ratio is never gated, so modeled=0 passes).

🔴 Crash instead of recorded casestats["schedule"] / schedule["regions"] / result.stats["original_cycles"] are hard keys. A file with nothing schedulable aborts the whole audit with a bare KeyError, producing no JSON at all. Contradicts the module docstring ("never silently skipped cases"). Prefer .get() + a recorded "skipped": "<reason>" row, or at least a message that names the input.

🟡 Empty corpus is silentglob("[0-9]*.dsl") returns nothing if the dir is missing or empty, and the loop happily runs on just the ONNX. Fail loudly if no .dsl cases are found.

🟡 Private API couplingdriver._parse, driver._generate_code, driver._run_asm_passes are implementation details; any refactor breaks the audit with AttributeError. Also driver is constructed per-allocator outside the path loop but reused across files — confirm it holds no per-file state, otherwise results leak between cases.

🟡 Line/id alignment assumedstart <= inst.id + 1 <= end only maps correctly to region["start_line"]/end_line if ids are 1:1 with source line numbers. Comments or blank lines inside a parsed block break the mapping, silently producing the wrong region body. Assert the mapping, or index by line number directly.

🟡 Unchecked operand indexing — in canonical_region, operands[0]/operands[1] when address is not None will IndexError on any single-operand addressing form. Guard the shape explicitly.

🟡 Masked missing statresult.stats.get("sensitivity_rejected_regions", 0) reports 0 when the stat simply isn't emitted. That's exactly the silent-skip pattern the docstring forbids; report null or treat it as an error.

🟡 Path.cwd() hardwired--root argument would remove the "run from repo root" footgun and match the --json/--llvm-mca style already there.

💭 JSON/markdown written non-atomically — a crash mid-write leaves a truncated report that looks authoritative.

💭 --version truncated to 3 lines may cut the actual version string on some builds.

💭 _gen_instructions is private; importing it cross-module makes the synthetic corpus a hidden dependency on another benchmark's internals.

Otherwise the structure is sound: subprocess uses list args (no shell injection), check=True plus the stderr.strip() guard and the Total Cycles regex all fail loud rather than parsing garbage, and keeping losses/ties in the JSON is the right call.


📁 benchmarks/bench_inst_scheduler.py

🔴 Verify SchedInst constructor signature_gen_instructions now calls SchedInst(index, op, operands, raw_line=...) positionally and drops defines/uses; the old call used id=/opcode=/defines=/uses=. If the class still requires defines/uses, or its positional order differs from (id, opcode, operands, ...), this raises on the first _gen_instructions call. Confirm defs/uses are derived from operands/raw_line and that positional order matches.

🔴 Fragile coupling to result.stats schemabench_schedule indexes ~10 string keys (original_cycles, regions, sensitivity_rejected_regions, ...). Any rename or omission in the scheduler produces an uncaught KeyError mid-benchmark, after several sizes already printed. Consider exporting a TypedDict from the scheduler and importing it here so mismatches fail at type-check time.

🟡 Self-dependencies slip into the chainsdst = rng.choice(groups[chain]) can equal src = previous[chain], giving e.g. add x5, x5, x5 (~12% per instr → hundreds over 5000). That puts a register in both defs and uses, adding a self-edge and undermining the "independent chains" intent. Filter with rng.choice([r for r in groups[chain] if r != src]) if clean chains matter.

🟡 sw reads an orphan register — for sw, dst is the stored value but previous[chain] isn't updated, so that read has no producer. If strict mode flags orphan uses, strict=True raises inside the timing loop. Validate parseability once, outside the loop, before timing.

🟡 strict=True can abort the whole run — one bad random operand combo kills all remaining sizes. Wrap the loop body, count failures, and surface errors per row rather than crashing.

🟡 Lost dependency-depth axis + dead code — the old benchmark swept dep_chains 1..20 at fixed size; the new one only varies size. bench_build_dag is now defined but never called in main. Either keep a small chain sweep (which would also revive bench_build_dag) or delete the function.

🟡 Text round-trip couples the benchmark to the assemblerraw_line is synthesized as " " + op + ", " + operands then re-parsed by schedule_assembly. Any whitespace or escaping drift silently changes operands or aborts. Build the source once and fail fast with a clear message on the first parse, rather than letting it die inside the timed loop.

💭 row["seed"] = 42 is hardcoded in main while _gen_instructions takes a seed param — pass it through so the record stays honest if the default changes.

💭 _markdown reads row['model'] only from records[0]; fine today, brittle if the model ever varies per size.


📁 benchmarks/cases/inst_scheduler_feature.asm

🟡 Missing symbol visibility — If the test runner resolves scheduler_feature via the ELF symbol table, add .globl scheduler_feature (or the equivalent in your toolchain's convention). Without it, local linker scripts or position-dependent lookups may silently fail.

🟡 No halt/return marker — If the runner distinguishes "execution complete" from "fell off the end," add a ret or NOP+break at the end. Currently the runner must know the exact instruction count to avoid stepping past the block.

💭 Comment precision — "model cycles 5 -> 4" is correct but could specify the baseline model name (e.g., "single-issue, 1-cycle load-use forwarding stall") to disambiguate from other scheduler strategies that might also achieve 4 cycles for different reasons.


📁 benchmarks/run_inst_scheduler_case.py

🔴 Bug: No file existence check — Line 125: case_path.read_bytes() raises FileNotFoundError which is caught by the broad except Exception in main(), but an explicit check with a clear error message would be better.

🟡 Fragile: Private API access — Lines 130, 138: baseline._run_asm_passes() and driver._run_asm_passes() are private methods. Consider using public APIs or wrapping them in a public method.

🟡 Redundant validationrepeats < 1 is checked in both run_case() (line 135) and main() (line 260). Remove one.

🟡 Static mode always reports "passed" — Lines 168-185: In static_only mode, errors is never populated, so status is always "passed" even if scheduling has issues (e.g., applied_regions == 0). Consider validating scheduling stats in static mode too.

💭 Broad exception handling — Line 268: except Exception as exc is broad. The # noqa: BLE001 comment acknowledges this, but consider catching specific exceptions (e.g., FileNotFoundError, ValueError) separately for better error messages.

💭 Inconsistent simulation structure — Static mode returns a simple dict with "status", "backend", "output_equal", "reason", while execution mode returns a complex dict with nested "before"/"after" dicts. This makes downstream consumers need to check the structure.

💭 Hardcoded valuesDATA_ADDRESS = 1024, DATA_WORDS = [9] + [0] * 15, INITIAL_REGISTERS = {10: DATA_ADDRESS, 7: 7} are hardcoded. Consider making these configurable or deriving them from the test case.

💭 Underscore-prefixed method in docstring — Line 3: Mentions _run_asm_passes in the docstring, reinforcing the private API coupling.


📁 docs/topic-18/18-指令调度器Benchmark.md

🔴 Version Ambiguity — Line 8-15: Recommends Python 3.12 as "CI 基线" but local tests use 3.14.7, and explicitly disclaims 3.8 support while metadata declares it. A reader can't determine the actual supported range. State the real floor explicitly (e.g., "requires ≥3.10") or remove the 3.8 hedge and clarify the metadata is stale.

🟡 Meta-Instructions Mixed into Documentation — Line 63: "不要继续使用旧模型的 240→187 等数字描述本版" reads as a changelog note, not stable docs. Move deprecation/migration guidance to a separate changelog or remove; the doc should describe what the system does now, not what not to say about old versions.

🟡 Reproducibility Gap for CNN Step — Line 34-37: gen_minimal_cnn.py is invoked when models/graph/cnn.onnx is missing, but the doc doesn't state whether the generated model is deterministic (same seed?). If it isn't, CI artifacts will diverge across runs. Add a seed/seed-parameter reference or note that outputs may differ.

🟡 Artifact vs. Gitignore Contradiction — Line 107: "报告文件属于生成产物,保留在已被忽略的 benchmark_reports/ 中" then immediately says CI collects them as artifacts. New contributors may git add . confused by the ignore. Add a one-liner: "CI uploads them before cleanup; local developers can safely delete this directory."

🟡 Dense Sections Lack Scannability — The "固定功能用例" section (lines 17-55) packs: case description, expected values, verifier behavior, safety constraints, and a table into one block. Split into "用例描述" / "验证与安全性" sub-headings for readers who only need one aspect.

💭 Phrasing — Line 8: "不能只凭 .venv 目录名认定兼容" is slightly awkward. Consider: ".venv 目录名不等于 Python 版本,请先确认解释器版本."


📁 docs/topic-18/18-指令调度器Review迭代报告.md

🔴 Repro Inconsistency — Section 6 command uses --llvm-mca llvm-mca-18, but Section 3 results explicitly state LLVM 22.1.8 and warn "不能把不同版本的数字直接混在同一列比较". Anyone running the documented command with LLVM 18 will get different numbers than those reported. Either update the command to 22.x or note that Section 3 numbers require LLVM 22.

🟡 Model Parameters Stated in Wrong Place — Primary latencies (mul=4, div=33) appear only in the F11 table row. Section 2, which describes the model design, only mentions the secondary check set (mul=3, div/remainder=66). A reader of Section 2 alone cannot determine the primary model's parameters. Suggest stating the primary set in Section 2 explicitly.

🟡 Python Version — ".venv/bin/python 3.14.7" — verify this is not a typo (3.12/3.13?). If intentional, the reproduction section says "Python 3.12+" but the actual toolchain is 3.14; the gap matters if anyone has 3.12 installed.

💭 F11 Table Wording — "乘法 4 拍、整数除法 33 拍;除法保守阻塞发射并保持相对顺序" — "除法" appears twice in close proximity (latency + blocking). Consider "整数除法延迟 33 拍并保守阻塞后续发射" for clarity.

💭 Section 4 Region Count — "本轮两条路径合计 12 个已应用区域" — greedy 3 files + linear 3 files = 6 files → 12 regions implies ~2 regions/file. Worth a parenthetical so readers don't expect 12 distinct files.


📁 docs/topic-18/18-指令调度器SPEC-Review.md

Code Review: docs/topic-18/18-指令调度器SPEC-Review.md

🔴 Blockers

🔴 Contradiction between §2.2 and §4.3 on fail-stop policy

§2.2 praises "失败原子性(第 11 条)确保部分调度结果不会污染输出" as a positive invariant design, but §4.3 criticizes the same fail-stop behavior as inappropriate for an educational compiler. The document takes two conflicting stances without acknowledging the tension.

🔴 Redundant restatement of the same 3 gaps across 4 sections

The "linear-scan / integration / MachineInstr" triad is restated in:

  • §3.1/3.2/3.3 (detailed)
  • §4.1 (brief)
  • §7.1 (summary)
  • §7.3 (action items)

A reader hits the same argument 4×. Suggest consolidating to one detailed treatment (§3) with a single-pointer reference in §7.

🟡 Suggestions

🟡 "课题意义" section (lines 14–80) is mis-scoped for a review document

This ~70-line block explains LLVM instruction scheduling fundamentals (pipeline stages, load-use hazards, pass pipeline) that are background education, not review content. It breaks the review's narrative flow and reads like a separate tutorial embedded mid-document.

Suggestion: Move to an appendix or a separate 18-指令调度器背景说明.md. The review should assume readers understand instruction scheduling and start directly with §1.

🟡 Section numbering is inconsistent

The document has an unnumbered "课题意义" section before numbered sections 1–7. Either number it as §0 or demote it to unnumbered appendix. Currently it disrupts the reading flow.

🟡 Code path references should be verified and dated

References like compiler.py:60, compiler.py:397-418, inst_scheduler.py:186-207, machine_types.py:134-142 assume a frozen code snapshot. The document has *代码基线:ScratchV main HEAD* but no commit hash. In 6 months these line numbers will be stale. Consider adding a git rev-parse HEAD snapshot.

🟡 §3.4 contradicts its own premise

§3.4 argues that _asm_parser.py's classify_def_use is more mature than SPEC assumes. But then §3.1–3.3 all argue the SPEC is too ambitious for the current code. If the text-level parser is already mature enough, the SPEC's push toward structured IR is less urgent — this tension should be acknowledged, not left implicit.

🟡 §6 "SPEC 中可从详细设计吸收的部分" is actually "SPEC V0.2 改进建议"

This section lists what SPEC should change (data class definitions, integration compromise, test cases). But it's positioned as "SPEC absorbs from detailed design" — the framing is off. It should be titled "SPEC V0.2 修改建议" to match its content.

💭 Nits

💭 §2.1 table: Column "严重性" uses "高/中" without defining a severity scale. Other tables use "9/10" numeric scores. Inconsistent rubric.

💭 The document header says "Review 生成日期:2026-07-28" but the SPEC under review is dated "2026-07-25" — only 3 days apart. The top banner says this is a "历史评审" as of 2026-09-16, which is 7 weeks later. The temporal framing is muddled — clarify whether this review was written at SPEC submission time or retroactively.

💭 ASCII pipeline diagram (IF→ID→EX→MEM→WB) is standard textbook content. If keeping it, cite the source (e.g., Patterson & Hennessy) for academic rigor.


Summary: The review is thorough and well-researched, but over-length (354 lines), contains one internal contradiction (fail-stop policy), and restates the same 3 gaps four times. The biggest improvement would be cutting the educational background section and consolidating the redundant gap analysis.



⚠️ 未审查的文件

  • docs/topic-18/18-指令调度器代码说明.md
  • docs/topic-18/18-指令调度器实现计划.md
  • docs/topic-18/18-指令调度器设计文档.md
  • docs/topic-18/README.md
  • docs/topic-18/review-iteration-results.json
  • docs/topic-18/topic18-review.md
  • scratchv/backend/init.py
  • scratchv/backend/inst_scheduler.py
  • scratchv/backend/machine_types.py
  • scratchv/backend/regalloc_linear.py
  • scratchv/backend/schedule_model.py
  • scratchv/backend/schedule_semantics.py
  • scratchv/backend/schedule_verify.py
  • scratchv/compiler.py
  • scratchv/main.py
  • tests/test_inst_scheduler.py
  • tests/test_inst_scheduler_integration.py
  • tests/test_inst_scheduler_report.py
  • tests/test_inst_scheduler_review.py
  • tests/test_inst_scheduler_safety.py

@Mastttttter Mastttttter changed the title feat(backend): 完善课题 18 安全局部指令调度、验证与 Benchmark feat(backend): 完善课题 18 安全指令调度与 A/B Benchmark 报告 Sep 14, 2026
Correct conservative timing and latency-sensitive scheduling, restore linear control-flow targets, improve section handling and coverage reports, and add independent CPU-model CI audits. Document review outcomes and stop tracking local session state.

Validation: 901 tests passed with no skips; both CPU audit thresholds passed, with remaining negative cases retained in the report.
@Mastttttter

Copy link
Copy Markdown
Author

已根据第二轮 review 完成迭代,并推送到同一分支,新增提交:d5372f8,请帮忙复审。

本轮主要修改:

  • 修正调度成本模型,保持除法指令的相对顺序,并增加延迟敏感性检查,避免仅在单一参数下有收益的重排。
  • 修复 linear 分配器的分支目标、标签发射及相关读写语义。
  • 修复段状态恢复、字符串误报和指令计数差异,补充覆盖率及未建模原因。
  • 增加 clang/QEMU 执行验证和双 CPU llvm-mca 审计的 CI 任务。
  • 清理误跟踪的会话文件,更新文档状态、兼容性说明及迭代报告。

本地验证结果:

  • 全仓 901 项测试通过,无跳过。
  • 72 个合成样例在 Rocket、E76 模型下均通过总体收益门槛;review 中的最小负例已保留原序。
  • 仍分别存在 9 个和 11 个负优化样例,已完整保留在报告中。调度继续默认关闭,不将静态模型结果解释为硬件加速。

F1–F16 逐项处理及迭代报告

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant