perf(cpu): vectorize the GDN depthwise causal convolution on AArch64 - #693
perf(cpu): vectorize the GDN depthwise causal convolution on AArch64#693Aharrypotter wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThe PR adds Qwen3.5 benchmark execution and telemetry validation, minimum-token controls across generation APIs, and an AArch64 NEON optimization for four-tap GDN convolution. It also adds focused GoogleTest coverage and CMake targets. ChangesQwen3.5 benchmark tooling
Minimum-token generation
AArch64 GDN convolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant runner as qwen3_5/main.cpp
participant engine as mllm engine
participant harness as benchmark_harness.hpp
participant jsonl as JSONL output
runner->>engine: reset and generate benchmark request
runner->>harness: capture telemetry
harness-->>runner: telemetry snapshot
runner->>harness: validate required and stable telemetry
runner->>jsonl: write measured result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
mllm/models/ARGeneration.hpp (1)
83-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
min_new_tokensargument contract.
min_new_tokensis a caller-visibleARGenerationArgskey. Document its bounds, EOS behavior, andstd::invalid_argumentconditions at the publicgenerate,streamGenerate, andchatdeclarations.As per coding guidelines, “Ensure public APIs, classes, and functions have clear docstrings or comments explaining purpose, parameters, returns, and errors.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mllm/models/ARGeneration.hpp` at line 83, Document the min_new_tokens contract in the public generate, streamGenerate, and chat declarations: state its valid bounds, how it affects EOS handling, and when std::invalid_argument is thrown. Ensure the documentation clearly identifies min_new_tokens as an ARGenerationArgs key without changing implementation behavior.Source: Coding guidelines
tests/core/ARGenerationTest.cpp (1)
209-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest invalid bounds in batch and streaming generation.
These tests only exercise
ARGenerationChatIterator. AddgenerateandstreamGeneratecases formin_new_tokens > max_length, negativemin_new_tokens, and non-positivemax_length. Both methods have independent validation code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/ARGenerationTest.cpp` around lines 209 - 229, The current invalid-bound tests cover only ARGenerationChatIterator; extend the test coverage for the generate and streamGenerate APIs, including min_new_tokens greater than max_length, negative min_new_tokens, and non-positive max_length. Add cases using the existing test fixtures/helpers and assert each API rejects these inputs, preserving the independent validation paths in generate and streamGenerate.tools/gdn-h1a-4b-r4/test_harness.py (2)
222-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse unpacking instead of list concatenation.
Ruff reports RUF005 on this line.
♻️ Proposed fix
- path.write_text("\n".join(lines + [lines[-1]]) + "\n", encoding="utf-8") + path.write_text("\n".join([*lines, lines[-1]]) + "\n", encoding="utf-8")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/gdn-h1a-4b-r4/test_harness.py` at line 222, Update the write_text construction in the test harness to use iterable unpacking when appending the final line, replacing the list concatenation that triggers Ruff RUF005 while preserving the existing output and encoding.Source: Linters/SAST tools
156-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the remaining dispositions and the controller validators.
The suite covers
PROMOTE_H1A,REJECT_CORRECTNESS,HARNESS_REHEARSAL_VALID, and manyContractErrorpaths. Two disposition branches stay untested:SCREENING_BLOCKED(mixed ceiling regimes or an unstablep95_over_median) andRETAIN_INCUMBENT(a candidate that does not improve). Both drive promotion decisions.controller.parse_promptandcontroller.validate_argsare also untested, so the prompt-format and frozen-contract rules have no coverage.Suggested cases:
- A candidate with
prefill_duration_usequal to the baseline yieldsRETAIN_INCUMBENT.- A record with a differing
telemetry_before.ceiling_vectoracross positions yieldsSCREENING_BLOCKED.controller.parse_prompt("bad-format")raisesargparse.ArgumentTypeError.controller.validate_argsrejectsthreads != 8.Do you want me to generate these tests?
Also applies to: 241-251
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/gdn-h1a-4b-r4/test_harness.py` around lines 156 - 159, Add tests in test_harness.py covering the untested analyzer dispositions: verify equal candidate and baseline prefill duration returns RETAIN_INCUMBENT, and differing telemetry_before.ceiling_vector values across positions return SCREENING_BLOCKED. Add controller validator tests confirming parse_prompt("bad-format") raises argparse.ArgumentTypeError and validate_args rejects threads values other than 8, using the existing fixture and assertion patterns.examples/qwen3_5/benchmark_harness.hpp (1)
78-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
cpu0online assumption.Line 81 reports
online = 1for CPU 0 without readingsysfs. Most kernels do not expose/sys/devices/system/cpu/cpu0/onlinebecause CPU 0 cannot be offlined. The hard-coded value is therefore correct in practice, but the reason is not visible. Add a short comment so the special case is not read as a validation bypass.♻️ Proposed comment
for (const int cpu : affinity) { const auto base = std::filesystem::path("/sys/devices/system/cpu") / ("cpu" + std::to_string(cpu)); const auto cpufreq = base / "cpufreq"; + // Most kernels omit cpu0/online because CPU 0 cannot be offlined, so report it as online. const auto online = cpu == 0 ? std::optional<int64_t>(1) : readIntegerFile(base / "online");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/qwen3_5/benchmark_harness.hpp` around lines 78 - 95, Add a short explanatory comment immediately before the `online` initialization in the CPU snapshot loop, documenting that CPU 0 cannot be offlined and therefore commonly lacks a sysfs `online` file; preserve the existing `cpu == 0` special case and all other behavior.tests/core/Qwen35BenchmarkHarnessTest.cpp (2)
76-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test passes without assertions on non-Apple platforms.
The whole body is inside
#if defined(__APPLE__). On Linux and Android CI the test reports success and checks nothing. UseGTEST_SKIP()so the report shows that the case did not run.♻️ Proposed fix
TEST(Qwen35BenchmarkHarnessTest, MacCaptureFailsClosedWhenTelemetryIsRequired) { `#if` defined(__APPLE__) const auto errors = mllm::examples::qwen3_5::benchmark::validateRequiredTelemetry(mllm::examples::qwen3_5::benchmark::captureTelemetry()); EXPECT_NE(std::find(errors.begin(), errors.end(), "unsupported_telemetry_platform"), errors.end()); +#else + GTEST_SKIP() << "macOS-only telemetry rejection check"; `#endif` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/Qwen35BenchmarkHarnessTest.cpp` around lines 76 - 82, Update Qwen35BenchmarkHarnessTest.MacCaptureFailsClosedWhenTelemetryIsRequired so non-Apple builds call GTEST_SKIP() instead of completing without assertions; keep the existing telemetry validation assertion unchanged under __APPLE__.
4-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude
<algorithm>forstd::find.Lines 60 and 80 call
std::find. The file includes only<gtest/gtest.h>andbenchmark_harness.hpp.benchmark_harness.hppincludes<algorithm>, so the build succeeds today, but the dependency is indirect. Include the header directly.♻️ Proposed fix
`#include` <gtest/gtest.h> +#include <algorithm> + `#include` "benchmark_harness.hpp"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/Qwen35BenchmarkHarnessTest.cpp` around lines 4 - 6, Update Qwen35BenchmarkHarnessTest.cpp to include the standard <algorithm> header directly, alongside its existing includes, because the test uses std::find; do not rely on benchmark_harness.hpp’s transitive include.tools/gdn-h1a-4b-r4/controller.py (2)
83-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--cpusetand--threadsare options that only one value passes.Line 102 rejects every value other than
8and"0-7", which are also the defaults. The options therefore cannot change behaviour. Either remove them and use module constants, or keep them and state in the help text that the r4 contract freezes both values.♻️ Proposed refactor
- parser.add_argument("--cpuset", default="0-7") - parser.add_argument("--threads", type=int, default=8) + parser.add_argument("--cpuset", default=REQUIRED_CPUSET, help=f"frozen by the r4 contract to {REQUIRED_CPUSET}") + parser.add_argument("--threads", type=int, default=REQUIRED_THREADS, + help=f"frozen by the r4 contract to {REQUIRED_THREADS}")Also applies to: 102-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/gdn-h1a-4b-r4/controller.py` around lines 83 - 84, Update the argument parsing and validation around --cpuset and --threads so these options can either meaningfully configure behavior or are removed in favor of module constants. If retaining them, remove the unconditional rejection of values other than "0-7" and 8, and document in their help text that the r4 contract intentionally freezes those values.
150-151: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHard-coded token counts in
controller.pyandanalyze.py. Both programs repeat the literals64and63even though the run contract already recordsgenerated_tokensanddecode_steps. The producer and the consumer can therefore drift apart, and the analyzer would reject every position with "wrong token denominator" after a contract change.
tools/gdn-h1a-4b-r4/controller.py#L150-L151: passstr(contract["generated_tokens"])to--max_new_tokens, and derivedecode_stepsfrom the same constant at Lines 238-239.tools/gdn-h1a-4b-r4/analyze.py#L139-L146: comparestats["generated_tokens"]andstats["decode_steps"]againstcontract["generated_tokens"]andcontract["decode_steps"], and dividedecode_duration_usbycontract["decode_steps"].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/gdn-h1a-4b-r4/controller.py` around lines 150 - 151, The token-count contract is duplicated as hard-coded 64/63 values across the producer and analyzer. In tools/gdn-h1a-4b-r4/controller.py lines 150-151, pass contract["generated_tokens"] to --max_new_tokens and derive decode_steps from that same contract value at lines 238-239; in tools/gdn-h1a-4b-r4/analyze.py lines 139-146, validate stats["generated_tokens"] and stats["decode_steps"] against the corresponding contract fields and divide decode_duration_us by contract["decode_steps"].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/qwen3_5/main.cpp`:
- Around line 100-104: Update the benchmark_jsonl validation around jsonl_path
to use error_code-aware filesystem checks, inspect size_error immediately after
file_size, and distinguish filesystem failures from the non-empty-file case.
Preserve the invalid_argument rejection only when the path exists and has
nonzero size, while reporting filesystem errors through the existing
error-handling approach.
- Around line 81-83: Update the benchmark_mode detection expression in main to
include benchmark_warmup.isSet(), ensuring that using only --benchmark_warmup
enters benchmark mode and triggers the existing required-option validation.
In `@mllm/models/ARGeneration.cpp`:
- Around line 35-41: Validate temperature in all three generation entry points
before sampling, rejecting non-finite or non-positive values so invalid inputs
cannot reach sampleTemperature; update the relevant validation paths around the
existing max_length/min_new_tokens checks. Add a regression test covering
min_new_tokens greater than 1 with temperature set to -1.0F.
In `@tests/core/CMakeLists.txt`:
- Around line 9-12: Register Mllm-Test-Core-Qwen35BenchmarkHarness with CTest
using the same gtest_discover_tests pattern already used by the other GTest core
targets. Add the discovery call after the target setup so the new executable is
included in the test framework.
In `@tests/cpu/Qwen35GDNConvTest.cpp`:
- Around line 13-17: Update Qwen35GDNConvTest.cpp to include the standard
headers <algorithm> and <stdexcept> directly, covering its uses of std::fill and
std::invalid_argument rather than relying on transitive includes.
In `@tools/gdn-h1a-4b-r4/analyze.py`:
- Around line 268-274: Update the exception handling in main around the
analyze(args.result_root) call to catch general Exception in addition to
ContractError, converting any unexpected analysis failure into the same
HARNESS_REJECTED result with the exception message in errors. Preserve the
existing output-file writing, disposition printing, and return-code logic for
both handled error types.
---
Nitpick comments:
In `@examples/qwen3_5/benchmark_harness.hpp`:
- Around line 78-95: Add a short explanatory comment immediately before the
`online` initialization in the CPU snapshot loop, documenting that CPU 0 cannot
be offlined and therefore commonly lacks a sysfs `online` file; preserve the
existing `cpu == 0` special case and all other behavior.
In `@mllm/models/ARGeneration.hpp`:
- Line 83: Document the min_new_tokens contract in the public generate,
streamGenerate, and chat declarations: state its valid bounds, how it affects
EOS handling, and when std::invalid_argument is thrown. Ensure the documentation
clearly identifies min_new_tokens as an ARGenerationArgs key without changing
implementation behavior.
In `@tests/core/ARGenerationTest.cpp`:
- Around line 209-229: The current invalid-bound tests cover only
ARGenerationChatIterator; extend the test coverage for the generate and
streamGenerate APIs, including min_new_tokens greater than max_length, negative
min_new_tokens, and non-positive max_length. Add cases using the existing test
fixtures/helpers and assert each API rejects these inputs, preserving the
independent validation paths in generate and streamGenerate.
In `@tests/core/Qwen35BenchmarkHarnessTest.cpp`:
- Around line 76-82: Update
Qwen35BenchmarkHarnessTest.MacCaptureFailsClosedWhenTelemetryIsRequired so
non-Apple builds call GTEST_SKIP() instead of completing without assertions;
keep the existing telemetry validation assertion unchanged under __APPLE__.
- Around line 4-6: Update Qwen35BenchmarkHarnessTest.cpp to include the standard
<algorithm> header directly, alongside its existing includes, because the test
uses std::find; do not rely on benchmark_harness.hpp’s transitive include.
In `@tools/gdn-h1a-4b-r4/controller.py`:
- Around line 83-84: Update the argument parsing and validation around --cpuset
and --threads so these options can either meaningfully configure behavior or are
removed in favor of module constants. If retaining them, remove the
unconditional rejection of values other than "0-7" and 8, and document in their
help text that the r4 contract intentionally freezes those values.
- Around line 150-151: The token-count contract is duplicated as hard-coded
64/63 values across the producer and analyzer. In
tools/gdn-h1a-4b-r4/controller.py lines 150-151, pass
contract["generated_tokens"] to --max_new_tokens and derive decode_steps from
that same contract value at lines 238-239; in tools/gdn-h1a-4b-r4/analyze.py
lines 139-146, validate stats["generated_tokens"] and stats["decode_steps"]
against the corresponding contract fields and divide decode_duration_us by
contract["decode_steps"].
In `@tools/gdn-h1a-4b-r4/test_harness.py`:
- Line 222: Update the write_text construction in the test harness to use
iterable unpacking when appending the final line, replacing the list
concatenation that triggers Ruff RUF005 while preserving the existing output and
encoding.
- Around line 156-159: Add tests in test_harness.py covering the untested
analyzer dispositions: verify equal candidate and baseline prefill duration
returns RETAIN_INCUMBENT, and differing telemetry_before.ceiling_vector values
across positions return SCREENING_BLOCKED. Add controller validator tests
confirming parse_prompt("bad-format") raises argparse.ArgumentTypeError and
validate_args rejects threads values other than 8, using the existing fixture
and assertion patterns.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 21f9c523-4c60-4b88-9b6d-e9fbce140748
📒 Files selected for processing (13)
examples/qwen3_5/benchmark_harness.hppexamples/qwen3_5/main.cppmllm/backends/cpu/kernels/common/gdn/gated_delta_net.cppmllm/models/ARGeneration.cppmllm/models/ARGeneration.hpptests/core/ARGenerationTest.cpptests/core/CMakeLists.txttests/core/Qwen35BenchmarkHarnessTest.cpptests/cpu/CMakeLists.txttests/cpu/Qwen35GDNConvTest.cpptools/gdn-h1a-4b-r4/analyze.pytools/gdn-h1a-4b-r4/controller.pytools/gdn-h1a-4b-r4/test_harness.py
| const std::filesystem::path jsonl_path(benchmark_jsonl.get()); | ||
| std::error_code size_error; | ||
| if (std::filesystem::exists(jsonl_path) && std::filesystem::file_size(jsonl_path, size_error) != 0) { | ||
| throw std::invalid_argument("benchmark_jsonl must be new or empty"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
size_error is never inspected, so failures report a misleading reason.
std::filesystem::file_size with an error_code returns static_cast<uintmax_t>(-1) on failure. A permission error or a directory at jsonl_path therefore produces "benchmark_jsonl must be new or empty", which hides the real cause. std::filesystem::exists(jsonl_path) without an error_code also throws filesystem_error instead of the intended invalid_argument. Check the status explicitly.
🐛 Proposed fix
const std::filesystem::path jsonl_path(benchmark_jsonl.get());
- std::error_code size_error;
- if (std::filesystem::exists(jsonl_path) && std::filesystem::file_size(jsonl_path, size_error) != 0) {
- throw std::invalid_argument("benchmark_jsonl must be new or empty");
+ std::error_code fs_error;
+ if (std::filesystem::exists(jsonl_path, fs_error)) {
+ const auto existing_size = std::filesystem::file_size(jsonl_path, fs_error);
+ if (fs_error) { throw std::invalid_argument("unable to stat benchmark_jsonl: " + fs_error.message()); }
+ if (existing_size != 0) { throw std::invalid_argument("benchmark_jsonl must be new or empty"); }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/qwen3_5/main.cpp` around lines 100 - 104, Update the benchmark_jsonl
validation around jsonl_path to use error_code-aware filesystem checks, inspect
size_error immediately after file_size, and distinguish filesystem failures from
the non-empty-file case. Preserve the invalid_argument rejection only when the
path exists and has nonzero size, while reporting filesystem errors through the
existing error-handling approach.
| min_new_tokens_ = args.count("min_new_tokens") ? args.at("min_new_tokens").get<int>() : 0; | ||
| eos_token_id_ = args.count("eos_token_id") ? args.at("eos_token_id").get<int>() : gen.eos_token_id_; | ||
| do_sample_ = args.count("do_sample") ? args.at("do_sample").get<bool>() : gen.do_sample_; | ||
| if (max_length_ <= 0) { throw std::invalid_argument("max_length must be positive"); } | ||
| if (min_new_tokens_ < 0 || min_new_tokens_ > max_length_) { | ||
| throw std::invalid_argument("min_new_tokens must be between 0 and max_length"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject invalid sampling temperatures before generation.
If temperature is negative, sampleTemperature multiplies the suppressed EOS logit by a negative value. This changes std::numeric_limits<float>::lowest() into the highest logit. EOS can then be sampled before min_new_tokens.
Reject non-finite and non-positive temperatures in all three entry points. Add a regression test with min_new_tokens > 1 and temperature = -1.0F.
Proposed fix
+#include <cmath>
`#include` <limits>
- if (max_length_ <= 0) { throw std::invalid_argument("max_length must be positive"); }
+ if (!(std::isfinite(temperature_) && temperature_ > 0.0F)) {
+ throw std::invalid_argument("temperature must be finite and positive");
+ }
+ if (max_length_ <= 0) { throw std::invalid_argument("max_length must be positive"); }
- if (max_length <= 0) { throw std::invalid_argument("max_length must be positive"); }
+ if (!(std::isfinite(temperature) && temperature > 0.0F)) {
+ throw std::invalid_argument("temperature must be finite and positive");
+ }
+ if (max_length <= 0) { throw std::invalid_argument("max_length must be positive"); }
- if (max_length <= 0) { throw std::invalid_argument("max_length must be positive"); }
+ if (!(std::isfinite(temperature) && temperature > 0.0F)) {
+ throw std::invalid_argument("temperature must be finite and positive");
+ }
+ if (max_length <= 0) { throw std::invalid_argument("max_length must be positive"); }As per coding guidelines, “Validate inputs for public APIs and critical internal functions.”
Also applies to: 154-160, 235-241
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mllm/models/ARGeneration.cpp` around lines 35 - 41, Validate temperature in
all three generation entry points before sampling, rejecting non-finite or
non-positive values so invalid inputs cannot reach sampleTemperature; update the
relevant validation paths around the existing max_length/min_new_tokens checks.
Add a regression test covering min_new_tokens greater than 1 with temperature
set to -1.0F.
Source: Coding guidelines
| try: | ||
| result = analyze(args.result_root) | ||
| except ContractError as error: | ||
| result = {"disposition": "HARNESS_REJECTED", "errors": [str(error)]} | ||
| args.output.write_text(json.dumps(result, indent=2, sort_keys=True) + "\n", encoding="utf-8") | ||
| print(result["disposition"]) | ||
| return 0 if result["disposition"] in ("HARNESS_REHEARSAL_VALID", "PROMOTE_H1A", "RETAIN_INCUMBENT", "REJECT_CORRECTNESS", "SCREENING_BLOCKED") else 2 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Only ContractError produces a receipt; other exceptions crash the analyzer.
analyze reaches unguarded dictionary and filesystem access on a malformed contract. Line 173 reads prompt["label"] and raises KeyError when a contract prompt omits the label. Line 198 sorts prompt.get("expected_tokens") and raises TypeError when a prompt omits the token count, because sorted compares None with int. Line 208 raises NotADirectoryError when a label names a regular file. In each case main propagates the exception, writes no output file, and prints no disposition. The fail-closed contract then produces no receipt. Catch Exception and record it as HARNESS_REJECTED.
🐛 Proposed fix
try:
result = analyze(args.result_root)
- except ContractError as error:
- result = {"disposition": "HARNESS_REJECTED", "errors": [str(error)]}
+ except ContractError as error:
+ result = {"disposition": "HARNESS_REJECTED", "errors": [str(error)]}
+ except Exception as error: # noqa: BLE001 - the analyzer must always emit a receipt
+ result = {"disposition": "HARNESS_REJECTED", "errors": [f"{type(error).__name__}: {error}"]}
args.output.write_text(json.dumps(result, indent=2, sort_keys=True) + "\n", encoding="utf-8")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| result = analyze(args.result_root) | |
| except ContractError as error: | |
| result = {"disposition": "HARNESS_REJECTED", "errors": [str(error)]} | |
| args.output.write_text(json.dumps(result, indent=2, sort_keys=True) + "\n", encoding="utf-8") | |
| print(result["disposition"]) | |
| return 0 if result["disposition"] in ("HARNESS_REHEARSAL_VALID", "PROMOTE_H1A", "RETAIN_INCUMBENT", "REJECT_CORRECTNESS", "SCREENING_BLOCKED") else 2 | |
| try: | |
| result = analyze(args.result_root) | |
| except ContractError as error: | |
| result = {"disposition": "HARNESS_REJECTED", "errors": [str(error)]} | |
| except Exception as error: # noqa: BLE001 - the analyzer must always emit a receipt | |
| result = {"disposition": "HARNESS_REJECTED", "errors": [f"{type(error).__name__}: {error}"]} | |
| args.output.write_text(json.dumps(result, indent=2, sort_keys=True) + "\n", encoding="utf-8") | |
| print(result["disposition"]) | |
| return 0 if result["disposition"] in ("HARNESS_REHEARSAL_VALID", "PROMOTE_H1A", "RETAIN_INCUMBENT", "REJECT_CORRECTNESS", "SCREENING_BLOCKED") else 2 |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 271-271: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result, indent=2, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/gdn-h1a-4b-r4/analyze.py` around lines 268 - 274, Update the exception
handling in main around the analyze(args.result_root) call to catch general
Exception in addition to ContractError, converting any unexpected analysis
failure into the same HARNESS_REJECTED result with the exception message in
errors. Preserve the existing output-file writing, disposition printing, and
return-code logic for both handled error types.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mllm/models/ARGeneration.hpp`:
- Around line 108-122: Expand the public comments for generate and its streaming
variant in ARGeneration.hpp to document the input and ARGenerationArgs
parameters, returned ARGenerationOutputPast or ARGenerationChatContext values,
callback invocation behavior, and all validation/error conditions. Cover both
APIs consistently, including the existing min_new_tokens/max_length contract,
and update the related declarations near the streaming API.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 25abffae-8f35-4c65-a651-e6406343fb36
📒 Files selected for processing (7)
examples/qwen3_5/benchmark_harness.hppexamples/qwen3_5/main.cppmllm/models/ARGeneration.hpptests/core/ARGenerationTest.cpptests/core/CMakeLists.txttests/core/Qwen35BenchmarkHarnessTest.cpptests/cpu/Qwen35GDNConvTest.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/core/ARGenerationTest.cpp
- examples/qwen3_5/main.cpp
- examples/qwen3_5/benchmark_harness.hpp
- tests/cpu/Qwen35GDNConvTest.cpp
- tests/core/Qwen35BenchmarkHarnessTest.cpp
- tests/core/CMakeLists.txt
Compare the production kernel against an independent scalar reference on both
output and final history, bitwise. Covers batch {1,2}, sequence
{1,2,16,69,128,517}, channels {1,2,3,4,5,7,130} plus production 6144/8192 and
their channel tails, kernel sizes {2,3,4,5}, zero and non-zero initial history,
one-shot/split/multi-chunk partitions, reset between requests, and the null and
geometry guards.
All 12 tests pass against the unmodified kernel.
Process four adjacent channels per iteration when kernel_size is 4, which is the production Qwen3.5 setting. vld3/vld4 deinterleave the [B, C, K-1] history and [C, K] weights into per-tap lanes, so the per-token history shift happens in registers instead of two scalar loads and two scalar stores per element. Accumulation order matches the scalar body exactly: a rounded multiply by the newest tap, then taps 0, 1, 2 fused in ascending order. Compilers already contract the scalar accumulation into an FMA, so the vector form is bitwise identical; the focused oracle asserts that rather than assuming it. Non-AArch64 targets, kernel sizes other than 4, and channel tails keep the incumbent scalar path. The public signature, layouts, and guards are unchanged. Validated: focused conv oracle (1397 bitwise output+history checks) and the GDN oracle pass on macOS arm64 with the NEON path active and on x86_64 with the scalar fallback active; Android arm64-v8a builds with the NEON opcodes present in libMllmCPUBackend.so.
260e39e to
ef595c9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/core/Qwen35BenchmarkHarnessTest.cpp`:
- Around line 58-63: Update the test around validateRequiredTelemetry to also
assert the error identifier for the missing ceiling_vector entry at index 2,
alongside the existing scaling_max_freq assertion, ensuring the test fails if
ceiling_vector validation is ignored.
- Around line 6-8: Update the includes in Qwen35BenchmarkHarnessTest to directly
include the standard headers required for std::string and std::vector, alongside
the existing includes, without relying on transitive dependencies.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bb80ff2-4876-4c87-853f-7b36d326117e
📒 Files selected for processing (10)
examples/qwen3_5/benchmark_harness.hppexamples/qwen3_5/main.cppmllm/backends/cpu/kernels/common/gdn/gated_delta_net.cppmllm/models/ARGeneration.cppmllm/models/ARGeneration.hpptests/core/ARGenerationTest.cpptests/core/CMakeLists.txttests/core/Qwen35BenchmarkHarnessTest.cpptests/cpu/CMakeLists.txttests/cpu/Qwen35GDNConvTest.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/core/CMakeLists.txt
- tests/cpu/CMakeLists.txt
- mllm/backends/cpu/kernels/common/gdn/gated_delta_net.cpp
- tests/core/ARGenerationTest.cpp
- examples/qwen3_5/benchmark_harness.hpp
- examples/qwen3_5/main.cpp
- mllm/models/ARGeneration.cpp
- mllm/models/ARGeneration.hpp
| #include <algorithm> | ||
|
|
||
| #include "benchmark_harness.hpp" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'std::(string|vector)|`#include` <(string|vector)>' tests/core/Qwen35BenchmarkHarnessTest.cppRepository: UbiquitousLearning/mllm
Length of output: 281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- top includes ---\n'
sed -n '1,30p' tests/core/Qwen35BenchmarkHarnessTest.cpp
printf '\n--- relevant include lines (string/vector) in repository headers used by test ---\n'
file="$(sed -n '8p' tests/core/Qwen35BenchmarkHarnessTest.cpp | awk '{print $2}' | tr -d '"<>')"
printf 'Direct included header: %s\n' "$file"
sed -n '1,80p' "$file" 2>/dev/null || true
printf '\n--- search for std::string/vector usages in test only ---\n'
rg -n 'std::(string|vector)|`#include` <(string|vector)>' tests/core/Qwen35BenchmarkHarnessTest.cppRepository: UbiquitousLearning/mllm
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- top includes ---'
sed -n '1,30p' tests/core/Qwen35BenchmarkHarnessTest.cpp
echo
echo '--- includes of std::string/std::vector in test only ---'
rg -n 'std::(string|vector)|`#include` <(string|vector)>' tests/core/Qwen35BenchmarkHarnessTest.cpp || trueRepository: UbiquitousLearning/mllm
Length of output: 1177
Include the standard-library dependencies directly.
This test uses std::string and std::vector, but it only gets them transitively through <gtest/gtest.h> or benchmark_harness.hpp. Add <string> and <vector> to avoid relying on header implementation details.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/core/Qwen35BenchmarkHarnessTest.cpp` around lines 6 - 8, Update the
includes in Qwen35BenchmarkHarnessTest to directly include the standard headers
required for std::string and std::vector, alongside the existing includes,
without relying on transitive dependencies.
| snapshot["cpus"][2]["scaling_max_freq"] = nullptr; | ||
| snapshot["ceiling_vector"][2] = nullptr; | ||
|
|
||
| const auto errors = mllm::examples::qwen3_5::benchmark::validateRequiredTelemetry(snapshot); | ||
| EXPECT_NE(std::find(errors.begin(), errors.end(), "cpu6_missing_scaling_max_freq"), errors.end()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the missing ceiling-vector error.
The test sets ceiling_vector[2] to nullptr, but it asserts only "cpu6_missing_scaling_max_freq". The test passes if validateRequiredTelemetry ignores ceiling_vector. Assert the error that specifically reports the missing ceiling entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/core/Qwen35BenchmarkHarnessTest.cpp` around lines 58 - 63, Update the
test around validateRequiredTelemetry to also assert the error identifier for
the missing ceiling_vector entry at index 2, alongside the existing
scaling_max_freq assertion, ensuring the test fails if ceiling_vector validation
is ignored.
…ion tests - ARGenerationTest: add generate/streamGenerate invalid-bound cases (min_new_tokens > max_length, negative min_new_tokens, non-positive max_length) covering the independent validation paths - examples/qwen3_5/main.cpp: --benchmark_warmup alone now enters benchmark mode and triggers the required-option validation - tests: include <algorithm>/<stdexcept> directly instead of relying on transitive includes; non-Apple builds GTEST_SKIP the macOS-only telemetry test - benchmark_harness.hpp: document why cpu0/online is hard-coded online - ARGeneration.hpp: document the min_new_tokens/max_length contract on the public generate/streamGenerate/chat declarations Note: CTest registration via gtest_discover_tests was considered and dropped: the repo registers no test target with CTest, and gtest_discover_tests runs the test executable at build time, which cannot execute an Android ARM64 binary on the x86 CI runner (build-android failure).
ef595c9 to
d32364c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Summary
Adds an AArch64 NEON fast path for the GDN depthwise causal convolution (the Qwen3.5 prefill hot spot) plus the 4B benchmark tooling used to measure it. The vectorized kernel is bitwise identical to the scalar path per toolchain; the focused oracle asserts that rather than assuming it.
Review map
mllm/backends/cpu/kernels/common/gdn/gated_delta_net.cppvld3/vld4/vfma/vst3), accumulation order vs scalar body, scalar fallback for non-AArch64 /kernel_size != 4/ channel tailstests/cpu/Qwen35GDNConvTest.cppmllm/models/ARGeneration.{hpp,cpp}+tests/core/ARGenerationTest.cppmin_new_tokens: bounds, EOS suppression, delayed termination, invalid-argument paths across iterator/batch/streamingexamples/qwen3_5/benchmark_harness.hpp,examples/qwen3_5/main.cppSuggested review order: GDN kernel → kernel oracle → generation contract → benchmark harness.
Supported contract
kernel_size == 4(the production Qwen3.5 setting), 4 channels per iterationkernel_size != 4, and channel tails; no behavior change elsewhereARGenerationArgskeymin_new_tokens(default 0 = legacy behavior)Validation
Qwen35GDNConvTest6/6; 1,397 bitwise output+history checks, 0 failuresarm64-v8a: all three test targets compile (Mllm-Test-Qwen35-GDN-Conv,Mllm-Test-Core-ARGeneration,Mllm-Test-Core-Qwen35BenchmarkHarness)git diff --checkclean; formatting per.clang-formatPerformance evidence
Conclusion: on-device prefill is faster by +30–35% (0.8B) and +18–36% (4B); decode is unchanged within run-to-run noise.
Setup — OnePlus 13T (PKX110, SM8750, Android 16, arm64-v8a), 8 CPU threads, 3 runs per configuration with 5 s cooldown between runs, median reported. Prefill speed in tokens/s, higher is better. Baseline =
origin/main@2c889d90(scalar convolution); candidate = this PR's vectorized convolution. Same device, same model artifact, same binary set, measured back-to-back.Qwen3.5-4B — real-model prefill:
Qwen3.5-0.8B — real-model prefill:
Decode is unaffected — this PR only touches the prefill convolution path. Decode speed changed by −6%…+2% across all six configurations, i.e. within noise and with no systematic direction.
Operator-level (supplementary) — macOS arm64, convolution only,
C=8192, S=517: +69–84% median. The end-to-end numbers above are lower because they include attention and MLP, which this PR does not touch.Files changed
10 files, +897/−37:
mllm/backends/cpu/kernels/common/gdn/gated_delta_net.cpp(the vectorized kernel, +39/−1)tests/cpu/Qwen35GDNConvTest.cpp(new oracle, 265 lines) +tests/cpu/CMakeLists.txtexamples/qwen3_5/benchmark_harness.hpp,examples/qwen3_5/main.cppmllm/models/ARGeneration.{hpp,cpp},tests/core/ARGenerationTest.cpp,tests/core/CMakeLists.txtKnown limits
kernel_size == 4only; all other targets/shapes use the incumbent scalar path.Scope notes
generate/streamGenerateinvalid-bound tests,--benchmark_warmupbenchmark-mode detection, direct includes in tests,GTEST_SKIPon non-Apple platforms, cpu0/online documentation, and themin_new_tokenspublic-API contract docs. The suggested CTest registration (gtest_discover_tests) was not applied: the repo registers no test target with CTest, andgtest_discover_testsexecutes the test binary at build time, which cannot run an Android ARM64 binary on the x86 CI runner (build-android failure).Summary by CodeRabbit
New Features
min_new_tokenssupport across standard, streaming, and chat generation, preventing early end-of-sequence termination.Performance
Tests