Impl/topic28

[FeelTheBeats]

No description provided.

[github-actions]

🤖 AI Code Review

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

📁 .github/workflows/ci.yml

Clean. No issues to flag.

• Test step follows the exact pattern of the preceding test_pr37_regression step (same python3.12 -m pytest, --tb=short conventions).
• Benchmark step matches the const_merge_report step structure.
• Summary append uses the same if [ -f ... ] guard pattern.
• mkdir -p is idempotent, harmless redundancy.

---

📁 benchmarks/cases/topic28_extended_isel_feature.dsl

Review: benchmarks/cases/topic28_extended_isel_feature.dsl

Arithmetic traces correctly (acc=6, sq=36, bias=36, total=42, res=7). No blockers.

💭 Implicit loop-scoping assumption — Lines 15-17 reference acc, neg_acc, etc. after endfor, assuming loop-body variables persist. This is non-obvious DSL semantics. A one-line comment noting this assumption (or that the harness expects it) would help future readers not chase a "scoping bug" that isn't one.

💭 No machine-checkable expected output — The comment on line 12 documents res = 7 for humans, but the harness (run_topic28_extended_isel_case.py) likely hardcodes the expectation elsewhere. Consider a trailing # expected: 7 convention if the harness supports it, to keep oracle and case colocated.

---

📁 benchmarks/run_topic28_extended_isel_case.py

🔴 Brittle private-API coupling — builder._emit(...) (line ~107), driver._generate_code(program) (line ~233), and _is_fd_mnemonic (line ~63) are all underscore-private. This benchmark is the single source of truth for Topic 28, and any refactor of those internals silently breaks it with no signal. Suggestion: expose thin public wrappers (builder.runtime_const(), driver.compile_program(), a public is_fp_mnemonic() predicate) rather than reaching into internals.

🟡 fp64_flags_without_extended_warn uses any(...) over two distinct rows — lines ~478-482. The check name implies both fp64_flag_without_extended and hardware_sqrt_without_extended warn, but only one passing is enough. If one regresses, the hard check still reports PASS. Suggestion: require both explicitly.

🟡 json.dumps(report) can crash on CompileResult.errors / .warnings — lines ~318-319, ~420. If those fields contain non-JSON-serializable objects (traceback objects, custom error types), the report never gets written and main dies after all measurement work. Suggestion: convert to str(...) / [str(w) for w in ...] before serializing.

🟡 _capture_failure swallows all Exception — line ~249. The comment justifies it, but a typo or unrelated crash inside the probe (e.g. KeyError from a bad config key) is reported as a clean "rejection" and flips a hard check to "unexpected" rather than failing loudly. Suggestion: restrict to the expected exception set ((ValueError, UnsupportedInstructionError)) and let anything else propagate.

🟡 measure_error_matrix re-reads case_path and recompiles the DSL case 5× — lines ~349-361. _compile_dsl in measure_dsl_ab already parsed the same file twice. Total DSL compiles in one run: ~9. Not wrong, but the read is redundant — parse source once in evaluate and pass it down.

🟡 EXPECTED_DSL_RESULT = 7 is a magic constant — line ~67. If the DSL case file is edited, the check silently compares against the stale value and fails with a confusing "dsl_execution_matches_expected" message rather than pointing at the drift. Suggestion: either derive the expected value from the case (or a sibling manifest) or fail fast if the case's declared expectation doesn't match.

🟡 emulator.load_code(bytes(binary)) — line ~296. assemble_to_binary already returns bytes; bytes(binary) is a needless full copy. Suggestion: emulator.load_code(binary).

💭 main renders the markdown twice — lines ~604-611 (render_markdown for the print and again for the write). Render once and reuse.

💭 warns() uses loose substring match — line ~378 (needle in w). "no effect" could match an unrelated warning containing that phrase. Consider anchoring or matching on a distinctive token.

💭 dsl_execution_registers_identical is redundant with identical_asm + deterministic execution — line ~446. Same bytes → same register state; the extra check adds signal only if execution is non-deterministic, which the emulator isn't.

💭 HONESTY is one ~1300-char string literal — line ~84. Renders fine in the report but is very hard to scan in source. A list of bullet strings joined at render time would be easier to edit.

💭 count_asm and fp_mnemonics each re-parse the same ASM — called separately per measurement. Minor, but for the FP probe the text is parsed twice per config; a single pass returning both would be cleaner.

---

📁 docs/topics/28-扩展指令选择-开发文档.md

这是一份开发文档（非代码），按内容完整性与可实施性进行审查。

---

🟡 硬编码行号 — §2.1: "禁止改动第 14–50 行既有成员"。行号随代码演进漂移，应改为语义锚点（如 OpCode.CONCAT 之后、is_arith() 之前）或断言式校验。

🟡 类型标注运行时不可用 — §2.3.2: _FP64_OPCODES 标注为 frozenset[str]。Python 3.9+ 才支持 frozenset[str] 内置泛型语法，且这是类属性而非变量注解，应写为 typing.FrozenSet[str] 或直接标注运行时表达式。

