topic 17 LinearScanAllocator Benchmark

[feiji111]

依据设计文档，设计了三个Benchmark：

1. 简单算术：3-5 个 vreg 的基本算术运算，验证无溢出时的分配正确性
2. 高密度变量：30 个 vreg 在 5 个物理寄存器上运行，验证溢出逻辑
3. CNN Conv2D 集成：将分配器接入 CNN 编译管线，验证生成的汇编能被 Spike 仿真正确执行

可能的问题：
Benchmark是基于v1.3的寄存器分配算法设计(PR#37)，但是目前PR还未合并到主线

[github-actions]

🤖 AI Code Review

共审查 9 个变更文件

📁 .github/workflows/ci.yml

🟡 Suggestion: Missing error resilience on benchmark step — Lines 213-218: The new register allocation benchmark lacks a failure fallback, unlike the dashboard generation step above it (which uses || echo "WARNING: dashboard generation failed"). If this benchmark fails intermittently (e.g., resource contention), it will fail the entire main push job. Consider adding || true or at minimum a timeout-minutes on the step.

💭 Nit: Cosmetic run: | change — Line 76: The original single-line run: python3.12 -m pytest ... works fine. Using | with nothing after it adds no benefit here (it's just one command). If this was done to match the block-scalar style of the new benchmark step, that's fine as a consistency choice, but not necessary.

🟡 Suggestion: No timeout on the benchmark step — --repeats 30 suggests a potentially long-running job. Consider adding timeout-minutes: 10 (or appropriate value) to prevent CI from hanging indefinitely if the benchmark hangs on a resource-constrained runner.

---

📁 benchmarks/test_regalloc/__init__.py

🟡 # flake8: noqa 无必要 — Line 1: 文件仅有 docstring 和 __future__ import，没有会触发 flake8 的代码。全文件禁用 lint 是反模式，会掩盖未来引入的真实问题。建议移除。

🟡 from __future__ import annotations 位置不常规 — Line 18: 虽然 PEP 允许 __future__ 在 docstring 之后，但几乎所有工具链和编码规范都期望它出现在文件最顶部（仅 docstring 可例外）。将其移到 Line 1（在 # flake8 之后）更符合惯例和可读性。

💭 包初始化缺少显式导入 — 模块名暗示 bench1_simple、bench2_dense、bench3_cnn 应从 benchmarks.test_regalloc 直接可用，但当前没有 from . import bench1_simple 等语句。如果依赖隐式子模块导入（Python 3.3+ 的 from .run_all import ... 路径），确保外部调用方知道必须直接引用子模块，否则会引发 ModuleNotFoundError。

---

📁 benchmarks/test_regalloc/bench_cnn.py

🔴 Misleading benchmark data — "warmup" loop is the measurement loop — Lines 216-223: The comment says "Warm up" but the loop is appending times and spill_counts used in the final stats. The actual timing data is the warmup. Either rename to "measurement" or add a real warmup before collecting stats.

🔴 Leaking internal objects in stats dict — Line 252-253: Returning "_alloc": alloc and "_report": alloc.report() in the stats dict. The allocator object is mutable state; if this dict is serialized (e.g., to JSON) or accessed after the function returns, it creates coupling. If _report is only needed for CLI display, print it there instead of storing it in stats.

🟡 Import ordering violation — Lines 62, 109-115: from benchmarks.test_regalloc.bench_utils import ... imports appear after function definitions and between logical sections. Move all imports to the top per PEP 8.

🟡 Inconsistent import style — Line 180 uses from .bench_utils import llvmlite_ir_to_riscv (relative) while other imports are absolute (benchmarks.test_regalloc.bench_utils). Pick one convention.

🟡 Dead commented code — Line 178: # lib = _load_llvm() is commented out but _load_llvm is still imported on line 177. Remove the import or the comment — either way, it adds confusion.

🟡 Bare except Exception in emulator path — Line 136: _run_emulator swallows all exceptions silently, returning {"passed": False, "error": ...}. If this is invoked from a test suite, important tracebacks (e.g., segfaults, memory errors) are lost. Consider logging the full traceback at debug level.

🟡 run_bench sets redundant alias — Line 268: stats["valid"] = stats["asm_valid"] creates a confusing duplicate key. Use one name consistently.

💭 # flake8: noqa disables all linting — Line 1: This suppresses every linter warning across the file. Consider targeted noqa comments if specific lines need exemptions.

💭 No structured output mode for CLI — main() prints human-readable text only. For benchmark collection pipelines, a --json flag would be more useful.

---

📁 benchmarks/test_regalloc/bench_dense.py

Review: benchmarks/test_regalloc/bench_dense.py

🟡 Leaking mutable state through return dict — bench_allocate returns _alloc (a live LinearScanAllocator instance) in the stats dict. This couples callers to an implementation detail and can cause memory leaks if the dict outlives the allocator. Consider returning only reportable values, or removing _alloc from the public return.

🟡 Redundant key reg_spill_count — Lines 78-79: spills and reg_spill_count are the same value (spill_counts[-1]). Remove one to avoid confusion; main() uses both but for the same purpose.

🟡 Broad # flake8: noqa — Suppresses all linting for the entire file. If specific warnings need suppression (e.g., line length from f-strings), use targeted ignores like # noqa: E501 so other issues still surface.

🟡 Global random.seed inside helper — _gen_block calls random.seed(seed) which mutates process-wide state. If this benchmark runs alongside other code that relies on the RNG, it silently resets. Consider using a local random.Random(seed) instance instead.

💭 sys.exit(main() or 0) — Works but reads oddly. main() already returns 0 or 1, so sys.exit(main()) is equivalent and clearer. The or 0 implies the return type might be falsy/None, which it isn't here.

💭 statistics.stdev guard is correct but implicit — len(times) > 1 prevents the StatisticsError when repeats=1, but repeats=0 would cause statistics.mean([]) to fail. Consider validating repeats >= 1 at the argparse or function level.

---

📁 benchmarks/test_regalloc/bench_regalloc_linear.py

🔴 Bug: KeyError on missing reg_spill_count — Lines 118, 130, 142 use r1['reg_spill_count'] with direct [] access. If a benchmark's result dict omits this key (e.g., on early failure), the script crashes before producing any report. The HTML/MD generators defensively use .get() with fallbacks — the console print should do the same.

🔴 Bug: .get('valid') without default in console output — Lines 119, 131, 143: r.get('valid') returns None if key is absent, causing false ✗. Inconsistent with _make_html/_make_markdown which use r.get("valid", True). A benchmark that doesn't set valid will pass in reports but fail on console.

🟡 No error isolation between benchmarks — If bench_simple.run_bench() raises, benchmarks 2 and 3 never run. Wrap each in try/except and record the failure in results so the suite always produces a partial report.

🟡 Hard-coded CNN path with no validation — Line 136: "models/graph/cnn.onnx" is fixed with no CLI override and no existence check. A missing file yields an opaque error downstream.

🟡 os imported but unused — Line 8: remove.

🟡 JSON report silently drops asm_errors — Line 166: filtering out asm_errors hides the root cause of validation failures from the JSON report. At minimum, include a valid + error summary field.

💭 Duplicate formatting logic — _make_html and _make_markdown repeat the same field-extraction pattern verbatim. Consider a shared helper like _format_row(r) -> dict[str, str] to reduce drift.

💭 sys.exit(main() or 0) — main() already returns 0 or 1; or 0 is a no-op. Just sys.exit(main()).

---

📁 benchmarks/test_regalloc/bench_simple.py

🔴 Duplicate dict key — Lines 76–77: "spills" and "reg_spill_count" both set to spill_counts[-1]. The second key shadows the first in the returned dict. If a caller reads stats["spills"] and a different one reads stats["reg_spill_count"], they get the same value, but the redundancy hides bugs (e.g. if one line is later changed to alloc.num_spills while the other stays). Use one key name.

🟡 Leaked internal state — "_report" and "_alloc" (the live allocator instance) are returned in the public stats dict. Returning a mutable internal object breaks encapsulation — callers could mutate allocator state after the benchmark. Print _report directly and drop _alloc from the return value.

🟡 Global random state mutated — _gen_block calls random.seed(seed), which modifies the process-wide RNG. If this benchmark is run alongside other tests that rely on random, they'll get unexpected values. Use a local rng = random.Random(seed) and call rng.choice instead.

🟡 stats["valid"] is set but unused — run_bench computes stats["valid"] = stats["spills"] == 0, but main() independently checks spills == 0. Either use stats["valid"] in the pass/fail logic or remove the key to avoid dead state.

🟡 vreg_count semantics ambiguous — len(alloc.alloc_map) counts vregs that got a physical register. If spilling is later introduced, this number will silently drop below num_vregs. Name it allocated_vregs or include total vregs for clarity.

🟡 No error handling in bench_allocate/main — If allocate() raises (e.g. missing instruction field), the benchmark crashes with no diagnostics. Wrap in try/except and print a clear message.

💭 Magic numbers scattered — num_insts=10, num_vregs=5, 8 phys regs appear in _gen_block, run_bench, and main. Hoist to module-level constants (e.g. NUM_VREGS = 5, NUM_PHYS_REGS = 8) for consistency and discoverability.

💭 sys.exit(main() or 0) — main() already returns 0 or 1. The or 0 is a no-op. sys.exit(main()) is clearer.

---

📁 benchmarks/test_regalloc/bench_utils.py

🔴 Bug: _CALLEE_SAVED defined twice — Lines 5–38 and 134–167 are identical. The second definition silently overrides the first. If a future edit changes only one copy, it will be a subtle bug. Remove the first (orphan) definition.

🔴 Bug: Redundant import inside function — from llvmlite import binding as llvm on line 213 runs on every call, even after line 196 already imported it (conditional on first call). Hoist this to module level or remove the inner duplicate.

🟡 _KNOWN_OPS is unused — The set is defined but never referenced in this module. If it's dead code, remove it. If it's intended for validation, add the validation logic.

🟡 Silent except Exception: pass — Line 207 swallows all errors loading libLLVM-20.so. A real loading failure (permissions, missing symbol, wrong architecture) becomes invisible. Consider at least logging a warning or catching a more specific exception.

🟡 Module-level imports should move up — from scratchv.standalone.compare_codegen import count_riscv_instrs on line 214 is imported on every function call. Move to module top-level for clarity and zero-overhead subsequent calls.

🟡 _CAT_MUL contains opcodes not in _KNOWN_OPS — mulh, mulhu, mulhsu appear in _CAT_MUL but not _KNOWN_OPS. Either _KNOWN_OPS is incomplete (inconsistent classification) or these buckets will never match.

💭 Grammar — Module docstring: "Common used benchmark utils" → "Commonly used benchmark utilities".

---

📁 benchmarks/test_regalloc/regalloc.md

🔴 指标歧义：CNN 中 reg_spill_count=0 指 ScratchV 还是 LLVM？ — §4.1 将 reg_spill_count 列为所有 benchmark 的通用键，来源是 len(alloc._spill_slots)。但 LLVM 路径不经过 ScratchV 的 allocator。§4.3 试图说明"LLVM 侧用 ScratchV 值"，但 §6.2 JSON 中 reg_spill_count: 0 与 llvm_spill_slots: 87 并存，读者无法判断 reg_spill_count 在此上下文的语义。建议在 §4.2 的 bench_cnn 特有键中明确定义该值属于 ScratchV 侧，并在 JSON schema 中用 scratchv_spill_count 消除歧义。

🔴 TODO 第3项应提升为文档警告而非 TODO — "编译路径无法完全正确生成算子的汇编指令，与 LLVM 对比不公平" 是衡量结果有效性的核心前提。放在 §8 TODO 底部容易被忽略，应移到 §3.3 开头作为 ⚠️ 醒目提示，说明 LLVM 对比仅具参考价值、不能作性能基准。

🟡 spills 与 reg_spill_count 冗余 — §4.1 列出两个键来源完全相同。统一接口应只保留 reg_spill_count 一个键，spills 应作为 deprecated 别名移除或仅作为兼容层。双键会增加下游报告渲染的维护成本。

🟡 greedy_time_s / greedy_out_instrs 缺乏上下文 — 这两个键在 §4.2 中孤悬，文档其余部分未提及 Greedy allocator 何时触发、作为 baseline 的意义、与 LinearScan 的关系。至少需要在 §3.3 或 §4.2 中说明：是否在 CNN 路径中同时运行 Greedy 作为对照？运行条件是什么？

🟡 repeats 未在指标规范中定义 — §6.2 JSON 示例中 "repeats": 3 出现在顶层，但 §4.1 通用键表未包含它。应补入 §4.1 或新增 §4.4 顶层字段说明。

🟡 LLVM 溢出统计误判风险未量化 — §5 的溢出检测基于 sw/lw/fsw/flw <reg>, N(sp) 正则，但合法的栈数组访问、局部变量初始化也会命中此模式。文档标注了"近似"但未给出误差范围或排除规则。建议补充：已知误报场景、与帧操作数的比例一致性检查（§6.2 中 frame_save=70 与 llvm_spill_slots=87 差异较大，是否合理？）。

🟡 bench_cnn 的模型路径参数传递链不完整 — §7 中 main runner bench_regalloc_linear 未列出 --cnn-path 参数，但它在内部会调用 bench_cnn.run_bench()。如果 CNN 路径需要自定义模型，runner 如何传递？应在 §2.1 或 §7 中说明参数透传机制。

💭 asm_lines 与 sv_static_instrs 的关系未在通用说明中澄清 — §4.1 定义 asm_lines = len(code.splitlines())（含标签/注释），§4.2 定义 sv_static_instrs = count_riscv_instrs()（不含）。简单 benchmark 只暴露 asm_lines，CNN 暴露两者。建议说明二者的换算关系或至少在简单/Dense 场景也统一暴露静态指令数。

---

📁 docs/topic17_benchmark文档.md

🔴 Bug: peak_active=14 与物理寄存器池=5 矛盾 — §3.2 定义 5 个物理寄存器（r0–r4），但 §6.3 示例表中 Dense 的 Peak = 14。
建议：若 peak_active 度量的是虚拟寄存器活跃数（live range 压力），应在 §4.1 明确写为「峰值同时活跃的虚拟寄存器数」，避免与物理寄存器池混淆。

🔴 Bug: reg_spill_count 语义对 LLVM 路径描述不清 — §4.3 写道「LLVM 侧 reg_spill_count 是本路径的 ScratchV 精确值（0）」。读者无法理解：LLVM 路径未调用 ScratchV regalloc，为何赋 0？
建议：明确说「LLVM 路径无 regalloc 输出，reg_spill_count 设为 null/None 或占位 0，并标注 reg_spill_source: "scratchv" vs "llvm_approx"」。

🟡 Suggestion: spills 与 reg_spill_count 重复冗余 — §4.1 两者来源完全相同（len(alloc._spill_slots)），保留别名增加维护成本且易产生不一致。
建议：只保留 reg_spill_count 一个键，删除 spills 列；若报告表格需要短名称，在渲染层做映射而非 dict 层重复。

🟡 Suggestion: 伪指令 vs 真实指令定义矛盾 — §3.3 说 ScratchV 输出「57 条伪指令（mv/mul/add/slt/benez…）」，但 mv/mul/add/slt/benez 均为标准 RISC-V 指令，与「伪指令」及「conv/maxpool 由仿真器实现」矛盾。
建议：明确哪些是 RISC-V 真指令、哪些是保留语义级操作；或统一称为「中间指令」而非「伪指令」。

🟡 Suggestion: _gen_block 随机性不可复现 — §3.1/3.2 使用随机生成指令但未指定随机种子。
建议：文档中声明 random.seed(42) 或类似固定种子，确保 benchmark 结果跨平台可复现。

🟡 Suggestion: 不公平对比问题应提前到主要位置 — TODO §8.3 承认「与 LLVM 后端对比不公平」，但这是影响整个 §3.3 结论有效性的核心缺陷，不应埋在 TODO。
建议：将该说明提升为 §3.3 内的独立限制说明块，与现有 instr_ratio_fd 注记合并。

🟡 Suggestion: greedy_out_instrs 来源缺失 — §4.2 来源 列为「—」，无法追溯。
建议：补充具体来源字段路径（如 alloc_greedy.get_code() 行数）。

💭 Nit: asm_lines 是否含空行/注释不明确 — §4.1 使用 len(code.splitlines())，会包含空行和注释行；而 sv_static_instrs 明确「不含标签/注释」。表格 §6.3 中 "Asm"=57 与 sv_static_instrs=46 不一致，未解释差异原因。
建议：在 §4.1 注明 asm_lines 为原始行数（含空行/注释），与 sv_static_instrs 区分。

💭 Nit: stdev 未指定总体/样本 — §4.1 写 stdev 但未说明 ddof 取值。
建议：写明 numpy.std(values, ddof=1) 或 statistics.stdev。

---

[jizhenjun]

移到ci里

[jizhenjun]

移到ci里

[jizhenjun]

集成到.github/workflows/ci.yml line143