Impl/topic29

[FeelTheBeats]

No description provided.

[github-actions]

🤖 AI Code Review

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

📁 .github/workflows/ci.yml

🔴 Bug: Summary write order race risk — the vectorize_report.md append is interleaved between const_merge_report.md and github_summary.md. If vectorize_report.md is written but empty (script early-exits), you'll get a blank section. Minor but verify the script always emits a header.

🟡 Missing timeout-minutes — the new run_topic29_vectorize_case.py benchmark has no timeout. Long RV32 execution-equivalence simulations can hang the job indefinitely. Suggest adding timeout-minutes: 10 (or matching whatever const_merge uses) to both new run steps and the pytest step.

🟡 No if: github.ref == 'refs/heads/main' on benchmark but visualization IS gated — the A/B + RV32 equivalence report runs on every PR yet the test visualization page doesn't. If that's intentional (parity with const_merge), fine; otherwise consider gating the benchmark too to save PR CI time.

🟡 Silent failure mode — if [ -f benchmark_reports/vectorize_report.md ] means a missing/failed report is swallowed in the summary step. Since the benchmark step itself fails the job on error, this is probably OK, but a missing file with exit 0 (script bug) would hide regressions. Consider set -e + explicit check, or a warning echo.

💭 N+1 test invocation style — the 4 test files are inlined in one command. If more topic29 tests get added, consider pytest tests/test_vector*.py tests/test_topic29_vectorize_case_report.py or a pytest.ini marker to avoid drift.

💭 Step naming inconsistency — one step is "Run topic29 SIMD-vectorize regressions" (lowercase topic29), the other is "Topic 29 SIMD vectorize case report" (Title Case, space). Trivial, but grep-ability suffers. Pick one convention.

💭 Shell line continuation style — the pytest block uses \ continuations; the benchmark block doesn't. Match the surrounding file's dominant style.

Otherwise the structure (regressions → benchmark → summary) is correct and consistent with the existing const_merge flow.

---

📁 benchmarks/cases/topic29_vectorize_feature.dsl

Code Review

🟡 Comment/code mismatch — Header says relu(a[i] + a[i]) with arrays a/out, but code uses scalars x/y/t. Either align the comment to relu(x + x) or note that array indices are elided in phase-1 grammar, so a future reader doesn't think this is a copy-paste error.

🟡 Loop bound ambiguity — for i = 0, 16 is not obviously exclusive or inclusive. The comment states [0, 16) (16 iterations), but if the DSL treats the upper bound as inclusive (as many educational DSLs do), this compiles to 17 iterations and silently diverges from the report's IR case. Add a one-line note: # upper bound exclusive, 16 iterations or assert the iteration count in the wiring check.

🟡 add(x, x) may be constant-folded — If any compiler pass recognizes add(x, x) as mul(x, 2) (or shl(x, 1)), the two-instruction chain t = add → y = relu collapses to one, and this test no longer exercises the canonical multi-op vectorizable pattern from appendix 5.1. Consider using distinct inputs (add(x, z)) or confirming in the driver that fold isn't enabled for this pass.

💭 Scalar shape is misleading — The shape is scalar-per-element, but the file describes a vectorizable loop. Rename to something like Element-wise relu(2*x), 16 elements to match the report's terminology.

---

📁 docs/topics/29-SIMD向量化-开发文档.md

Review: docs/topics/29-SIMD向量化-开发文档.md

---

🔴 Bug: _VECTOR_OPS 定义位置自相矛盾 — Section 2.3

代码块将 _VECTOR_OPS 放在 class OpCode 定义前（用 OpCode.VLOAD 等成员引用），但紧接着又要求"放在类之后"或"用字符串集合+运行时转换"。两种方案互斥，实施者必然踩坑。
Suggestion: 只保留一种方案（推荐类后定义 + 直接引用成员），删掉另一种。

🔴 Bug: C1 嵌套 FOR 的 _collect_region 未说明配对策略 — Section 4.2/4.4

_collect_region(block, i) 扫描到"配对 ENDFOR"，但若区域内有嵌套 FOR...ENDFOR，直接找下一个 ENDFOR 会先闭合内层。当前描述"不匹配则整块放弃"不覆盖此场景。
Suggestion: 明确 _collect_region 必须做嵌套计数（遇到 FOR 计数+1、ENDFOR 计数-1，计为 0 时停），否则 C1 的"区域内出现 FOR"判定和 region 边界都不可靠。