🟡 实现结果混入规格文档 — 末尾"实现结果（2026-09-14 集成）"节包含 commit hash、测试结果、偏差说明。这属于变更日志/PR 描述，混在规格文档中造成读者困惑（哪些是计划，哪些是已完成）。建议拆为独立文件或 PR body。

🟡 错误消息契约模糊 — §2.3.3: 模板 {opname} requires destination dtype in {allowed}, got {dtype} 中 {opname} 未明确是 opcode.value（如 "sqrt"）还是 opcode.name（如 "SQRT"）。测试按子串匹配 opcode 名 时可能歧义。应明确取值。

🟡 行为矩阵遗漏交叉条件 — §2.4.2: 未定义 extended_isel=True + enable_fp64=False + use_dag_isel=True + backend=llvm 的组合行为。多个开关同时冲突时优先级链未交代清楚。

🟡 P4 sqrt f64 立即数设计不完整 — §3.4 P4: "f64 字面量抛 ValueError（无 64 位物化手段）"。这是一个已知功能缺口，但文档未说明预期修复路径或临时替代方案（如拆分 hi/lo 32-bit 加载到 f-reg）。用户按文档实现后会困惑。

🟡 GLOBL/SIZE/TYPE 缺口未标注 — §2.7 列出的是 Topic 28 需要的 MachineOp 成员清单，但 R4 提到 GLOBL/SIZE/TYPE 不在其中。这三个 opcode 如果存在于现有 MachineOp 枚举中，应加入盘点清单或明确说明无需关注。

💭 文档职责重叠 — 文档反复引用"设计文档 §2.3.2 / §3 / 附录"，但本文档自身也包含设计契约（§2）和实现指南（§3），边界模糊。建议在文首明确：本文档 §2 为契约源，设计文档仅做概念说明，避免冲突时读者不知以哪个为准。

💭 Python 版本矛盾 — 文首声明 "Python 3.11+"，§4.4 却建议用 /usr/local/bin/python3.11 来跑测试，暗示默认 venv 是 3.8。建议统一表述为"项目 venv 为 Python 3.11（路径 /usr/local/...）"，删去 3.8 相关措辞。

💭 store_f64 返回值类型不一致 — §2.2: 其他 builder 方法返回 Value，store_f64 返回 Instruction。虽然 store 无 dest 值所以合理，但调用方可能期望统一接口。建议加一行注释说明为何破坏一致性。

💭 验收标准 A7 可操作性弱 — "默认管线输出与改动前逐字节一致（抽样对比）" 缺乏具体方法。建议给出对比命令或 baseline 文件位置。

---

📁 scratchv/backend/asm_emit.py

🟡 Behavioral contract change — Unknown opcodes now raise instead of producing a comment line. If any existing test fixture or upstream pass emits a forward-compat opcode intentionally skipped by this backend, it will now crash. Consider documenting this in a changelog or confirming all test suites pass without changes.

💭 instr.op.name availability — Relies on MachineOp being a Python Enum. If it's ever replaced with a plain IntEnum or int subclass that doesn't expose .name, this line itself raises AttributeError. Minor risk, but a defensive fallback (getattr(instr.op, 'name', instr.op.value)) would make the error message itself robust.

---

📁 scratchv/backend/inst_select_ext.py

🔴 Bug: _select_neg non-fp64 path uses _op instead of _materialized_op — Line ~465: if src is a literal constant, _op returns an immediate operand, but SUB is register-only in RISC-V. src = self._op(instr, 0) should be self._materialized_op(instr, 0, prefix="neg_const"). Pre-existing bug, but easy to fix now.

🟡 abs INT64: SRAI 31 is wrong for 64-bit values — Line ~249: the integer branch uses SRAI tmp1, src, 31 which only extracts the sign of a 32-bit value. For INT64, sign is bit 63. Either exclude INT64 from the allowed dtypes or emit a 64-bit-safe sequence (e.g., srli + shift-based sign extraction).

🟡 LI with full 32-bit f32 bit pattern — Line ~531: _constant_bits returns a 32-bit signed int for f32 literals, emitted via MachineOperand.immediate(bits). RISC-V LI only encodes 12-bit immediates. This relies on the encoder expanding to lui+addi+slli. If MachineOp.LI doesn't do this expansion, f32 constants will silently produce wrong code. Worth a comment or assertion.

🟡 Branchless min/max: signed subtraction overflow — Lines ~170-185 and ~198-212: sub diff, a, b overflows for e.g. a = INT32_MIN, b = INT32_MAX. The and+add result is then wrong. Acceptable if IR guarantees no signed overflow, but worth documenting in the docstring.

💭 _defined_names is program-global — Lines ~58-64: names from all functions are collected into one set. In proper SSA this is correct (names are unique), but if the same SSA name ever appears in two functions, one will silently skip materialization. Consider scoping to the current function if feasible.

