You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
🟡 Magic number in bench-topic06 — --benchmark 3 has no explanation of what 3 represents (3 iterations? 3 test cases? a benchmark ID?). If the semantics change or a new reader encounters this, it's opaque. Consider adding a comment (e.g., # 3 = number of runs for statistical confidence) or, if scripts/run_topic06_benchmarks.py supports a named option (e.g., --runs or --mode), use the more descriptive flag.
💭 No test dependency — bench-topic06 is documented as "DSL correctness + TinyFive benchmark" but doesn't run test first. The existing bench target also doesn't enforce this, so it's consistent, but worth noting in case a stale build gives misleading benchmark numbers.
💭 The target is clean and follows the existing style. No issues with .PHONY coverage.
📁 README.md
🟡 Suggestion: make bench-topic06 命名不一致 — 其他 benchmark 命令都是 make bench-cnn / make bench-ci / make bench-reports,按模块或用途命名。bench-topic06 混入了项目内部术语("课题 06"),与 bench-cnn(按模型命名)风格不一致。考虑改为 make bench-tinyfive 或 make bench-dsl,既与 bench-cnn 对齐,又对未参与课题分配的人更友好。
🟡 Missing benchmarks/topic06/ directory context — This is a new baseline file with no companion code (e.g., benchmark definitions or collection script). If other files in this PR define these 20 cases, ensure their names are identical to avoid silent mismatches.
🟡 No metadata for baseline provenance — There's no indication of the hardware, compiler version, or environment used to produce these avg_instr_count values. Future reviewers won't know whether drift in avg_instr_count is meaningful or expected. Consider adding a "meta" field or a sibling README.
🟡 avg_instr_count stored as float but always integer-valued — All values are .0. If these are true averages over runs: 3, consider rounding explicitly and documenting that, or store as int if runs always yields whole-number averages. The float type suggests the expectation of non-integer results that never materializes.
💭 No trailing newline — \ No newline at end of file. Most POSIX tools and git diff behave better with a trailing newline.
💭 No "runs" variance captured — With runs: 3, the standard deviation or min/max could help distinguish stable baselines (e.g., vector_add: 3.0) from noisy ones. A future regression alert could benefit from knowing the spread.
🟡 Suggestion: pytest already included in existing groups? — If pytest is already listed elsewhere in [project.optional-dependencies] (not shown in this diff), consider whether topic06 should just reference it rather than duplicating the dependency. Duplicated version pins across groups can cause drift.
🟡 Suggestion: Version pinning — tinyfive, jinja2, and matplotlib have no version constraints. If these are third-party packages, consider pinning (e.g., matplotlib>=3.8,<4) to prevent unexpected breaking changes when pip install .[topic06-report] is run in the future.
💭 Nit: Naming inconsistency — Other groups use descriptive verbs (verify, llvm) while these use topic/course numbering (topic06, topic06-report). If these are permanent project groups rather than temporary course assignments, a more descriptive name like simulation-report would be clearer long-term.
📁 scratchv/backend/register_alloc.py
🟡 No guard on allocation state — register_map returns an empty {} if called before run() completes allocation. Consider raising RuntimeError or returning None if self._output is None, so callers get a clear signal rather than silently getting an empty map.
🟡 Shallow copy on every access — dict(self._vreg_map) allocates a new dict each call. If this property is accessed frequently (e.g., in a loop during instruction emission), consider returning self._vreg_map.copy() only when mutation is needed, or documenting that callers should not mutate the result.
💭 Documentation gap — The docstring doesn't mention that the result is a snapshot (immutable copy), which is a useful contract for consumers.
📁 scratchv/compiler.py
🟡 Inconsistent deep copy of register map — Lines 419, 446: self._last_register_map = alloc.register_map assigns a direct reference, while line 420 uses dict(lsa.alloc_map) (a copy). If alloc.register_map is mutated later (e.g., allocator reuse across calls), the stored map silently changes. Apply dict() consistently, or document the ownership contract.
💭 Defensive reset placement — Line 238: self._last_register_map = {} resets at the top of compile(), which is good. However, if _generate_riscv_linear/_generate_riscv_dag raises before reaching the stats dict construction, the returned result (via raise CompileError) would not include stale stats, so this is fine in practice — just noting the dependency on early reset.
💭 Naming — _last_register_map reads as an internal implementation detail, but it's now exposed to callers via stats["register_map"]. Consider whether a clearer public-facing key name (e.g., "alloc_map") would reduce confusion between temporary spill registers and architectural registers.
📁 scratchv/main.py
🟡 Uncaught TypeError in JSON serialization — Line ~267: json.dumps will raise TypeError if any value in register_map isn't JSON-serializable (e.g., int vs np.int64, custom objects). This isn't covered by the OSError catch, so it would propagate as an unhandled exception.
Suggestion: Either catch (OSError, TypeError) or add a default=str fallback in json.dumps.
🟡 No confirmation on success — The register map is written silently. If something went wrong with the data (e.g., empty register map written), the user gets no feedback. Consider printing a confirmation message like "Register map written to {path}" to stderr after a successful write.
🟡 Potential issue with result.stats.get(...) — If result.stats is ever None or a non-dict type, this will raise an uncaught AttributeError. Depends on how stats is always populated, but a defensive check (result.stats and result.stats.get(...)) or a comment asserting it's always a dict would improve robustness.
💭 OSError is broad — Catches IsADirectoryError, PermissionError, FileExistsError, etc. Consider catching more specific exceptions or at least adding a comment explaining the intent. OSError isn't wrong here, just wide.
💭 Indentation consistency — The print(f"OK ...") line appears pre-existing but uses a trailing comma + line break for the second argument. Minor, but worth noting if this is the style for this codebase.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
变更概述
将课题 06 编译器性能测试套件迁移并集成到 ScratchV 项目目录中,用于自动验证 DSL 编译结果、TinyFive 模拟结果和编译性能。
主要改动
scripts/run_topic06_benchmarks.py测试入口。scripts/、tests/、benchmarks/和docs/目录。测试结果
已知限制
当前真实 TinyFive 全量测试为 13/23 passed。
未通过用例主要集中在:
这些用例保留为诊断用例,不作为当前稳定 CI 门禁。稳定 CI 当前只启用 activation、elementwise 和 loop 类别。
使用方法