🟡 Semantic: C5 死代码复用 REASON_UNSUPPORTED_OP — Section 4.4

"分类为 ELEM/CONST 但结果未被任何 STORE 使用 → REASON_UNSUPPORTED_OP"——死代码混入不是"不支持的操作"，而是无用指令。用同名 reason 会导致测试断言语义模糊，后续排查困难。
Suggestion: 新增 REASON_DEAD_CODE（或 REASON_UNUSED_RESULT），或在文档中明确这是有意的复用并加注。

🟡 Doc-Code 偏差: VRELU 展开方式未同步 — Section 5.2 vs 实现结果

Section 5.2 写的是 MAX vd_k, va_k, MachineOperand.immediate(0)，但实现结果说明"VRELU 使用 zero 寄存器"（即 MAX vd_k, va_k, x0）。同理，lane 地址实际用 ADDI a1, addr, 4k + lw a1, 0(a1) 而非文档描述的 LW vd_k, (a_k) 直接寻址。
Suggestion: 更新 5.2 到实现匹配的最终形态，或在实现结果部分标注"文档已同步"的修订日期。

🟡 Test ambiguity: test_strip_mining_no_remainder 的向量 op 计数 — Section 10.1

断言写 2×VLOAD+VMUL+VRELU+VSTORE——按 _make_map_loop（双输入 base），这是 2 VLOAD + 1 VMUL + 1 VRELU + 1 VSTORE = 5 ops，但写法可能被解读为 "2×(VLOAD+VMUL+VRELU+VSTORE)" = 8 ops。
Suggestion: 改为 1 VLOAD+1 VLOAD+1 VMUL+1 VRELU+1 VSTORE 或写全 5 vector ops 避免歧义。

💭 Nit: _fresh() 命名无碰撞保护 — Section 4.1/4.3

_fresh(prefix="v") 生成 v_1, v_2, ...，但未说明与区域外已有同名 Value 冲突时的行为。如果原 IR 中有手动命名的 v_1，新生成值会重名。
Suggestion: 要么 _fresh 检查已存在名，要么文档中声明"原 IR 中不应有 v_N 模式命名"（并在测试中约束）。

💭 Nit: Section 8.2 ISA 拒绝时机

vector_isa != "scalar" 的拒绝在 IR dump 之后、codegen 之前。用户用 --vector-isa v 时会看到向量化 IR dump 但编译失败——debug 友好，但可能困惑。
Suggestion: 可考虑在 parse 阶段提前拒绝，或至少在文档中标注这是有意设计。

---

总结: 文档本身质量极高（848 行覆盖接口契约→风险回退→二期依赖全链条），实现结果与评审修复记录完整。上述 🔴 两项是实施时会踩的坑（尤其是 _collect_region 嵌套处理），🟡 三项是文档同步债务。建议优先修复 🔴 后再作为实施基线。

---

📁 scratchv/backend/riscv_encoder.py

🔴 Bug: Incomplete regex misses many vector mnemonics — vmslt, vmsle, vmseq, vmsne, vmsgt, vmsgte, vsadd, vssub, vsra, vsll, vsrl, vandn, vcls, vclz, vclo, vpfirst, vfirst, vpopc are not matched. These would fall through to a generic unknown-instruction error, defeating the purpose of the guard.
Suggestion: Either enumerate all RISC-V vector mnemonics, or simplify to re.compile(r"^v[a-z]") — if any v-prefixed instruction reaches this encoder it's a phase-1 violation, so a broad catch is safer and future-proof.

🟡 Redundant alternatives — set already prefix-matches setvli and setivli; slide covers slideup/slidedown; redsum covers redsumu. These longer forms are never reached.
Suggestion: Trim to the shortest prefixes, or document that redundancy is intentional for readability.

💭 No $ anchor — ^v(...) matches any v-prefixed token containing one of those fragments, which could theoretically misattribute an unknown non-vector instruction. Low risk in practice, but a $-anchored or \b-terminated match would be more precise.

---

📁 scratchv/backend/vector_scalar.py

🔴 Scratch register a1 invariant is fragile — The entire lowering depends on a1 never being allocated to a vreg or used for spills (lines 22–26, 104). If machine_types.ALL_REGS or the spilling logic ever changes, lane addresses silently corrupt. Consider adding a hard assertion in begin_function or the register allocator that verifies _ADDR_SCRATCH is not in the allocatable set, and/or emitting it from the register allocator module so the dependency is structurally enforced.

