feat(cpu): add MiniCPM5-1B with native KV-head GQA - #700
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMiniCPM5 support is added for ARM CPU execution. The change includes model configuration, tokenizer, grouped-query attention, KV caching, Kai quantization support, a command-line runner, telemetry-based benchmarking, documentation, and regression tests. ChangesMiniCPM5 and grouped-query attention
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: ⚪ Minimal · up to This PR adds CPU support for MiniCPM5-1B with native KV-head caching and grouped-query attention, reducing cache memory while preserving the existing framework path. No actionable merge-blocking risk remains at the current head after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Runner
participant MiniCPM5ForCausalLM
participant GroupedQueryAttention
participant CPUGroupedQueryAttentionDecodeOp
participant KVHeadStaticCache
Runner->>MiniCPM5ForCausalLM: generate token sequence
MiniCPM5ForCausalLM->>KVHeadStaticCache: update and read KV history
MiniCPM5ForCausalLM->>GroupedQueryAttention: compute attention
GroupedQueryAttention->>CPUGroupedQueryAttentionDecodeOp: dispatch single-token FP32 decode
CPUGroupedQueryAttentionDecodeOp->>KVHeadStaticCache: consume native KV-head cache
CPUGroupedQueryAttentionDecodeOp-->>MiniCPM5ForCausalLM: return attention output
MiniCPM5ForCausalLM-->>Runner: return logits and streamed tokens
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: 13
🧹 Nitpick comments (22)
tests/nn/GroupedQueryAttentionTest.cpp (1)
170-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the supported 2,048-token cache boundary.
Lines 170-171 slice only 201 KV positions. The test does not execute decode at the supported 2,048-token cache limit. Add a maximum-length decode case that reads the final KV position and checks the output against the reference implementation.
🤖 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/nn/GroupedQueryAttentionTest.cpp` around lines 170 - 178, Extend the test around groupedQueryAttention to use a 2,048-token KV cache slice, ensuring the decode reads the final KV position at index 2047 rather than only 201 positions. Preserve the existing shape, finite-value, and expectNear comparisons against gqaReference for this maximum-length case.mllm/models/minicpm5/modeling_minicpm5.hpp (4)
117-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename or encapsulate the public members that use the private naming suffix.
logical_cache_slot_andself_attn_are public, but the trailing underscore marks a private data member in Google C++ Style. Either drop the suffix, or keep the members private and add a small accessor thatMiniCPM5Textuses to assign the cache slot.As per coding guidelines: "Adhere to language-specific best practices and idioms (e.g., PEP 8 for Python, Google C++ Style Guide for C++)."
Also applies to: 149-149
🤖 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/minicpm5/modeling_minicpm5.hpp` at line 117, Update the public members logical_cache_slot_ and self_attn_ to follow Google C++ naming conventions: either remove the trailing underscores or make them private and provide the minimal accessor needed by MiniCPM5Text to assign the cache slot, then update all affected references consistently.Source: Coding guidelines
23-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHoist the raw pointer out of the loop and reject an odd
output_dim.Two points:
inv_freq.ptr<float>()is re-evaluated on every iteration. Hoist it once before the loop.- If
output_dimis odd,output_dim / 2truncates and the rotary table silently covers fewer dimensions than the head.MiniCPM5Configvalidates onlyhead_dim > 0, so an oddhead_dimreaches this function and produces a wrong embedding with no error.♻️ Proposed change
inline auto makeMiniCPM5RoPEInvFreq(int32_t output_dim, float rope_theta) -> Tensor { + if (output_dim <= 0 || output_dim % 2 != 0) { + throw std::invalid_argument("MiniCPM5 RoPE requires a positive even head_dim"); + } auto inv_freq = Tensor::empty({output_dim / 2}, kFloat32, kCPU).alloc(); - for (int32_t dim = 0; dim < output_dim / 2; ++dim) { - inv_freq.ptr<float>()[dim] = 1.0F / std::pow(rope_theta, 2.0F * static_cast<float>(dim) / output_dim); - } + const int32_t half_dim = output_dim / 2; + auto* inv_freq_data = inv_freq.ptr<float>(); + for (int32_t dim = 0; dim < half_dim; ++dim) { + inv_freq_data[dim] = 1.0F / std::pow(rope_theta, 2.0F * static_cast<float>(dim) / static_cast<float>(output_dim)); + } return inv_freq; }🤖 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/minicpm5/modeling_minicpm5.hpp` around lines 23 - 29, Update makeMiniCPM5RoPEInvFreq to reject odd or otherwise invalid output_dim before allocating the table, preserving the existing behavior only for positive even dimensions; then obtain inv_freq.ptr<float>() once before the loop and reuse that pointer for all assignments.
39-53: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the tensor data pointers out of the nested loops.
The innermost loop calls
inv_freq.ptr<float>(),sin_embedding.ptr<float>(), andcos_embedding.ptr<float>()on every iteration, and the middle loop callsposition_ids.ptr<int64_t>()on every iteration. For a 200-token prefill withhalf_dim = 64that is roughly 50k redundant accessor calls per layer-independent call. Hoist each pointer once.♻️ Proposed change
+ const auto* position_data = position_ids.ptr<int64_t>(); + const auto* inv_freq_data = inv_freq.ptr<float>(); + auto* sin_data = sin_embedding.ptr<float>(); + auto* cos_data = cos_embedding.ptr<float>(); for (int32_t batch_index = 0; batch_index < batch; ++batch_index) { for (int32_t sequence_index = 0; sequence_index < sequence; ++sequence_index) { - const auto position = position_ids.ptr<int64_t>()[batch_index * sequence + sequence_index]; + const auto position = position_data[batch_index * sequence + sequence_index]; for (int32_t index = 0; index < half_dim; ++index) { - const float frequency = static_cast<float>(position) * inv_freq.ptr<float>()[index]; + const float frequency = static_cast<float>(position) * inv_freq_data[index]; const float sine = std::sin(frequency); const float cosine = std::cos(frequency); const auto offset = (batch_index * sequence + sequence_index) * dim + index; - sin_embedding.ptr<float>()[offset] = sine; - sin_embedding.ptr<float>()[offset + half_dim] = sine; - cos_embedding.ptr<float>()[offset] = cosine; - cos_embedding.ptr<float>()[offset + half_dim] = cosine; + sin_data[offset] = sine; + sin_data[offset + half_dim] = sine; + cos_data[offset] = cosine; + cos_data[offset + half_dim] = cosine; } } }🤖 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/minicpm5/modeling_minicpm5.hpp` around lines 39 - 53, Hoist the data pointers for position_ids, inv_freq, sin_embedding, and cos_embedding before the nested batch/sequence/index loops in the rotary embedding computation. Use these cached pointers inside the loops instead of calling ptr<T>() repeatedly, while preserving the existing indexing and output values.
179-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo public entity in the three new MiniCPM5 headers has a doc comment. All three files declare public classes and free functions that throw
std::invalid_argumentorstd::runtime_error, and none documents purpose, parameters, returns, or error conditions. The shared root cause is one missing documentation pass over the new public surface.
mllm/models/minicpm5/modeling_minicpm5.hpp#L179-L192: documentMiniCPM5ForCausalLM,forward(rank-2 int64 CPU batch-1 input contract and its four throw conditions),resetState, andkvCache; also documentMiniCPM5Attention,MiniCPM5Decoder,MiniCPM5Text,makeMiniCPM5RoPEInvFreq, andmakeMiniCPM5RotaryPosEmbedding.mllm/models/minicpm5/configuration_minicpm5.hpp#L18-L21: documentMiniCPM5Configand its constructor throw conditions, plusmatchesOfficialMiniCPM5_1BRuntimeContractandvalidateModelConfigMatch.mllm/models/minicpm5/tokenization_minicpm5.hpp#L215-L217: documentMiniCPM5Tokenizer,applyChatTemplate,detokenizeBytes,convertMessage,MiniCPM5StreamingUtf8Decoder, andMiniCPM5Message.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/minicpm5/modeling_minicpm5.hpp` around lines 179 - 192, Add clear documentation comments for every listed public API across mllm/models/minicpm5/modeling_minicpm5.hpp:179-192, mllm/models/minicpm5/configuration_minicpm5.hpp:18-21, and mllm/models/minicpm5/tokenization_minicpm5.hpp:215-217. Document the purpose, parameters, returns, and relevant std::invalid_argument/std::runtime_error conditions for MiniCPM5ForCausalLM, its forward/resetState/kvCache members, MiniCPM5Attention, MiniCPM5Decoder, MiniCPM5Text, both RoPE helpers, MiniCPM5Config and its constructor, both configuration validators, MiniCPM5Tokenizer and its conversion methods, MiniCPM5StreamingUtf8Decoder, and MiniCPM5Message; make forward explicitly state its rank-2 int64 CPU batch-1 input contract and four throw conditions.Source: Coding guidelines
examples/minicpm5/benchmark_harness.hpp (3)
119-160: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
thermal_zonesis captured but never validated.
captureTelemetryrecordsthermal_zoneswithtemp_milli_cper zone. NeithervalidateRequiredTelemetrynorvalidateStableTelemetryinspects that field. A run that thermally throttles betweentelemetry_beforeandtelemetry_afterstill reportsstatus: "ok", because only affinity, ceiling vector, online state, and governor are gated.If thermal stability is part of the intended evidence, add a drift check with an explicit threshold. If thermal data is informational only, that is a reasonable choice; state it in a comment so a later reader does not assume the gate exists.
🤖 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/minicpm5/benchmark_harness.hpp` around lines 119 - 160, Update validateStableTelemetry to account for captured thermal_zones: compare each zone’s temp_milli_c between before and after using an explicit, documented drift threshold, and report a dedicated validation error when exceeded. If thermal data is intentionally informational rather than gated, instead add a clear comment documenting that decision and leave validation behavior unchanged.
97-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck the
error_codefromdirectory_iterator, or record the failure.
erroris passed to bothstd::filesystem::existsandstd::filesystem::directory_iterator, then never inspected. If enumeration of/sys/class/thermalfails,thermal_zonesstays empty and the record looks identical to a device with no thermal zones. Because the benchmark uses this snapshot as evidence about the device state, record the failure instead of dropping it.♻️ Proposed change
const std::filesystem::path thermal_root("/sys/class/thermal"); std::error_code error; if (std::filesystem::exists(thermal_root, error)) { std::vector<std::filesystem::path> zones; - for (const auto& entry : std::filesystem::directory_iterator(thermal_root, error)) { + std::error_code iterate_error; + for (const auto& entry : std::filesystem::directory_iterator(thermal_root, iterate_error)) { if (entry.path().filename().string().starts_with("thermal_zone")) { zones.push_back(entry.path()); } } + if (iterate_error) { snapshot["thermal_zones_error"] = iterate_error.message(); } std::sort(zones.begin(), zones.end());🤖 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/minicpm5/benchmark_harness.hpp` around lines 97 - 114, Update the thermal-zone enumeration around directory_iterator to inspect the shared error_code after iteration and record an explicit failure in the snapshot when enumeration fails, rather than leaving thermal_zones indistinguishable from an empty device. Preserve the existing zone collection and per-zone recording behavior when enumeration succeeds.
78-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplain the hardcoded
onlinevalue for CPU 0.Line 81 returns
1for CPU 0 without reading sysfs. The reason is that Linux normally does not expose/sys/devices/system/cpu/cpu0/onlinebecause CPU 0 cannot be offlined. A reader cannot infer that from the code, andvalidateRequiredTelemetrywould otherwise reportcpu0_missing_onlineon every run. Add a one-line comment that states this.🤖 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/minicpm5/benchmark_harness.hpp` around lines 78 - 95, Add a concise one-line comment immediately above the CPU 0 `online` initialization in the affinity loop, explaining that Linux omits `cpu0/online` because CPU 0 cannot be offlined, so the value is hardcoded to 1 to satisfy telemetry validation.mllm/models/minicpm5/tokenization_minicpm5.hpp (5)
101-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting this pre-tokenizer into named helpers.
miniCPM5TokenizerMatchPatternchains seven independent alternatives with nested loops and manual backtracking throughoriginal_position. The cyclomatic complexity makes each alternative hard to test in isolation, and a wrongpositionreset in any block silently changes tokenization for every prompt.Extract each alternative into a small named function, for example
matchContraction,matchWordWithLeadingSymbol,matchDigitRun,matchSymbolRun, andmatchWhitespace. Each returns whether it consumed input. That also makes unit tests per alternative practical.🤖 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/minicpm5/tokenization_minicpm5.hpp` around lines 101 - 169, Refactor miniCPM5TokenizerMatchPattern by extracting each matching alternative into named helpers such as matchContraction, matchWordWithLeadingSymbol, matchDigitRun, matchSymbolRun, and matchWhitespace, with each helper reporting whether it consumed input and preserving position/matched updates. Have the main function invoke these helpers in the existing order, removing its nested matching logic and manual backtracking while preserving tokenization behavior.
267-279: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the byte-to-unicode map lookup with a fixed array.
Line 274 performs an
std::unordered_maplookup for every input byte of every piece.bytes_to_unicode_has exactly 256 entries keyed by byte value, so astd::array<wchar_t, 256>gives O(1) indexed access with no hashing and no cache misses. This loop runs over the full prompt on every request.
operator[]also inserts a default-constructedwchar_tfor a key that is absent, which would silently map an unmapped byte to\0instead of failing. An array removes that failure mode as well.🤖 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/minicpm5/tokenization_minicpm5.hpp` around lines 267 - 279, Replace the bytes_to_unicode_ lookup used in _tokenize with a fixed-size std::array<wchar_t, 256>, initialized for every byte value, and index it directly by byte in the mapping loop. Update its declaration and initialization consistently so no unordered_map or operator[] lookup remains, while preserving the existing byte-to-character mappings.
171-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two whitespace blocks and document the intent.
Lines 171-184 and lines 186-191 implement the two whitespace alternatives of the Qwen pattern (
\s+(?!\S)then\s+). When the run length is 1, the first block resetspositiontostartand the second block consumes the same run, so the first block does no useful work on that path. The result is correct, but a reader cannot tell that from the code.Add a short comment that names the two regex alternatives, or collapse both blocks into one that keeps the trailing character only when the run length exceeds 1 and the input continues.
🤖 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/minicpm5/tokenization_minicpm5.hpp` around lines 171 - 191, Merge the two adjacent whitespace-handling blocks in the tokenizer into one implementation, preserving the Qwen alternatives \s+(?!\S) and \s+. Keep the trailing character only when the whitespace run exceeds one character and input remains; otherwise consume the full run, and add a brief comment documenting this intent.
221-233: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid parsing the tokenizer JSON twice.
bpe_.initFromSentencePieceJson(file_path)reads and parsestokenizer.json, then lines 225-227 open and parse the same file again. The official MiniCPM5 tokenizer file carries a 130560-entry vocabulary and its merge table, so the second parse roughly doubles tokenizer load time and peak parse memory. That matters on the memory-constrained mobile target this example targets.Parse the JSON once and pass the parsed object to the BPE initializer, or read the required-token metadata from the object that the BPE initializer already built.
Separately,
nlohmann::json::parseat line 227 throwsnlohmann::json::parse_errorfor a malformed file, while every other failure in this constructor throwsstd::invalid_argument. Wrap the parse so the constructor reports one consistent error type.🤖 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/minicpm5/tokenization_minicpm5.hpp` around lines 221 - 233, Update the MiniCPM5 tokenizer constructor around bpe_.initFromSentencePieceJson to parse tokenizer.json only once, reusing the parsed JSON for BPE initialization and added_tokens/model validation instead of reopening the file. Also catch nlohmann::json::parse_error from malformed input and rethrow it as std::invalid_argument, preserving the constructor’s consistent failure type.
331-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplication between
convert2IdsandconvertMessage.Both functions allocate a
{1, tokens.size()}int64 CPU tensor with the same name and fill it withbpe_._lookup_vocab. OnlysetMemTypediffers (kExtraInputversuskNormal). Extract one private helper that takes the token list and the memory type.♻️ Proposed change
+ private: + Tensor makeSequenceTensor(const std::vector<std::wstring>& tokens, MemTypes mem_type) { + auto sequence = Tensor::empty({1, static_cast<int32_t>(tokens.size())}, kInt64, kCPU) + .setMemType(mem_type) + .setName("minicpm5-tokenizer-i0") + .alloc(); + for (size_t index = 0; index < tokens.size(); ++index) { + sequence.ptr<int64_t>()[index] = bpe_._lookup_vocab(tokens[index]); + } + return sequence; + } + + public: + Tensor convert2Ids(const std::vector<std::wstring>& tokens) override { return makeSequenceTensor(tokens, kExtraInput); } + + ARGenerationOutputPast convertMessage(const MiniCPM5Message& message) { + return {{"sequence", makeSequenceTensor(tokenize(applyChatTemplate(message)), kNormal)}}; + }Confirm the exact
MemTypesenum name before applying.🤖 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/minicpm5/tokenization_minicpm5.hpp` around lines 331 - 352, Extract the shared tensor allocation and vocabulary-ID population from convert2Ids and convertMessage into one private helper that accepts the token list and the appropriate memory-type value. Have each caller delegate to this helper while preserving kExtraInput for convert2Ids, kNormal for convertMessage, and the existing tensor shape, name, and return behavior; use the project’s exact MemTypes enum type.mllm/models/minicpm5/configuration_minicpm5.hpp (1)
98-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the contract check from shared constants.
The official values appear twice: once as default member initializers at lines 78-95, and again as literals here. A future change to one location can silently desynchronize the other. Extract the official values into named constants and reference them in both places.
Also note that
rms_norm_eps == 1.0e-6Fandrope_theta == 5000000.0Fuse exact float equality. That is acceptable for a strict contract gate, but it makes the check sensitive to how the JSON writer formats the value.🤖 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/minicpm5/configuration_minicpm5.hpp` around lines 98 - 107, Extract the official MiniCPM5-1B runtime values into shared named constants, then use those constants in both the MiniCPM5Config default member initializers and matchesOfficialMiniCPM5_1BRuntimeContract. Replace the duplicated literals without changing the strict contract behavior, including exact float comparisons for rms_norm_eps and rope_theta.examples/minicpm5/quant_cfg_1B_w4a32_kai.json (1)
2-2: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDrop
biasfrom the patterns, or give bias tensors their own hints.Every pattern matches
\.(bias|weight), and every rule pairs that with a 2-Dshapehint such as[2048, 1536]. The official MiniCPM5-1B checkpoint setsattention_bias: falseand uses no MLP orlm_headbias, so no bias key matches today and the configuration is correct. If a checkpoint variant ever carried a bias tensor, the converter would apply a 2-D weight shape hint to a 1-D bias of length 2048 and silently produce a corrupt packed weight.Restrict the patterns to
\.weight, so a bias tensor fails loudly instead of matching a weight rule.Also applies to: 12-12, 22-22, 32-32, 42-42, 52-52
🤖 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/minicpm5/quant_cfg_1B_w4a32_kai.json` at line 2, Update the regex patterns in the MiniCPM5 quantization configuration to match only .weight tensors, removing bias from the q_proj pattern and the corresponding patterns at the other referenced locations. Preserve the existing 2-D shape hints so any unexpected bias tensor no longer matches a weight rule and instead fails loudly.mllm/nn/Functional.hpp (1)
113-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported GQA interfaces.
The new public APIs do not document their input layout, output layout, supported execution conditions, or error behavior.
mllm/nn/Functional.hpp#L113-L114: document tensor layout, output layout, supported inputs, and failure behavior.mllm/nn/layers/GroupedQueryAttentionDecode.hpp#L11-L17: document layer purpose, constructor options, forward inputs, output, and errors.mllm/nn/llm_components/GroupedQueryAttention.hpp#L63-L96: document the GQA contract, CPU and dtype restrictions, return value, and exceptions.As per coding guidelines, public APIs, classes, and functions must have clear comments that explain 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/nn/Functional.hpp` around lines 113 - 114, Document the exported GQA APIs at all affected sites: in mllm/nn/Functional.hpp lines 113-114, describe the tensor input layouts, output layout, supported inputs, and failure behavior for groupedQueryAttentionDecode; in mllm/nn/layers/GroupedQueryAttentionDecode.hpp lines 11-17, document the layer purpose, constructor options, forward inputs, output, and errors; and in mllm/nn/llm_components/GroupedQueryAttention.hpp lines 63-96, document the GQA contract, CPU and dtype restrictions, return value, and exceptions.Source: Coding guidelines
mllm/compile/ir/GeneratedRTTIKind.hpp (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake RTTI generation reproducible.
rtti_kind_gen.pyinsertsdatetime.now()into both generated headers, so identical inputs produce different output on each run. Remove the wall-clock timestamp or derive it from a fixed input.🤖 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/compile/ir/GeneratedRTTIKind.hpp` at line 1, Make rtti_kind_gen.py generate reproducible headers by removing the datetime.now() wall-clock value or replacing it with a deterministic fixed-input value. Apply the change to both mllm/compile/ir/GeneratedRTTIKind.hpp:1-1 and mllm/compile/ir/NodeRTTIClassOfImpl.hpp:1-1; both generated headers must remain identical across runs with identical inputs.mllm/nn/lmcache/KVHeadStaticCache.cpp (1)
59-62: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
setCurrentSeqCntexposes uninitialized cache content.The method accepts any
seqin[0, max_cache_length_]and applies it to every slot. If a caller raisesseqabove the value written byupdateKVCache,getKVCachereturns a slice that covers buffer regions that no append has written. The tensors are zero-initialized at construction, so the attention kernel then consumes zero keys and values as valid history.Consider rejecting a
seqvalue greater than the current count, or document that the method is only for rollback.🤖 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/nn/lmcache/KVHeadStaticCache.cpp` around lines 59 - 62, Update KVHeadStaticCache::setCurrentSeqCnt to prevent advancing any slot beyond the sequence count established by updateKVCache; retain support for valid rollback values and existing range validation, and ensure getKVCache cannot expose unwritten cache regions.mllm/backends/cpu/ops/LinearOp.hpp (1)
10-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe I8MM M threshold
4is unnamed and duplicated across two files. Both the selection predicate and the trace filter test the same tile M-step with a bare literal, so the two conditions can drift apart.
mllm/backends/cpu/ops/LinearOp.hpp#L10-L17: define a named constant such askKaiW4A32I8mmMinM = 4indetailand use it inshouldUseKaiW4A32I8mmPrefill.mllm/backends/cpu/ops/LinearOp.cpp#L51-L67: replace them < 4early return intraceKaiW4A32PrefillTilewith a test againstdetail::kKaiW4A32I8mmMinM.As per coding guidelines: "Use named constants instead of magic numbers."
🤖 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/backends/cpu/ops/LinearOp.hpp` around lines 10 - 17, Define detail::kKaiW4A32I8mmMinM as the shared named constant with value 4 in LinearOp.hpp, and update shouldUseKaiW4A32I8mmPrefill to use it. In LinearOp.cpp, update traceKaiW4A32PrefillTile to compare m against detail::kKaiW4A32I8mmMinM instead of the literal 4, keeping both conditions synchronized.Source: Coding guidelines
mllm/nn/lmcache/KVHeadStaticCache.hpp (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark
getKVCacheas[[nodiscard]].The method is
constand returns a value with no side effects. The neighboring accessors already use[[nodiscard]]. clang-tidy reports this as an error undermodernize-use-nodiscardwith warnings-as-errors, so the build can fail.🔧 Proposed fix
- std::array<Tensor, 2> getKVCache(int32_t logical_slot) const; + [[nodiscard]] std::array<Tensor, 2> getKVCache(int32_t logical_slot) const;🤖 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/nn/lmcache/KVHeadStaticCache.hpp` at line 36, Mark the const value-returning KVHeadStaticCache::getKVCache method with [[nodiscard]], matching the neighboring accessor declarations and satisfying modernize-use-nodiscard.Source: Linters/SAST tools
mllm/backends/cpu/ops/LinearOp.cpp (1)
35-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
HWCAP2_I8MMfrom<asm/hwcap.h>.The project uses Android NDK r28b, whose AArch64 sysroot defines this macro. Replace the local
1UL << 13constant withHWCAP2_I8MM.🤖 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/backends/cpu/ops/LinearOp.cpp` around lines 35 - 43, Update cpuSupportsI8mm to include and use the platform-defined HWCAP2_I8MM constant from <asm/hwcap.h> instead of the local kHwcap2I8mm bit shift, preserving the existing Linux AArch64 guard and support check.mllm/backends/cpu/CMakeLists.txt (1)
146-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
OpenMP::OpenMP_CXXwhen available and preserve the fallback.
OpenMP_CXX_FLAGScontains compiler flags, not a link library. Passing it totarget_link_librariesis not portable. LinkOpenMP::OpenMP_CXXPUBLICwhen the imported target exists. Keep the Android andNOT OpenMP_FOUNDfallback paths. Propagate the fallback compile options asPUBLICbecausemllm/core/Parallel.hppexpandsOMP_PRAGMAin CPU headers. Apply the same correction to the duplicateMllmRTOpenMP block.🤖 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/backends/cpu/CMakeLists.txt` around lines 146 - 148, Update the OpenMP linking blocks for both MllmCPUBackend and MllmRT to link PUBLIC against OpenMP::OpenMP_CXX when that imported target exists, while preserving the Android and NOT OpenMP_FOUND fallback paths. In fallback paths, propagate OpenMP_CXX_FLAGS through target_compile_options as PUBLIC and do not pass compiler flags to target_link_libraries; apply the same visibility and linking correction to both targets.Source: Learnings
🤖 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/minicpm5/config_1B_w4a32_kai.json`:
- Around line 2-5: Update the model identity fields in config_1B_w4a32_kai.json
to use the MiniCPM5-specific architecture and model_type identifiers expected by
the runner and conversion tooling, rather than Llama identifiers; preserve the
remaining configuration unchanged.
In `@examples/minicpm5/main.cpp`:
- Around line 141-216: Update the invalid-request termination logic in the
request loop so warmup records are still written but do not set exit_code or
break the loop when warmup is true. Measured requests must retain the existing
abort behavior for non-empty invalid_reasons.
- Around line 102-106: Update the benchmark_jsonl validation around jsonl_path
to inspect size_error immediately after std::filesystem::file_size returns,
before interpreting its value. Preserve the fail-closed behavior, but report the
filesystem/stat failure distinctly instead of throwing “benchmark_jsonl must be
new or empty” when file_size cannot be obtained.
In `@examples/minicpm5/README.md`:
- Around line 49-65: Update the reproducible demo command in the README to
include the --require_device_telemetry flag, ensuring copied benchmark runs
enforce telemetry completeness and CPU-frequency drift validation.
- Around line 67-70: Update the throughput formulas in the README to convert
microsecond durations to seconds: multiply the prefill and decode duration
denominators by 1,000,000, or equivalently multiply each resulting rate by
1,000,000, so both reported values are in tokens per second.
In `@mllm/backends/cpu/ops/GroupedQueryAttentionDecodeOp.cpp`:
- Around line 61-68: Update the reference fallback’s output write in
GroupedQueryAttentionDecodeOp to use the output tensor’s last-dimension stride
rather than assuming unit stride. Locate the value-dimension loop that assigns
output_head[value_dim] and index output_head using the corresponding output
stride, matching the stride-aware query and value reads.
- Around line 78-101: Guard CPUGroupedQueryAttentionDecodeOp::forward before
indexing inputs or outputs by asserting the required vector sizes, then invoke
the existing reshape validation to verify tensor shapes and prevent invalid
group-size calculations. Only access shapes, compute group_size, or call
ptr<float>() after validation succeeds.
In `@mllm/backends/cpu/ops/LinearOp.cpp`:
- Around line 75-82: Update CPULinearOp::acquireKaiWorkspace so concurrent m ==
1 calls cannot share or race on kai_decode_workspace_; use an execution-local
workspace, or protect cached workspace allocation and access with
synchronization covering the Kai operation. Preserve the existing resizing
behavior and the per-call allocation path for m != 1.
In `@mllm/compile/jit/interpreter/AopsFromJson.cpp`:
- Around line 601-608: Update __groupedQueryAttentionDecodeFromJson to validate
the parsed backend before calling Context::instance().getBackend or createOp.
Reject invalid values and any backend other than DeviceTypes::kCPU via
MLLM_ERROR_EXIT, while preserving the existing CPU operation creation path.
In `@mllm/core/aops/GroupedQueryAttentionDecodeOp.hpp`:
- Line 30: Update the const accessor GroupedQueryAttentionDecodeOp::options() to
include the C++20 [[nodiscard]] attribute, preserving its existing return type
and behavior.
In `@mllm/nn/llm_components/GroupedQueryAttention.hpp`:
- Around line 15-22: Ensure groupedQueryAttentionEager cannot access tensor
shapes before validating dimensionality and KV-head count: either make it
private to the already-validated caller or add equivalent validation at its
entry. Reject tensors with fewer than four dimensions and zero key/value heads
before computing groups, scale, or context_offset, while preserving the existing
validated execution path.
In `@mllm/nn/lmcache/KVHeadStaticCache.cpp`:
- Around line 83-95: Validate lane-packed key and value dtypes before the memcpy
loop in the cache update method: when lanesOfType(k_dtype_) or
lanesOfType(v_dtype_) is greater than one, require head_dim_ to be divisible by
the corresponding lane count, otherwise reject the operation. Preserve the
existing copy path only for aligned shapes, preventing offsettedPtr and
byte-size calculations from aliasing or truncating per-head data.
In `@README.md`:
- Line 109: Update the MiniCPM5-1B row in the README CPU column to label the
example as w4a32 instead of w4a8, matching the configuration and documented
W4A32 support.
---
Nitpick comments:
In `@examples/minicpm5/benchmark_harness.hpp`:
- Around line 119-160: Update validateStableTelemetry to account for captured
thermal_zones: compare each zone’s temp_milli_c between before and after using
an explicit, documented drift threshold, and report a dedicated validation error
when exceeded. If thermal data is intentionally informational rather than gated,
instead add a clear comment documenting that decision and leave validation
behavior unchanged.
- Around line 97-114: Update the thermal-zone enumeration around
directory_iterator to inspect the shared error_code after iteration and record
an explicit failure in the snapshot when enumeration fails, rather than leaving
thermal_zones indistinguishable from an empty device. Preserve the existing zone
collection and per-zone recording behavior when enumeration succeeds.
- Around line 78-95: Add a concise one-line comment immediately above the CPU 0
`online` initialization in the affinity loop, explaining that Linux omits
`cpu0/online` because CPU 0 cannot be offlined, so the value is hardcoded to 1
to satisfy telemetry validation.
In `@examples/minicpm5/quant_cfg_1B_w4a32_kai.json`:
- Line 2: Update the regex patterns in the MiniCPM5 quantization configuration
to match only .weight tensors, removing bias from the q_proj pattern and the
corresponding patterns at the other referenced locations. Preserve the existing
2-D shape hints so any unexpected bias tensor no longer matches a weight rule
and instead fails loudly.
In `@mllm/backends/cpu/CMakeLists.txt`:
- Around line 146-148: Update the OpenMP linking blocks for both MllmCPUBackend
and MllmRT to link PUBLIC against OpenMP::OpenMP_CXX when that imported target
exists, while preserving the Android and NOT OpenMP_FOUND fallback paths. In
fallback paths, propagate OpenMP_CXX_FLAGS through target_compile_options as
PUBLIC and do not pass compiler flags to target_link_libraries; apply the same
visibility and linking correction to both targets.
In `@mllm/backends/cpu/ops/LinearOp.cpp`:
- Around line 35-43: Update cpuSupportsI8mm to include and use the
platform-defined HWCAP2_I8MM constant from <asm/hwcap.h> instead of the local
kHwcap2I8mm bit shift, preserving the existing Linux AArch64 guard and support
check.
In `@mllm/backends/cpu/ops/LinearOp.hpp`:
- Around line 10-17: Define detail::kKaiW4A32I8mmMinM as the shared named
constant with value 4 in LinearOp.hpp, and update shouldUseKaiW4A32I8mmPrefill
to use it. In LinearOp.cpp, update traceKaiW4A32PrefillTile to compare m against
detail::kKaiW4A32I8mmMinM instead of the literal 4, keeping both conditions
synchronized.
In `@mllm/compile/ir/GeneratedRTTIKind.hpp`:
- Line 1: Make rtti_kind_gen.py generate reproducible headers by removing the
datetime.now() wall-clock value or replacing it with a deterministic fixed-input
value. Apply the change to both mllm/compile/ir/GeneratedRTTIKind.hpp:1-1 and
mllm/compile/ir/NodeRTTIClassOfImpl.hpp:1-1; both generated headers must remain
identical across runs with identical inputs.
In `@mllm/models/minicpm5/configuration_minicpm5.hpp`:
- Around line 98-107: Extract the official MiniCPM5-1B runtime values into
shared named constants, then use those constants in both the MiniCPM5Config
default member initializers and matchesOfficialMiniCPM5_1BRuntimeContract.
Replace the duplicated literals without changing the strict contract behavior,
including exact float comparisons for rms_norm_eps and rope_theta.
In `@mllm/models/minicpm5/modeling_minicpm5.hpp`:
- Line 117: Update the public members logical_cache_slot_ and self_attn_ to
follow Google C++ naming conventions: either remove the trailing underscores or
make them private and provide the minimal accessor needed by MiniCPM5Text to
assign the cache slot, then update all affected references consistently.
- Around line 23-29: Update makeMiniCPM5RoPEInvFreq to reject odd or otherwise
invalid output_dim before allocating the table, preserving the existing behavior
only for positive even dimensions; then obtain inv_freq.ptr<float>() once before
the loop and reuse that pointer for all assignments.
- Around line 39-53: Hoist the data pointers for position_ids, inv_freq,
sin_embedding, and cos_embedding before the nested batch/sequence/index loops in
the rotary embedding computation. Use these cached pointers inside the loops
instead of calling ptr<T>() repeatedly, while preserving the existing indexing
and output values.
- Around line 179-192: Add clear documentation comments for every listed public
API across mllm/models/minicpm5/modeling_minicpm5.hpp:179-192,
mllm/models/minicpm5/configuration_minicpm5.hpp:18-21, and
mllm/models/minicpm5/tokenization_minicpm5.hpp:215-217. Document the purpose,
parameters, returns, and relevant std::invalid_argument/std::runtime_error
conditions for MiniCPM5ForCausalLM, its forward/resetState/kvCache members,
MiniCPM5Attention, MiniCPM5Decoder, MiniCPM5Text, both RoPE helpers,
MiniCPM5Config and its constructor, both configuration validators,
MiniCPM5Tokenizer and its conversion methods, MiniCPM5StreamingUtf8Decoder, and
MiniCPM5Message; make forward explicitly state its rank-2 int64 CPU batch-1
input contract and four throw conditions.
In `@mllm/models/minicpm5/tokenization_minicpm5.hpp`:
- Around line 101-169: Refactor miniCPM5TokenizerMatchPattern by extracting each
matching alternative into named helpers such as matchContraction,
matchWordWithLeadingSymbol, matchDigitRun, matchSymbolRun, and matchWhitespace,
with each helper reporting whether it consumed input and preserving
position/matched updates. Have the main function invoke these helpers in the
existing order, removing its nested matching logic and manual backtracking while
preserving tokenization behavior.
- Around line 267-279: Replace the bytes_to_unicode_ lookup used in _tokenize
with a fixed-size std::array<wchar_t, 256>, initialized for every byte value,
and index it directly by byte in the mapping loop. Update its declaration and
initialization consistently so no unordered_map or operator[] lookup remains,
while preserving the existing byte-to-character mappings.
- Around line 171-191: Merge the two adjacent whitespace-handling blocks in the
tokenizer into one implementation, preserving the Qwen alternatives \s+(?!\S)
and \s+. Keep the trailing character only when the whitespace run exceeds one
character and input remains; otherwise consume the full run, and add a brief
comment documenting this intent.
- Around line 221-233: Update the MiniCPM5 tokenizer constructor around
bpe_.initFromSentencePieceJson to parse tokenizer.json only once, reusing the
parsed JSON for BPE initialization and added_tokens/model validation instead of
reopening the file. Also catch nlohmann::json::parse_error from malformed input
and rethrow it as std::invalid_argument, preserving the constructor’s consistent
failure type.
- Around line 331-352: Extract the shared tensor allocation and vocabulary-ID
population from convert2Ids and convertMessage into one private helper that
accepts the token list and the appropriate memory-type value. Have each caller
delegate to this helper while preserving kExtraInput for convert2Ids, kNormal
for convertMessage, and the existing tensor shape, name, and return behavior;
use the project’s exact MemTypes enum type.
In `@mllm/nn/Functional.hpp`:
- Around line 113-114: Document the exported GQA APIs at all affected sites: in
mllm/nn/Functional.hpp lines 113-114, describe the tensor input layouts, output
layout, supported inputs, and failure behavior for groupedQueryAttentionDecode;
in mllm/nn/layers/GroupedQueryAttentionDecode.hpp lines 11-17, document the
layer purpose, constructor options, forward inputs, output, and errors; and in
mllm/nn/llm_components/GroupedQueryAttention.hpp lines 63-96, document the GQA
contract, CPU and dtype restrictions, return value, and exceptions.
In `@mllm/nn/lmcache/KVHeadStaticCache.cpp`:
- Around line 59-62: Update KVHeadStaticCache::setCurrentSeqCnt to prevent
advancing any slot beyond the sequence count established by updateKVCache;
retain support for valid rollback values and existing range validation, and
ensure getKVCache cannot expose unwritten cache regions.
In `@mllm/nn/lmcache/KVHeadStaticCache.hpp`:
- Line 36: Mark the const value-returning KVHeadStaticCache::getKVCache method
with [[nodiscard]], matching the neighboring accessor declarations and
satisfying modernize-use-nodiscard.
In `@tests/nn/GroupedQueryAttentionTest.cpp`:
- Around line 170-178: Extend the test around groupedQueryAttention to use a
2,048-token KV cache slice, ensuring the decode reads the final KV position at
index 2047 rather than only 201 positions. Preserve the existing shape,
finite-value, and expectNear comparisons against gqaReference for this
maximum-length case.
🪄 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: bb0b206d-5c55-406c-8592-d65e1937120d
⛔ Files ignored due to path filters (1)
docs/assets/minicpm5-pr0-performance-strategy.pngis excluded by!**/*.png
📒 Files selected for processing (47)
README.mdexamples/CMakeLists.txtexamples/minicpm5/CMakeLists.txtexamples/minicpm5/README.mdexamples/minicpm5/benchmark_harness.hppexamples/minicpm5/config_1B_w4a32_kai.jsonexamples/minicpm5/demo_prompt_200.txtexamples/minicpm5/main.cppexamples/minicpm5/quant_cfg_1B_w4a32_kai.jsonmllm/backends/cpu/CMakeLists.txtmllm/backends/cpu/CPUBackend.cppmllm/backends/cpu/kernels/arm/linear/kai.cppmllm/backends/cpu/kernels/common/gqa_decode/fwd_bhsd.hppmllm/backends/cpu/ops/GroupedQueryAttentionDecodeOp.cppmllm/backends/cpu/ops/GroupedQueryAttentionDecodeOp.hppmllm/backends/cpu/ops/LinearOp.cppmllm/backends/cpu/ops/LinearOp.hppmllm/compile/ir/GeneratedRTTIKind.hppmllm/compile/ir/NodeRTTIClassOfImpl.hppmllm/compile/ir/linalg/Op.cppmllm/compile/ir/linalg/Op.hppmllm/compile/ir/rtti_kind_gen.pymllm/compile/jit/binary/LinalgIRSerialization.cppmllm/compile/jit/binary/LinalgIRSerialization.hppmllm/compile/jit/interpreter/AopsFromJson.cppmllm/compile/jit/interpreter/AopsFromJson.hppmllm/core/OpTypes.hppmllm/core/aops/GroupedQueryAttentionDecodeOp.cppmllm/core/aops/GroupedQueryAttentionDecodeOp.hppmllm/models/minicpm5/configuration_minicpm5.hppmllm/models/minicpm5/modeling_minicpm5.hppmllm/models/minicpm5/tokenization_minicpm5.hppmllm/nn/Functional.cppmllm/nn/Functional.hppmllm/nn/Nn.hppmllm/nn/layers/GroupedQueryAttentionDecode.cppmllm/nn/layers/GroupedQueryAttentionDecode.hppmllm/nn/llm_components/GroupedQueryAttention.hppmllm/nn/lmcache/KVHeadStaticCache.cppmllm/nn/lmcache/KVHeadStaticCache.hpptests/cpu/CMakeLists.txttests/cpu/MiniCPM5ConfigTest.cpptests/cpu/MiniCPM5ModelTest.cpptests/cpu/MiniCPM5TokenizerTest.cpptests/nn/CMakeLists.txttests/nn/GroupedQueryAttentionTest.cpptests/nn/KVHeadStaticCacheTest.cpp
| "architectures": [ | ||
| "LlamaForCausalLM" | ||
| ], | ||
| "model_type": "llama", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
architectures and model_type identify Llama, not MiniCPM5.
This file declares "architectures": ["LlamaForCausalLM"] and "model_type": "llama", but it configures the MiniCPM5 runner. MiniCPM5Config ignores both keys, so the runner works today. Any tool that dispatches on model_type — including the conversion pipeline referenced in the README — would select a Llama code path. Either set the values to the MiniCPM5 identifiers, or add a comment in the README that explains why the Llama identifiers are intentional.
🤖 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/minicpm5/config_1B_w4a32_kai.json` around lines 2 - 5, Update the
model identity fields in config_1B_w4a32_kai.json to use the MiniCPM5-specific
architecture and model_type identifiers expected by the runner and conversion
tooling, rather than Llama identifiers; preserve the remaining configuration
unchanged.
| 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.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Check size_error before you trust file_size.
If std::filesystem::file_size fails, it sets size_error and returns static_cast<std::uintmax_t>(-1). That value is not 0, so the code throws "benchmark_jsonl must be new or empty" even though the real fault was a stat failure, for example a permission error. The tool fails closed, which is correct, but the message misdirects the user.
♻️ Proposed change
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");
+ if (std::filesystem::exists(jsonl_path)) {
+ const auto existing_size = std::filesystem::file_size(jsonl_path, size_error);
+ if (size_error) {
+ throw std::invalid_argument("unable to stat benchmark_jsonl: " + size_error.message());
+ }
+ if (existing_size != 0) { throw std::invalid_argument("benchmark_jsonl must be new or empty"); }
}📝 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.
| 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"); | |
| } | |
| const std::filesystem::path jsonl_path(benchmark_jsonl.get()); | |
| std::error_code size_error; | |
| if (std::filesystem::exists(jsonl_path)) { | |
| const auto existing_size = std::filesystem::file_size(jsonl_path, size_error); | |
| if (size_error) { | |
| throw std::invalid_argument("unable to stat benchmark_jsonl: " + size_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/minicpm5/main.cpp` around lines 102 - 106, Update the
benchmark_jsonl validation around jsonl_path to inspect size_error immediately
after std::filesystem::file_size returns, before interpreting its value.
Preserve the fail-closed behavior, but report the filesystem/stat failure
distinctly instead of throwing “benchmark_jsonl must be new or empty” when
file_size cannot be obtained.
| for (int request_index = 0; request_index < total_requests; ++request_index) { | ||
| const bool warmup = request_index < warmup_count; | ||
| nlohmann::json record = { | ||
| {"schema", "mllm.minicpm5.product_benchmark.v1"}, | ||
| {"variant", benchmark_variant.get()}, | ||
| {"source_sha", benchmark_source_sha.get()}, | ||
| {"request_index", request_index}, | ||
| {"warmup", warmup}, | ||
| {"prompt_file", prompt_file.get()}, | ||
| {"prompt_tokens", prompt_tokens}, | ||
| {"prompt_token_ids", prompt_token_ids}, | ||
| {"max_new_tokens", generation_limit}, | ||
| {"min_new_tokens", generation_limit}, | ||
| {"do_sample", false}, | ||
| {"enable_thinking", false}, | ||
| {"cpu_op_threads", mllm::Context::instance().getCpuOpThreads()}, | ||
| }; | ||
| record["telemetry_before"] = mllm::examples::minicpm5::benchmark::captureTelemetry(); | ||
| std::vector<std::string> invalid_reasons; | ||
| if (require_device_telemetry.isSet() && require_device_telemetry.get()) { | ||
| invalid_reasons = mllm::examples::minicpm5::benchmark::validateRequiredTelemetry(record["telemetry_before"]); | ||
| } | ||
|
|
||
| const auto reset_start = std::chrono::steady_clock::now(); | ||
| model.resetState(); | ||
| const auto reset_end = std::chrono::steady_clock::now(); | ||
| std::vector<int64_t> generated_token_ids; | ||
| const auto request_start = std::chrono::steady_clock::now(); | ||
| model.streamGenerate(inputs, | ||
| {{"max_length", mllm::AnyValue(generation_limit)}, | ||
| {"min_new_tokens", mllm::AnyValue(generation_limit)}, | ||
| {"do_sample", mllm::AnyValue(false)}}, | ||
| [&](int64_t token_id) { generated_token_ids.push_back(token_id); }); | ||
| const auto request_end = std::chrono::steady_clock::now(); | ||
| record["telemetry_after"] = mllm::examples::minicpm5::benchmark::captureTelemetry(); | ||
| if (require_device_telemetry.isSet() && require_device_telemetry.get()) { | ||
| auto after_errors = mllm::examples::minicpm5::benchmark::validateRequiredTelemetry(record["telemetry_after"]); | ||
| invalid_reasons.insert(invalid_reasons.end(), after_errors.begin(), after_errors.end()); | ||
| auto stability_errors = mllm::examples::minicpm5::benchmark::validateStableTelemetry(record["telemetry_before"], | ||
| record["telemetry_after"]); | ||
| invalid_reasons.insert(invalid_reasons.end(), stability_errors.begin(), stability_errors.end()); | ||
| } | ||
|
|
||
| const auto stats = model.perfStats(); | ||
| record["generated_token_ids"] = generated_token_ids; | ||
| record["reset_duration_us"] = std::chrono::duration_cast<std::chrono::microseconds>(reset_end - reset_start).count(); | ||
| record["request_wall_duration_us"] = | ||
| std::chrono::duration_cast<std::chrono::microseconds>(request_end - request_start).count(); | ||
| record["stats"] = { | ||
| {"valid", stats.valid}, | ||
| {"completed", stats.completed}, | ||
| {"total_duration_us", stats.total_duration_us}, | ||
| {"prefill_duration_us", stats.prefill_duration_us}, | ||
| {"decode_duration_us", stats.decode_duration_us}, | ||
| {"ttft_duration_us", stats.ttft_duration_us}, | ||
| {"prefill_tokens", stats.prefill_tokens}, | ||
| {"generated_tokens", stats.generated_tokens}, | ||
| {"decode_steps", stats.decode_steps}, | ||
| }; | ||
| if (!stats.valid) { invalid_reasons.push_back("invalid_performance_stats"); } | ||
| if (!stats.completed) { invalid_reasons.push_back("incomplete_generation"); } | ||
| if (stats.prefill_tokens != prompt_tokens) { invalid_reasons.push_back("prefill_token_count_mismatch"); } | ||
| if (stats.generated_tokens != generation_limit || stats.decode_steps != generation_limit - 1 | ||
| || generated_token_ids.size() != static_cast<size_t>(generation_limit)) { | ||
| invalid_reasons.push_back("generation_length_mismatch"); | ||
| } | ||
| record["invalid_reasons"] = invalid_reasons; | ||
| record["status"] = invalid_reasons.empty() ? "ok" : "invalid"; | ||
| jsonl << record.dump() << '\n'; | ||
| jsonl.flush(); | ||
| if (!jsonl) { throw std::runtime_error("failed to write benchmark_jsonl"); } | ||
| if (!invalid_reasons.empty()) { | ||
| exit_code = 2; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A failing warmup request aborts the whole benchmark run.
The loop covers warmup and measured requests together. Lines 212-215 set exit_code = 2 and break whenever invalid_reasons is non-empty, and that applies to warmup requests as well. The first warmup on a cold device is the most likely request to show incomplete telemetry or a governor change, so a transient warmup condition discards the entire measured run.
Decide the intended contract. If warmup requests should not gate the run, skip the abort when warmup is true but keep the record. If they should gate it, state that in the README so the failure is not surprising.
🤖 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/minicpm5/main.cpp` around lines 141 - 216, Update the
invalid-request termination logic in the request loop so warmup records are
still written but do not set exit_code or break the loop when warmup is true.
Measured requests must retain the existing abort behavior for non-empty
invalid_reasons.
| ```bash | ||
| mllm-minicpm5-runner \ | ||
| --model_path /path/to/minicpm5-1b-w4a32-kai.mllm \ | ||
| --model_version v2 \ | ||
| --tokenizer_path /path/to/MiniCPM5-1B/tokenizer.json \ | ||
| --config_path examples/minicpm5/config_1B_w4a32_kai.json \ | ||
| --prompt_file examples/minicpm5/demo_prompt_200.txt \ | ||
| --expected_prompt_tokens 200 \ | ||
| --max_new_tokens 32 \ | ||
| --benchmark_warmup 1 \ | ||
| --benchmark_samples 5 \ | ||
| --benchmark_jsonl /path/to/fresh-results.jsonl \ | ||
| --benchmark_variant minicpm5-1b-w4a32-kai \ | ||
| --benchmark_source_sha SOURCE_COMMIT_SHA \ | ||
| --engine_cpu_op_thread 4 \ | ||
| --engine_dispatcher_thread 4 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document --require_device_telemetry in the reproducible demo command.
The README states that benchmark mode "writes one JSON object per request rather than relying on console timing", and the section is titled "Reproducible 200-token demo". However main.cpp validates telemetry completeness and CPU-frequency drift only when --require_device_telemetry is set, and the flag defaults to off. A user who copies this command gets records that pass with incomplete or drifting telemetry.
Add the flag to the example command, or state explicitly that telemetry gating is opt-in.
🤖 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/minicpm5/README.md` around lines 49 - 65, Update the reproducible
demo command in the README to include the --require_device_telemetry flag,
ensuring copied benchmark runs enforce telemetry completeness and CPU-frequency
drift validation.
| The runner uses greedy decoding with `min_new_tokens=max_new_tokens`, so every valid record contains 32 generated | ||
| tokens and 31 decode steps. Compute prefill throughput from `prefill_tokens / prefill_duration_us`, and decode | ||
| throughput from `decode_steps / decode_duration_us`; the first generated token belongs to TTFT and is not counted as a | ||
| decode step. The JSONL also retains wall time, token IDs, process affinity, and visible CPU/thermal telemetry. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the throughput formulas; the units are wrong.
prefill_tokens / prefill_duration_us yields tokens per microsecond, not tokens per second. The same applies to decode_steps / decode_duration_us. A reader who follows these formulas literally gets a value 1,000,000 times smaller than the tok/s figures the PR reports.
📝 Proposed fix
-tokens and 31 decode steps. Compute prefill throughput from `prefill_tokens / prefill_duration_us`, and decode
-throughput from `decode_steps / decode_duration_us`; the first generated token belongs to TTFT and is not counted as a
-decode step. The JSONL also retains wall time, token IDs, process affinity, and visible CPU/thermal telemetry.
+tokens and 31 decode steps. Compute prefill throughput in tokens per second from
+`prefill_tokens * 1e6 / prefill_duration_us`, and decode throughput from `decode_steps * 1e6 / decode_duration_us`;
+the first generated token belongs to TTFT and is not counted as a decode step. The JSONL also retains wall time,
+token IDs, process affinity, and visible CPU/thermal telemetry.📝 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.
| The runner uses greedy decoding with `min_new_tokens=max_new_tokens`, so every valid record contains 32 generated | |
| tokens and 31 decode steps. Compute prefill throughput from `prefill_tokens / prefill_duration_us`, and decode | |
| throughput from `decode_steps / decode_duration_us`; the first generated token belongs to TTFT and is not counted as a | |
| decode step. The JSONL also retains wall time, token IDs, process affinity, and visible CPU/thermal telemetry. | |
| The runner uses greedy decoding with `min_new_tokens=max_new_tokens`, so every valid record contains 32 generated | |
| tokens and 31 decode steps. Compute prefill throughput in tokens per second from | |
| `prefill_tokens * 1e6 / prefill_duration_us`, and decode throughput from `decode_steps * 1e6 / decode_duration_us`; | |
| the first generated token belongs to TTFT and is not counted as a decode step. The JSONL also retains wall time, | |
| token IDs, process affinity, and visible CPU/thermal telemetry. |
🤖 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/minicpm5/README.md` around lines 67 - 70, Update the throughput
formulas in the README to convert microsecond durations to seconds: multiply the
prefill and decode duration denominators by 1,000,000, or equivalently multiply
each resulting rate by 1,000,000, so both reported values are in tokens per
second.
| BaseOp::ptr_t __groupedQueryAttentionDecodeFromJson(const nlohmann::json& json) { | ||
| aops::GroupedQueryAttentionDecodeOpOptions options; | ||
|
|
||
| DeviceTypes backend = DeviceTypes::kCPU; | ||
| if (json.contains("backend")) { backend = str2DeviceType(json["backend"]); } | ||
|
|
||
| return Context::instance().getBackend(backend)->createOp(OpTypes::kGroupedQueryAttentionDecode, options); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'GroupedQueryAttentionDecode|kGroupedQueryAttentionDecode|createOp\(' \
mllm/backends/cpu/CPUBackend.cpp mllm/engine/Context.hpp mllm/compile/jit/interpreter/AopsFromJson.cpp
rg -n -C3 'DeviceTypes::kDeviceTypes_End|getBackend\(' \
mllm/engine/Context.hpp mllm/core/DeviceTypes.hppRepository: UbiquitousLearning/mllm
Length of output: 44297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Context implementation and backend lookup ---'
rg -n -C8 'Backend::ptr_t Context::getBackend|registerBackend|backend_map|backends_' mllm --glob '*.{cpp,hpp,h}'
printf '%s\n' '--- Backend createOp implementations ---'
rg -n -C8 'createOp\(.*OpTypes|createOp\(const|createOp\(' mllm/backends mllm/core mllm/engine --glob '*.{cpp,hpp,h}'
printf '%s\n' '--- DeviceTypes declarations and all GQA registrations ---'
cat -n mllm/core/DeviceTypes.hpp
rg -n -C3 'CPUGroupedQueryAttentionDecodeOpFactory|GroupedQueryAttentionDecodeOpFactory|kGroupedQueryAttentionDecode' mllm --glob '*.{cpp,hpp,h}'
printf '%s\n' '--- Backend files ---'
git ls-files 'mllm/backends/**' | rg '(Backend|backend).*(cpp|hpp|h)$'Repository: UbiquitousLearning/mllm
Length of output: 49996
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- SymbolTable lookup semantics ---'
rg -n -C10 'class SymbolTable|struct SymbolTable|operator\[\]|bool has\(' mllm --glob '*.{cpp,hpp,h}'
printf '%s\n' '--- Backend constructors and registered operation factories ---'
for f in \
mllm/backends/cuda/CudaBackend.cpp \
mllm/backends/opencl/OpenCLBackend.cpp \
mllm/backends/qnn/QNNBackend.cpp \
mllm/backends/ascend/AscendBackend.cpp \
mllm/backends/cpu/CPUBackend.cpp; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f" | sed -n '1,130p'
fi
done
printf '%s\n' '--- Error macro behavior ---'
rg -n -C8 'define MLLM_ERROR_EXIT|MLLM_ERROR_EXIT\(' mllm --glob '*.{cpp,hpp,h}' | head -160Repository: UbiquitousLearning/mllm
Length of output: 48088
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
gqa = "kGroupedQueryAttentionDecode"
cpu = Path("mllm/backends/cpu/CPUBackend.cpp").read_text(encoding="utf-8")
backend = Path("mllm/backends/base/Backend.cpp").read_text(encoding="utf-8")
context = Path("mllm/engine/Context.cpp").read_text(encoding="utf-8")
symbols = Path("mllm/utils/SymbolTable.hpp").read_text(encoding="utf-8")
log = Path("mllm/utils/Log.hpp").read_text(encoding="utf-8")
assert "CPUGroupedQueryAttentionDecodeOpFactory" in cpu
assert "op_factories_[op_type]->create" in backend
assert 'if (!has(name))' in symbols and "MLLM_ERROR_EXIT" in symbols
assert "if (!backends_.has(device))" in context and "MLLM_ERROR_EXIT" in context
assert "abort();" in log
for path in [
"mllm/backends/cuda/CudaBackend.cpp",
"mllm/backends/opencl/OpenCLBackend.cpp",
"mllm/backends/qnn/QNNBackend.cpp",
"mllm/backends/ascend/AscendBackend.cpp",
]:
text = Path(path).read_text(encoding="utf-8")
assert gqa not in text, path
print("CPU registers the GQA factory.")
print("Other inspected backend constructors do not register the GQA factory.")
print("Invalid backend lookup and missing operation-factory lookup call MLLM_ERROR_EXIT, which aborts.")
PYRepository: UbiquitousLearning/mllm
Length of output: 358
Reject non-CPU and invalid backends before createOp.
These paths call MLLM_ERROR_EXIT, which aborts the process. Validate that backend is kCPU before the lookup.
🤖 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/compile/jit/interpreter/AopsFromJson.cpp` around lines 601 - 608, Update
__groupedQueryAttentionDecodeFromJson to validate the parsed backend before
calling Context::instance().getBackend or createOp. Reject invalid values and
any backend other than DeviceTypes::kCPU via MLLM_ERROR_EXIT, while preserving
the existing CPU operation creation path.
|
|
||
| void setup(const std::vector<Tensor>& inputs, std::vector<Tensor>& outputs) override; | ||
|
|
||
| inline const GroupedQueryAttentionDecodeOpOptions& options() const { return options_; } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'CMAKE_CXX_STANDARD|cxx_std_|modernize-use-nodiscard' \
-g 'CMakeLists.txt' -g '*.cmake' -g '.clang-tidy' .Repository: UbiquitousLearning/mllm
Length of output: 1833
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- clang-tidy configuration files ---'
find . -name '.clang-tidy' -o -name '_clang-tidy' | sort
printf '%s\n' '--- nodiscard rule references ---'
rg -n -C3 'modernize-use-nodiscard|nodiscard' \
-g '.clang-tidy' -g '*.yaml' -g '*.yml' -g '*.cmake' -g 'CMakeLists.txt' -g 'Makefile*' .
printf '%s\n' '--- target header ---'
nl -ba mllm/core/aops/GroupedQueryAttentionDecodeOp.hpp | sed -n '1,70p'
printf '%s\n' '--- related accessors and attributes ---'
rg -n -C2 '\boptions\(\)|\[\[nodiscard\]\]' mllm mllm-cli pymllm \
-g '*.{h,hh,hpp,c,cc,cpp,cxx,py,pyi,sh}'Repository: UbiquitousLearning/mllm
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- clang-tidy configuration ---'
cat -n .clang-tidy
printf '%s\n' '--- target header ---'
cat -n mllm/core/aops/GroupedQueryAttentionDecodeOp.hpp | sed -n '1,70p'
printf '%s\n' '--- related accessors and attributes ---'
rg -n -C2 '\boptions\(\)|\[\[nodiscard\]\]' mllm mllm-cli pymllm \
-g '*.{h,hh,hpp,c,cc,cpp,cxx,py,pyi,sh}' || trueRepository: UbiquitousLearning/mllm
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- clang-tidy availability ---'
command -v clang-tidy || true
clang-tidy --version 2>/dev/null || true
printf '%s\n' '--- lint invocations and exclusions ---'
rg -n -C3 'clang-tidy|WarningsAsErrors|HeaderFilterRegex|CMAKE_CXX_CLANG_TIDY' \
-g '*.yml' -g '*.yaml' -g '*.json' -g '*.toml' -g '*.sh' -g 'CMakeLists.txt' -g '*.cmake' -g 'Makefile*' . || true
printf '%s\n' '--- focused options() declarations ---'
rg -n -C1 'inline const [A-Za-z0-9_:]+& options\(\) const \{ return options_; \}' mllm/core/aops \
-g '*.hpp' -g '*.h' || true
printf '%s\n' '--- focused nodiscard count in aops ---'
rg -n '\[\[nodiscard\]\].*\boptions\(\)' mllm/core/aops \
-g '*.hpp' -g '*.h' | head -40 || trueRepository: UbiquitousLearning/mllm
Length of output: 7803
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- target-header diagnostic ---'
clang-tidy mllm/core/aops/GroupedQueryAttentionDecodeOp.hpp \
-checks=-*,modernize-use-nodiscard \
-- -std=c++20 -I. 2>&1 || true
printf '%s\n' '--- minimal accessor diagnostic ---'
tmp_file="$(mktemp --suffix=.cpp)"
trap 'rm -f "$tmp_file"' EXIT
cat >"$tmp_file" <<'CPP'
struct Options {};
struct Op {
inline const Options& options() const { return options_; }
private:
Options options_;
};
int main() {
Op op;
(void)op.options();
return 0;
}
CPP
clang-tidy "$tmp_file" \
-checks=-*,modernize-use-nodiscard \
-- -std=c++20 2>&1 || trueRepository: UbiquitousLearning/mllm
Length of output: 4977
Add [[nodiscard]] to options().
modernize-use-nodiscard reports this accessor as an error, and C++20 supports the attribute.
🧰 Tools
🪛 Clang (14.0.6)
[error] 30-30: function 'options' should be marked [[nodiscard]]
(modernize-use-nodiscard,-warnings-as-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/core/aops/GroupedQueryAttentionDecodeOp.hpp` at line 30, Update the
const accessor GroupedQueryAttentionDecodeOp::options() to include the C++20
[[nodiscard]] attribute, preserving its existing return type and behavior.
Source: Linters/SAST tools
| inline Tensor groupedQueryAttentionEager(const Tensor& query, const Tensor& key, const Tensor& value) { | ||
| const auto q_shape = query.shape(); | ||
| const auto k_shape = key.shape(); | ||
| const auto v_shape = value.shape(); | ||
| const int32_t groups = q_shape[1] / k_shape[1]; | ||
| const float scale = 1.0F / std::sqrt(static_cast<float>(q_shape[3])); | ||
| const int32_t context_offset = k_shape[2] - q_shape[2]; | ||
| auto causal_mask = Tensor::zeros({q_shape[0], 1, q_shape[2], k_shape[2]}, kFloat32, kCPU); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent direct calls from bypassing validation.
groupedQueryAttentionEager is an inline function in a public header. It reads tensor dimensions before validation. A tensor with fewer than four dimensions can cause out-of-bounds access. A tensor with zero KV heads can cause division by zero at q_shape[1] / k_shape[1].
Make this helper private to the validated path, or apply the same validation before any shape access.
As per coding guidelines, validate inputs for public APIs and critical internal functions.
🤖 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/nn/llm_components/GroupedQueryAttention.hpp` around lines 15 - 22,
Ensure groupedQueryAttentionEager cannot access tensor shapes before validating
dimensionality and KV-head count: either make it private to the
already-validated caller or add equivalent validation at its entry. Reject
tensors with fewer than four dimensions and zero key/value heads before
computing groups, scale, or context_offset, while preserving the existing
validated execution path.
Source: Coding guidelines
| const size_t k_head_bytes = static_cast<size_t>(sequence) * static_cast<size_t>(head_dim_) | ||
| * static_cast<size_t>(bytesOfType(k_dtype_)) / static_cast<size_t>(lanesOfType(k_dtype_)); | ||
| const size_t v_head_bytes = static_cast<size_t>(sequence) * static_cast<size_t>(head_dim_) | ||
| * static_cast<size_t>(bytesOfType(v_dtype_)) / static_cast<size_t>(lanesOfType(v_dtype_)); | ||
|
|
||
| for (int32_t head = 0; head < kv_heads_; ++head) { | ||
| auto* k_dst = k_cache_[logical_slot].offsettedPtr<mllm_byte_t>({0, head, current, 0}); | ||
| auto* v_dst = v_cache_[logical_slot].offsettedPtr<mllm_byte_t>({0, head, current, 0}); | ||
| const auto* k_src = k.coffsettedPtr<mllm_byte_t>({0, head, 0, 0}); | ||
| const auto* v_src = v.coffsettedPtr<mllm_byte_t>({0, head, 0, 0}); | ||
| std::memcpy(k_dst, k_src, k_head_bytes); | ||
| std::memcpy(v_dst, v_src, v_head_bytes); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the semantics of bytesOfType and lanesOfType for packed dtypes.
rg -nP -C6 '\b(bytesOfType|lanesOfType)\s*\(' mllm/core/DataTypes.hpp mllm/core/DataTypes.cppRepository: UbiquitousLearning/mllm
Length of output: 2262
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Data type definitions and implementations ---'
sed -n '1,130p' mllm/core/DataTypes.cpp
rg -n -C5 'k(Int|UInt|Float|BFloat|Q|NF|GPTQ|AWQ)|lanes\(\)|bytes\(\)' mllm/core/DataTypes.hpp | head -n 260
printf '%s\n' '--- Cache implementation and declarations ---'
sed -n '1,180p' mllm/nn/lmcache/KVHeadStaticCache.cpp
fd -i 'KVHeadStaticCache' .
for f in $(fd -i 'KVHeadStaticCache' .); do
printf '\n--- %s ---\n' "$f"
sed -n '1,220p' "$f"
done
printf '%s\n' '--- Cache call sites and dtype construction ---'
rg -n -C4 'KVHeadStaticCache|k_dtype_|v_dtype_|lanesOfType|bytesOfType' mllm mllm-cli pymllm --glob '*.{c,cc,cpp,cxx,h,hh,hpp,py,pyi,sh}' --glob '*.md' --glob '*.yml' --glob '*.yaml' | head -n 400Repository: UbiquitousLearning/mllm
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Quantized type metadata ---'
sed -n '630,731p' mllm/core/DataTypes.hpp
rg -n 'MLLM_DEFINE_QUANT_TYPE_INFO|struct mllm_block|QK[0-9_]*|kGGUF_|kMXFP4|kInt4|kUInt4' mllm/core/DataTypes.hpp mllm/core/DataTypes.cpp | head -n 180
printf '%s\n' '--- Tensor storage and pointer arithmetic ---'
sed -n '30,65p' mllm/core/TensorStorage.cpp
sed -n '55,95p' mllm/core/TensorViewImpl.hpp
rg -n -C4 'contiguous\(\)|stride_|offsettedPtr' mllm/core/TensorViewImpl.cpp mllm/core/TensorViewImpl.hpp mllm/core/Tensor.cpp | head -n 180
printf '%s\n' '--- Minimal arithmetic verifier ---'
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class Case:
name: str
lanes: int
bytes_per_lane_group: int
heads: int
max_length: int
head_dim: int
sequence: int
current: int
def report(c):
total_cache_elements = c.heads * c.max_length * c.head_dim
total_input_elements = c.heads * c.sequence * c.head_dim
cache_valid = total_cache_elements % c.lanes == 0
input_valid = total_input_elements % c.lanes == 0
per_head_elements = c.sequence * c.head_dim
copied = per_head_elements * c.bytes_per_lane_group // c.lanes
expected_if_independent_heads = (per_head_elements * c.bytes_per_lane_group + c.lanes - 1) // c.lanes
src_offset_elements = c.heads * 0 # first head starts at zero
dst_offset_elements = c.current * c.head_dim
print(c.name)
print({
"cache_shape_storage_valid": cache_valid,
"input_shape_storage_valid": input_valid,
"per_head_elements": per_head_elements,
"memcpy_bytes": copied,
"per_head_byte_count_if_rounded": expected_if_independent_heads,
"destination_offset_elements": dst_offset_elements,
"destination_lane_aligned": dst_offset_elements % c.lanes == 0,
"head_dim_lane_aligned": c.head_dim % c.lanes == 0,
})
# Q4_0 metadata is 32 logical elements per 18-byte block.
report(Case("two heads, one 16-element row", 32, 18, 2, 16, 16, 1, 0))
report(Case("two heads, aligned 32-element rows", 32, 18, 2, 64, 32, 1, 1))
# A non-quantized packed example, if supported by metadata, has the same offset issue.
report(Case("two heads, four-element rows, four-lane group", 4, 2, 2, 8, 4, 1, 1))
PYRepository: UbiquitousLearning/mllm
Length of output: 32331
Reject or align lane-packed dtypes before copying.
lanesOfType(...) > 1 includes GGUF block types and kMXFP4, not only sub-byte types. Valid tensor shapes can still have non-lane-aligned head_dim_. In that case, offsettedPtr floors element offsets and the byte count omits partial blocks, which can alias or corrupt per-head data. Reject these dtypes, or require head_dim_ to be lane-aligned for both key and value dtypes.
🤖 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/nn/lmcache/KVHeadStaticCache.cpp` around lines 83 - 95, Validate
lane-packed key and value dtypes before the memcpy loop in the cache update
method: when lanesOfType(k_dtype_) or lanesOfType(v_dtype_) is greater than one,
require head_dim_ to be divisible by the corresponding lane count, otherwise
reject the operation. Preserve the existing copy path only for aligned shapes,
preventing offsettedPtr and byte-size calculations from aliasing or truncating
per-head data.
Source: Coding guidelines
| | [Qwen3-4B](https://github.com/QwenLM/Qwen3) | [✔️ w4a8](https://www.modelscope.cn/models/mllmTeam/Qwen3-4B-w4a8-i8mm-kai) | | | | ||
| | [Qwen3.5-0.8B](https://huggingface.co/Qwen/Qwen3.5-0.8B) | [✔️ w4a8](./examples/qwen3_5/README.md) | | | | ||
| | [Qwen3.5-4B](https://huggingface.co/Qwen/Qwen3.5-4B) | [✔️ w4a8](./examples/qwen3_5/README.md) | | | | ||
| | [MiniCPM5-1B](https://huggingface.co/openbmb/MiniCPM5-1B) | [✔️ w4a8](./examples/minicpm5/README.md) | | | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The CPU column says w4a8, but this example supports W4A32.
The PR objectives, examples/minicpm5/README.md, config_1B_w4a32_kai.json, and quant_cfg_1B_w4a32_kai.json all describe W4A32 KAI weights with FP32 activations. This row advertises w4a8, which tells a reader that INT8 activations are supported. Correct the label to w4a32.
📝 Proposed fix
-| [MiniCPM5-1B](https://huggingface.co/openbmb/MiniCPM5-1B) | [✔️ w4a8](./examples/minicpm5/README.md) | | |
+| [MiniCPM5-1B](https://huggingface.co/openbmb/MiniCPM5-1B) | [✔️ w4a32](./examples/minicpm5/README.md) | | |📝 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.
| | [MiniCPM5-1B](https://huggingface.co/openbmb/MiniCPM5-1B) | [✔️ w4a8](./examples/minicpm5/README.md) | | | | |
| | [MiniCPM5-1B](https://huggingface.co/openbmb/MiniCPM5-1B) | [✔️ w4a32](./examples/minicpm5/README.md) | | | |
🤖 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 `@README.md` at line 109, Update the MiniCPM5-1B row in the README CPU column
to label the example as w4a32 instead of w4a8, matching the configuration and
documented W4A32 support.
2 participants
Summary
This PR adds CPU inference support for the official MiniCPM5-1B checkpoint and the native KV-head cache/GQA foundation needed by grouped-query models.
Reviewer headline: persistent K/V is stored at the model's 2 KV heads instead of its 16 query heads, reducing the 2,048-token FP32 cache from approximately 768 MiB to 96 MiB across 24 layers. Single-token FP32 decode uses a new stride-aware GQA CPU kernel, exposed through the complete mllm Layer/AOps/IR/Backend path. The model does not call backend code directly.
KVHeadStaticCachestorage in[batch, kv_heads, sequence, head_dim]layout.GroupedQueryAttentionDecodeoperation and its scalar fallback.P @ Vdecode reuse and C19's packed-weight-compatible I8MM prefill dispatch.Reviewer TL;DR
KVHeadStaticCache/MiniCPM5.GroupedQueryAttentionDecodeis stateless.OpTypes/AOps -> eager CPU factory/backend op/kernel, plus linalg IR/RTTI and serialization/interpreter reconstruction.Review map
Reviewing in this order keeps state ownership, semantic registration, and backend optimization separate:
mllm/nn/lmcache/KVHeadStaticCache.{hpp,cpp},tests/nn/KVHeadStaticCacheTest.cppmllm/nn/layers/GroupedQueryAttentionDecode.{hpp,cpp},mllm/nn/Functional.{hpp,cpp},mllm/models/minicpm5/modeling_minicpm5.hppmllm/core/OpTypes.hpp,mllm/core/aops/GroupedQueryAttentionDecodeOp.*,mllm/compile/ir/linalg/Op.*,mllm/compile/jit/{binary,interpreter}/mllm/backends/cpu/CPUBackend.cpp,mllm/backends/cpu/ops/GroupedQueryAttentionDecodeOp.*,mllm/backends/cpu/kernels/common/gqa_decode/fwd_bhsd.hppM=1decode routemllm/backends/cpu/ops/LinearOp.*,mllm/backends/cpu/kernels/arm/linear/kai.cppmllm/models/minicpm5/,examples/minicpm5/,tests/{cpu,nn}/Supported scope
openbmb/MiniCPM5-1B@4e9de7a0778dc1c362e983e6858f0e77542cbdcahead_dim=128Not included: QNN/GPU execution, paged KV allocation, block tables, or a PagedAttention scheduler integration.
Why native KV-head storage is required — cache math and head mapping
The previous eager
StaticCacheallocated storage at the query-head count and copied every KV head once per query-head group. MiniCPM5-1B has 16 query heads and 2 KV heads, so the old representation stored each K/V head eight times.At the current FP32, 24-layer, 2,048-token mobile cache limit:
[1, 16, 2048, 128][1, 2, 2048, 128]The query-to-KV mapping is:
Queries and outputs remain independent; only K/V history is shared.
Profiling of the original single-token eager path attributed 81.4% of decode time to GQA and 13.8% to KAI Linear. That result motivated optimizing the native-stride attention consumer instead of expanding K/V back to query-head count.
Architecture and invariants
The formal decode operation is stateless and receives current cache views explicitly:
The important invariants are:
Hq % Hkv == 0, K/V share batch, KV-head, and sequence dimensions, and Q/K shareDqk.Hq.Validation status
Candidate commit:
193c0a0d359db9765b1f0d0fdd9e842073dc553b. Upstreammainobserved at PR creation:49782817bdaa00209962ea9108fdf5b864aed823; merge base:9a0a21ded8567076c37edb17f91f639a031500a3. The retained C19 evidence is separately bound to08bca1c223cdda65b4f43c4865a9f5914ed9b743+c19-82ccb5a8f68147ae; its source-manifest SHA-256 is82ccb5a8f68147ae708ebf7e758f77ba6d9d419fe04a35ca07984e441f897707.Detailed validation matrix
git diff --check; abstraction-boundary audit reports 0 errors and one pre-existing MiniCPM raw-tensor orchestration warning; targeted clang-format dry-run passesMllm-Test-Nn-GroupedQueryAttention,Mllm-Test-MiniCPM5-Model, andmllm-minicpm5-runnerbuild; runner--helpexits 04ad2be8fd10dc5ffae2e809ee8725f279f80275337f0da339cef467faa49c0b0Performance
Evidence boundary: the figures below are exact-artifact C09-C19 OnePlus 13T results. The later standard-operation integration leaves
mllm/backends/cpu/kernels/common/gqa_decode/fwd_bhsd.hppbyte-identical at SHA-25679a625407c922b493026b7715f043b62ef636d3e849029ab0314d17f97b07f0f. These results support the retained kernel mechanisms, not current-head end-to-end dispatcher performance.P @ Vreuse across 8 query headsM=1is unchangedDetailed paired full-model results and run contracts
All formal comparisons used the same exact 200-token prompt, 32 generated tokens, greedy decoding, verified loaded paths, and paired execution orders.
C09 -> C10: scalar native-stride to shared-vector GQA
The formal OnePlus 13T comparison used
ABBA-BAAB-AB, one warmup per block, five measured requests per variant, 31 steady-state decode steps, and the same measured CPU frequency-ceiling vector for retained samples.All five paired decode comparisons improved, with gains from +44.02% to +64.89%. Generated token IDs were deterministic within each variant.
C10 -> C14: grouped
P @ VreuseC14 changes only
P @ V: QK and softmax keep C10's per-query-head order, while the eight query heads sharing one KV head consume each V token together. The screen usedABBA-BAAB, four measured requests per variant, and the same 200/32/31 workload.P @ VBoth decode order halves improved (+33.49%, +13.06%); all four paired comparisons were positive (+28.22%, +39.03%, +21.03%, +5.77%). C14 does not change multi-token prefill, so the observed prefill difference is not attributed to this mechanism.
C14 -> C19: prefill-only I8MM
C19 retains C14 decode and changes only eligible multi-token KAI Linear dispatch to a packed-weight-compatible I8MM tile. The screen used fresh processes,
ABBA-BAAB, four measured requests per variant, and the same 200/32/31 workload.Both prefill order halves improved (+97.14%, +60.71%); all four paired prefill comparisons were positive (+122.03%, +72.73%, +62.40%, +58.97%). Every C19 process activated I8MM prefill exactly once, and forced DotProd fallback produced the frozen output. Since
M=1is unchanged, the decode delta is not attributed to I8MM.Focused operator oracle and thread-policy decision
The C10 OnePlus operator oracle compared against an independent scalar reference:
Hq=4, Hkv=2, Skv=4, Dqk=5, Dv=3tail2.98e-8Hq=16, Hkv=2, Skv=201, D=1282.53e-7Hq=16, Hkv=2, Skv=2048, D=1286.02e-8The C14 exact-function gate was bitwise equal to C10 in 5/5 cases. Median latency reductions were 1.69% at
Skv=32, 10.19% atSkv=201, and 9.38% atSkv=2048; both order halves were positive for every shape.The CLI requested four CPU operator threads, but profiling proved the retained Linear options dispatched eight threads to each KAI W4A32 call. Both variants in every paired comparison used the same actual behavior. A global 1/2/4/8 sweep found eight threads fastest. A small-N single-thread policy showed +2.60% aggregate decode but reversed across order halves (+6.25%, -2.74%), so it was rejected.
H20 was used for correctness and Android builds, not as an ARM performance proxy.
200-token demo workload and sample output
The runner includes a prompt whose final official no-tool chat-template input is exactly 200 tokens. The validated workload uses greedy generation with 32 output tokens and 31 steady-state decode steps.
Known limitations and follow-up
Summary by CodeRabbit
New Features
Documentation
Tests