Impl/topic06

[FeelTheBeats]

No description provided.

[github-actions]

🤖 AI Code Review

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

📁 .github/workflows/ci.yml

🔴 Missing --junit-xml — test results 不会进入流水线聚合报告

现有的 test step 使用 --junit-xml=benchmark_reports/test_results.xml，新加的 step 没有。这些测试结果不会被任何后续报告步骤消费。

python3.12 -m pytest \
  tests/test_dsl_suite.py \
  tests/test_suite_report_contract.py \
  -v --tb=short \
  --junit-xml=benchmark_reports/dsl_suite_results.xml

🟡 "硬门禁" 语义矛盾 — 注释写"硬门禁，xfail 不红灯"。如果 xfail 失败也不阻断构建，它不是硬门禁。确认意图：是要真正 gate（则去掉 xfail），还是只是 best-effort report（则去掉"硬门禁"措辞，避免未来维护者误判）。

🟡 dsl_suite.json 只写入文件，没有后续消费 — 报告生成了 JSON + MD，但 MD 只进了 step summary。JSON 是否应该上传为 artifact 供 PR diff / 趋势对比使用？对比同文件里其他 benchmark report 的上传逻辑。

💭 Step summary 插入位置 — 新块插在 regalloc_bench 之后，如果 regalloc_bench.md 不存在，前面已有的 echo "" >> $GITHUB_STEP_SUMMARY 可能产生连续空行。可考虑去掉冗余空行，或者把条件也包在 if 里。

---

📁 benchmarks/cases/001_simple_add.meta.json

Code Review

🔴 Bug: Missing inputs — inputs is {}, but the description says "addition of two vectors" and expected_return is [2.0, 4.0, 6.0, 8.0] (implying two vectors of length 4). The benchmark runner has nothing to feed into the oracle — this case is unexecutable. Likely intended something like:

"inputs": { "a": [1.0, 2.0, 3.0, 4.0], "b": [1.0, 2.0, 3.0, 4.0] }

🔴 Bug: max_instructions: 3 seems too low — A 4-element vector addition requires at minimum 4 scalar adds, or 1 vector add depending on the IR. Either way, 3 is likely to cause a false failure. Verify against the instruction model used by the runner.

🟡 Suggestion: Add input_shapes or equivalent metadata — Other benchmark cases in this repo (if they exist) likely specify input shapes/types. Without them, tooling that validates type consistency or generates random inputs can't consume this case.

💭 Nit — The file is named 001_simple_add.meta.json; ensure the accompanying source file (e.g., 001_simple_add.source.slang, .tvm, etc.) follows the same naming convention and lives in the expected directory. No way to confirm from this diff alone.

---

📁 benchmarks/cases/002_simple_mul.meta.json

🔴 Bug: Empty inputs — Line 7: "inputs": {} provides no data for the benchmark runner to execute. If this case intends to multiply [1,2,3,4] by itself, the inputs should be explicitly declared (or the file is non-functional as a standalone test case).

🟡 Suggestion: Description ambiguity — Line 3: "Element-wise multiplication of two vectors" implies distinct operands, but expected_return is [1,4,9,16] (i.e. [1,2,3,4]²). Either clarify it's squaring, or change expected output to reflect two genuinely different vectors.

💭 Nit: max_instructions — Line 14: Is 3 sufficient for a 4-element vector multiplication? Verify against the target ISA — if each multiply is one instruction per element, you may need at least 4. Confirm this constant aligns with the architecture being benchmarked.

---

📁 benchmarks/cases/003_sub_div.meta.json

🟡 All-zero expected_return lacks context — If any operation involves division by zero, the expected 0.0 behavior should be documented (comment, field, or linked spec). A reader can't tell if 0.0 means "division produced zero" or "division by zero returns zero by convention." This will confuse anyone debugging a mismatch.

🟡 Empty inputs with no explanation — For a case described as "Chained subtraction and division," having no inputs is suspicious. Either the program is self-contained (hardcoded operands), in which case the expected values are only verifiable by inspecting the referenced program file — or there's a missing inputs field. Consider adding a "note" or "instructions" field that clarifies where the operands come from.

💭 max_instructions: 4 — This is an upper bound, not a count. If the program under test is a fixed sequence, consider also recording expected_instructions for tighter regression detection.

---

📁 benchmarks/cases/004_relu.meta.json

🔴 Missing test cases for negative/zero inputs — ReLU's core behavior is max(0, x). expected_return: [1,2,3,4] all positive means the benchmark never actually validates the rectification step. Add cases with negatives (e.g. input -1 → expected 0.0) to ensure correctness.

🔴 Empty inputs: {} — If inputs is empty, the benchmark harness has no data to feed ReLU. Are inputs derived elsewhere (e.g. from expected_return by identity)? If so, document it. If not, populate it explicitly — e.g. {"x": [-1, 0, 1, 2, 3, 4]} with matching expected_return: [0.0, 0.0, 1.0, 2.0, 3.0, 4.0].

🟡 Consider adding a zero edge case — ReLU(0) = 0 is a boundary condition worth pinning in the expected output to catch off-by-one or sign errors.

💭 max_instructions: 3 seems tight but reasonable for a single-activation benchmark — just confirm it's sufficient if negative-value tests are added (they might require an extra branch/compare instruction).

---

📁 benchmarks/cases/005_gelu.meta.json