🟡 No width validation — _width() (line 87) returns whatever int(raw) produces. A malformed attr like width: 0 or width: -1 yields an empty or nonsensical lowering with no error. Clamp to max(1, width) or raise VectorLoweringError on invalid values.

🟡 Silent scalar broadcast in _lanes_of (line 101–102) — A non-vector, non-constant operand falls through to return [MachineOperand.vreg(value.name)] * width, implicitly broadcasting a scalar across all lanes. This masks IR-level bugs where a scalar is accidentally passed to a vector operation. Consider raising VectorLoweringError unless the calling op explicitly supports scalar-vector semantics (e.g., only allow for _expand_binary).

💭 Redundant constant materialization — Every _lanes_of call on a constant emits a fresh LI (line 99). If the same constant appears in multiple ops, it's emitted repeatedly. A simple dict[int, MachineOperand] cache keyed on the constant value would reduce code size.

💭 _lanes_of returns mutable internal state (line 95) — The bound list is returned by reference from self._lanes. No current caller mutates it, but a future caller could silently corrupt the stored lane binding. Consider returning list(bound) (shallow copy) to make the API safe by default.

---

📁 scratchv/compiler.py

🔴 Bug: vector_isa validation is too late — Step 4 block (line ~361): vector_isa is only rejected after parsing, IR gen, optimization, and vectorization have all run. Move this check into the upfront validation block alongside vectorize/vector_width/backend. Currently a user passes --vector-isa p and wastes cycles on a doomed compilation.

# Move into the upfront block (top of compile()):
if self.config.vector_isa != "scalar":
    return CompileResult(
        success=False,
        errors=[
            f"vector-ISA '{self.config.vector_isa}' is not "
            "implemented (phase 2); use --vector-isa scalar"
        ],
    )