---

Good stuff: deterministic temp naming via _fresh_temp + counter reset in run() is clean; exact IEEE-754 bit materialization (no more int() truncation) is the right fix; the _involves_fp64/_require_fp64 split is a nice pattern; docstrings on min/max explain the branchless encoding well.

---

📁 scratchv/backend/instruction_select.py

💭 Nit: "Topic 28" is opaque — If this refers to an external spec or internal ticket, future readers may not have context. Consider linking to the relevant doc/issue, or rephrasing as something self-explanatory like "opcodes introduced after the base ISA."

No functional issues — the comment accurately describes the dispatch contract and is helpful.

---

📁 scratchv/backend/riscv_encoder.py

🟡 Redundant prefixes in _FD_PREFIXES — fmadd., fnmadd., fmsub., fnmsub., fclass., fli., fround. are all already covered by _FD_PATTERN (^f[a-z0-9]+\.[sd]$ matches fmadd.s, fclass.d, etc.). Only fcvt. and fmv. are needed for multi-suffix forms. The redundancy is a maintenance burden — if someone adds a new single-suffix F/D instruction family, they'd likely add a redundant prefix too. Simplify to just the two multi-suffix prefixes, or add a comment explaining the redundancy is intentional defense-in-depth.

🟡 No visible test coverage — No tests for _is_fd_mnemonic or the new UnsupportedInstructionError. At minimum test: fld, fadd.s, fadd.d, fcvt.s.d, fmv.x.w, flw, and a non-F/D unknown instruction like foobar (should raise plain ValueError, not UnsupportedInstructionError).

💭 li.d in _FD_EXACT but not li.s — If li.s is also a project-specific pseudo-instruction, it should be included for consistency. If not, a short comment on why li.d is singled out would help.

💭 "Topic 28" in the error message — Internal tracking context that future readers (or non-privileged contributors) won't have. Consider replacing with the actual reason, e.g. "final machine-code output is out of scope; emit assembly instead".

---

📁 scratchv/compiler.py

🔴 Bug: DAG path likely still uses old selector — The diff only shows _generate_riscv_linear updated to conditionally use ExtendedInstructionSelector. If _generate_riscv_dag (line 434 branch) wasn't updated identically, then --extended-isel is silently ignored when --dag-isel is active — contradicting the warning that explicitly mentions this precedence. Either apply the same conditional to the DAG path, or suppress the extended_isel selection there explicitly and log a debug trace so the precedence is observable.

🟡 Default enable_fp64=True is a silent behavior change — Even though extended_isel defaults to False (so the old selector is used), shipping enable_fp64=True as default means any downstream code that checks this config field will assume D-extension availability. If the old selector ever grows D-extension support, this default flips behavior without a migration path. Consider defaulting to False and requiring an explicit opt-in.

🟡 No guard on use_hardware_sqrt=True + enable_fp64=False — _collect_selector_warnings warns about the flags being useless without extended_isel, but it doesn't catch the contradictory combination within extended_isel mode. If enable_fp64=False but use_hardware_sqrt=True, you'd emit fsqrt.s calls while also generating libm fallback for D-precision — confusing mixed behavior. Add a validation (or warning) for this conflict.

💭 Warning message is passive — "have no effect without --extended-isel" doesn't tell the user what to do. Suggest: "add --extended-isel for --no-fp64/--hardware-sqrt to take effect".

---

📁 scratchv/ir/builder.py

🔴 Bug: fcvt_s_d / fcvt_d_s naming is inverted — Lines 305-313: fcvt_s_d produces FLOAT32 (i.e., double→single) but the name reads as single→double. fcvt_d_s produces FLOAT64 (float32→float64) but the name reads as double→single. They are swapped relative to each other.
Suggestion: Either swap the method names, or clarify convention in a docstring (e.g., [dest]_[src] vs [src]_[dest]).

🟡 No dtype parameter or validation on idiv/rem/mod — Lines 245-257: All three hardcode DataType.INT32 but accept arbitrary Value operands. If an INT64 value is passed, this silently emits a mismatched instruction with no warning.
Suggestion: Accept a dtype parameter (defaulting to INT32) or assert the operand dtype matches.

🟡 min/max/abs hardcode default dtype but don't validate operands — Lines 219-237: Same issue as above. If someone passes two FLOAT32 values to min(), it defaults to INT32 dest with no error.
Suggestion: At minimum, add a runtime assert or a comment clarifying the caller's contract.

💭 Builtin shadowing — min, max, abs shadow Python builtins. This is an accepted IR-builder pattern (LLVM bindings do the same), but a one-line comment noting the intent would help future readers.

---

---

⚠️ 未审查的文件

• scratchv/ir/types.py
• scratchv/main.py
• tests/test_extended_isel_cli.py
• tests/test_inst_select_ext.py
• tests/test_riscv_encoder_fd.py
• tests/test_topic28_extended_isel_case_report.py