🔴 **Bug: Empty inputs make test unverifiable** — Line 8:
   `"inputs": {}` is an empty object, but `expected_return` has 4 values.
   Without explicit inputs, the test case is not self-contained and its
   correctness cannot be independently verified. If defaults are injected by
   the harness, document this or inline the inputs (e.g., `[0, 1, 2, 3, 4]`).

🟡 **Missing tolerance for float comparison** — Line 9–13:
   Floating-point results require a tolerance field (e.g.,
   `"tolerance": 1e-5`). Without it, consumers must guess the acceptable
   deviation, leading to flaky comparisons.

🟡 **`max_instructions: 5` may be too tight for GELU** — Line 14:
   GELU requires erf/tanh evaluation. Depending on the backend (e.g., if
   tanh is decomposed into exp operations), 5 instructions may exclude
   valid correct implementations. Consider whether this limit was measured
   against a reference compiler or is aspirational.

💭 **Precision inconsistency** — Line 12:
   `3.99993` has 4 decimal places; other values have 6. Normalize to
   `"3.999968"` (or whichever the authoritative value is) for consistency.

💭 **Consider adding `"expected_inputs"` to clarify contract** —
   Even if inputs are defined externally, a comment or companion field
   stating what inputs these expected values correspond to would aid
   debugging when this test fails.

---

📁 benchmarks/cases/006_softmax.meta.json

🔴 Missing input definition — "inputs": {} 是空的，但 expected_return 的值对应 softmax([1, 2, 3, 4])。没有指定输入，这个 benchmark case 无法正确执行。

Suggestion: 显式定义输入，例如：

"inputs": {
  "vector": [1, 2, 3, 4]
}

并确认框架的 input 引用方式与 runner 一致。

🟡 No numerical tolerance — 浮点期望值精度不一致（7~9 位小数），且未指定比较容差（atol/rtol）。如果 runner 缺少 tolerance 字段，不同实现可能因精度差异误判 pass/fail。

Suggestion: 添加 "tolerance": 1e-5 或框架约定的容差字段，并与同目录其他 case 保持一致。

💭 Input 格式与目录其他 case 不一致？ — 确认此文件的 inputs schema 与 benchmarks/cases/001_* 等兄弟文件的格式对齐，避免 runner 解析分歧。

---

📁 benchmarks/cases/007_matmul.meta.json

🔴 inputs 为空 — 矩阵乘法测试没有定义输入矩阵，用例不可复现且无法正确验证。应提供具体的 2x2 输入矩阵。

🔴 expected_return 为标量 30.0 — 两个 2x2 矩阵相乘的结果是 2x2 矩阵，不可能是单个标量。除非 schema 定义为"所有元素之和"，否则此值类型与语义不匹配。

🟡 max_instructions: 3 — 2x2 矩阵乘法至少需要 8 次乘加（或 4 次点积）。若 ISA 无内置 matmul 指令，此上限过紧会导致误判；若有，请确认 schema 文档说明。

💭 description 提到 "2x2" 但无实际矩阵定义 — 描述与数据不一致，读者无法从文件本身理解测试意图。

---

📁 benchmarks/cases/008_dot.meta.json

🔴 Bug: Empty inputs — "inputs": {} but dot product requires two length-4 vectors. The benchmark cannot be executed without input definitions.

🟡 Missing documentation of expected inputs — "expected_return": 30.0 is hardcoded with no indication of what input vectors produce this result (e.g., [1,2,3,4]·[1,2,3,4] = 30). Makes it impossible to verify correctness or understand what's being tested.

💭 max_instructions: 3 — Consider a brief comment on why this budget was chosen (e.g., "dot product of length-4 should be fusible into a single kernel call + unpacks").

---

📁 benchmarks/cases/009_maxpool.meta.json

🔴 inputs is empty — expected_return specifies [2.0, 4.0] for a MaxPool with kernel=2, stride=2, but no inputs are defined. If inputs are not defined elsewhere (e.g., a shared fixture or convention), this test has no way to produce the expected output.

🟡 xfail.reason is oversized — The reason string reads like a commit message with implementation detail (first bad line: bnez a1 # .Lmp_gt_1, specific emitter behavior). This belongs in the tracking issue or changelog, not inline in test metadata. Keep the reason to one or two sentences pointing to the issue; put the detail in a ticket.

💭 strict: false on xfail — Non-strict means this test silently passes if it starts succeeding, without anyone noticing. Consider strict: true so it fails CI when the backend bug is fixed, forcing someone to remove the xfail block.

---

---

⚠️ 未审查的文件

• benchmarks/cases/010_exp_neg.meta.json
• benchmarks/cases/011_multi_op_chain.meta.json
• benchmarks/cases/012_nn_pipeline.meta.json
• benchmarks/cases/013_for_sum.meta.json
• benchmarks/cases/014_for_dot.meta.json
• benchmarks/cases/015_for_relu.meta.json
• benchmarks/cases/016_if_simple.meta.json
• benchmarks/cases/017_while_sum.dsl
• benchmarks/cases/017_while_sum.meta.json
• benchmarks/cases/018_nested_if.meta.json
• benchmarks/cases/019_nested_loop.meta.json
• benchmarks/cases/020_constant_propagation.meta.json
• benchmarks/cases/021_dsl_if_else.meta.json
• benchmarks/cases/022_dsl_while_sum.dsl
• benchmarks/cases/022_dsl_while_sum.meta.json
• benchmarks/cases/023_large_chain.meta.json
• benchmarks/dsl_suite.py
• benchmarks/run_suite.py
• docs/topics/06-性能基准套件-开发文档.md
• docs/topics/06-性能基准套件-设计文档.md