Then delete the step-4 block entirely (it's now unreachable after the move, since vectorize is always true there).

🟡 Side effect: config mutation in compile() — Lines ~330-340: self.config.use_dag_isel = False and self.config.reg_alloc = "greedy" permanently mutate the config object. If the same CompilerConfig is reused across multiple compile() calls, the second call will silently skip DAG isel and linear-scan. Consider restoring original values after use, or working on a shallow copy:

saved_dag = self.config.use_dag_isel
saved_alloc = self.config.reg_alloc
try:
    ...
finally:
    self.config.use_dag_isel = saved_dag
    self.config.reg_alloc = saved_alloc

🟡 Narrow exception catch — Upfront validation catches only ValueError. If vector_width is passed a non-int (e.g., a string from CLI), validate_vector_width may raise TypeError. Suggest broadening to (ValueError, TypeError) or wrapping the whole upfront block in a broader catch that returns a CompileResult failure.

💭 Docstring / reality mismatch — Docstring says vector_isa is "rejected" for "p"/"v", but the rejection happens at step 4, not at config time. Once the upfront validation is moved (per 🔴 above), this becomes accurate.

---

📁 scratchv/ir/builder.py

🔴 Bug: vload dest dtype is wrong — Line ~215: dest.dtype = addr.dtype propagates the pointer dtype. The dest should reflect the element type derived from elem_bytes, not the address type. If downstream passes rely on dest.dtype to emit correct scalar arithmetic, this silently produces mismatched types.

🟡 No parameter validation on width / elem_bytes / align — Passing width=0, negative align, or elem_bytes that doesn't match any supported element type will silently emit invalid IR. Consider adding a guard in the builder:

if width <= 0:
    raise ValueError(f"width must be > 0, got {width}")

🟡 No operand compatibility check in _emit_vector_binary — If lhs.shape is (4,) and rhs.shape is (8,), the IR is emitted with width=w from lhs but rhs is actually wider. Consider:

if _shape_width(lhs) != _shape_width(rhs):
    raise ValueError(f"vector width mismatch: {_shape_width(lhs)} vs {_shape_width(rhs)}")

🟡 Missing elem_bytes/align in binary & unary ops — vload/vstore accept and pass elem_bytes and align, but _emit_vector_binary and vrelu don't. If the IR consumer needs these for packed layouts or alignment scheduling, the metadata is lost for non-mem ops.

💭 _shape_width is module-level but only used inside the class — If it's only consumed by Builder, consider making it a private class method (_shape_width) to keep it co-located and avoid accidental external use.

💭 vstore return type annotation is Instruction but others return Value — This is likely correct (stores are void), but worth a comment since it breaks the pattern of all other methods returning Value.

---

📁 scratchv/ir/types.py

🟡 _VECTOR_OPS placement — The frozenset sits between OpCode and DataType class definitions, breaking module-level flow. Consider moving it either above OpCode (if it's a constant) or below all class definitions.

# Current:
class OpCode(enum.Enum):
    ...

_VECTOR_OPS = frozenset({...})   # ← orphaned between classes

class DataType(enum.Enum):
    ...

# Suggestion: place at top of file (after imports) or after all classes

🟡 Pattern inconsistency — is_arith and is_control_flow use inline tuple membership; is_vector uses a module-level frozenset. Frozenset is arguably better for O(1) lookup, but the three methods should follow the same pattern. Either promote all to frozensets, or inline the vector set for consistency.

# Existing style:
def is_arith(self):
    return self in (OpCode.ADD, OpCode.SUB, ...)

# New style:
def is_vector(self):
    return self in _VECTOR_OPS

💭 Docstring inconsistency — is_vector has a docstring; is_arith / is_control_flow don't. Minor, but pick one convention.

---

All opcodes are correctly covered in the frozenset (8/8). No correctness or security issues. The partition comments for Topic 28/29 are a nice touch for future maintainability.

---

📁 scratchv/main.py

🔴 Security/Input: --vector-isa accepts invalid values without early rejection — Lines 107–109: The parser permits "p" and "v" but the help says they're "rejected until phase 2." No argparse-level guard is shown, so if the rejection happens downstream, users get a confusing error far from the CLI boundary. Suggestion: either reject at parse time (custom type= or a parser.error callback) or change the choices to only ["scalar"] and remove the others until phase 2 is ready.

🟡 Missing cross-flag validation — --vectorize, --vector-width, and --vector-isa interact with each other, but no validation is visible. Consider checking that --vector-width is ignored/validated only when --vectorize is set, and that incompatible flags (e.g., --vectorize with something that disables vectorization) produce a clear error.

💭 Nit — choices=[2, 4] is hardcoded; a type=lambda x: _parse_width(x) or at minimum a module-level constant would make extending to width 8 in phase 2 cleaner.

---

📁 tests/test_backend.py

🔴 **Bug: Inverted assertion** — `test_vectorize_linear_regalloc_falls_back_to_greedy`:
   `assert ".label" not in result.output_text` is backwards. The test name says
   it falls back to greedy, which *should* produce labels. After the fallback,
   you want labels present. As written, this test passes when the fallback
   **doesn't** work and fails when it does.
   Suggestion: `assert ".label" in result.output_text` (or remove if labels
   aren't expected in this specific IR).

🟡 **Brittle string assertion** — `test_compile_vectorized_program`:
   `assert "vectorized 1/1 loop(s), width=2" in result.stats["opt_message"]`
   will break on any cosmetic format change.
   Suggestion: `assert "vectorized" in ... and "width=2" in ...`

🟡 **Weak assertion** — `test_no_rtype_immediate`:
   `assert "li" in asm` only checks `li` appears *somewhere*. It wouldn't catch
   a regression that silently drops one constant.
   Suggestion: Assert the count or check specific instructions, e.g.
   `asm.count("li") >= 3` (one per constant use).

🟡 **Duplicated setup** — All 5 `TestVectorDriverIntegration` tests repeat
   `monkeypatch` + `tmp_path` + `source.write_text` + `driver.compile`.
   Suggestion: Extract to a fixture (e.g. `@pytest.fixture def vector_compile(monkeypatch, tmp_path):`)
   taking `config` as a parameter. Reduces the diff surface for future changes.

💭 **Missing CLI-level width validation test** — `test_vectorize_invalid_width_rejected`
   tests `vector_width=0` via `CompilerConfig` directly, but there's no test for
   `--vector-width 0` or negative values through the CLI. If `args_to_config`
   lacks validation, a bad width could reach `CompilerConfig` uncaught.

---

---

⚠️ 未审查的文件

• tests/test_topic29_vectorize_case_report.py
• tests/test_vector_encoder.py
• tests/test_vector_lowering.py
• tests/test_vectorize.py