Skip to content

GLM 5.2 Indexer support#25407

Merged
ngxson merged 13 commits into
ggml-org:masterfrom
pcuenca:glm-dsa-indexer
Jul 24, 2026
Merged

GLM 5.2 Indexer support#25407
ngxson merged 13 commits into
ggml-org:masterfrom
pcuenca:glm-dsa-indexer

Conversation

@pcuenca

@pcuenca pcuenca commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Overview

Adds indexer support to GLM 5.2. Currently, indexer tensors are loaded when present, but ignored. If accepted, this PR would in my understanding complete the implementation of GLM 5.2 in terms of modeling.

The implementation just copies the graph from Deepseek 3.2 and applies the GLM 5.2 changes:

  • Only "full" indexers compute top_k, "shared" ones reuse top_k from the previous full layer. In Deepseek 3.2, all indexers are "full".
  • The indexer uses interleaved rope, instead of the neox-style used by Deepseek 3.2.

Additional information

  • Existing unsloth checkpoints work without conversion. They contain indexer weights at every layer, but only those in "full" indexers are used.
  • Here's another checkpoint, quantized using this PR, that can be used for testing purposes. It uses unsloth's imatrix, and preserves indexer weights in Q8_0.
  • I verified that generations in greedy mode match the previous implementation for short sequences (indexers are essentially a no-op the first 2048 tokens). I have verified that long sequences remain coherent, but have not compared KLD against a reference implementation.
  • I was a bit surprised to find decoding speed to be much slower than with the previous (deepseek2-based) implementation. I haven't investigated the reason yet, if there are ways to make it faster, or if it happens across all sequence lengths.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES, as follows:
    • All code, debugging, and testing performed by a human 🙋‍♂️
    • PR description written by the same person
    • AI was used to ask about the codebase, navigate it, find patterns, identify idiomatic approaches.

pcuenca added 6 commits July 5, 2026 19:17
This is transformers' `apply_rotary_pos_emb_interleave`
Previous converted GGUFs like https://huggingface.co/unsloth/GLM-5.2-GGUF write indexer weights to _all_ layers, even if they are only required for "full" types. This PR relies on a new key "%s.attention.indexer.types"; if absent, it will use the default GLM 5.2 schedule as defined in https://huggingface.co/zai-org/GLM-5.2/blob/main/config.json#L26.

Note that conversion is not saving this key yet.
@pcuenca
pcuenca requested review from CISC and ggerganov as code owners July 7, 2026 14:18
Comment thread src/models/glm-dsa.cpp Outdated
#include "llama-kv-cache-dsa.h"

// https://huggingface.co/zai-org/GLM-5.2/blob/main/config.json#L26
const std::array<uint32_t, LLAMA_MAX_LAYERS> GLM_DSA_DEFAULT_INDEXER_TYPES = {

@pcuenca pcuenca Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered the alternative to store just two numbers to represent the frequency of full indexers, and the number of prefix full indexers before the regular pattern. I decided towards the array because:

  • There are also precedents in the codebase for irregular patterns.
  • Deepseek 3.2 could be included in this pattern via get_key_or_arr with a single scalar (all DS indexers are "full").

Comment thread src/models/glm-dsa.cpp

switch (hparams.n_layer()) {
case 79: type = LLM_TYPE_744B_A40B; break;
case 78: type = LLM_TYPE_744B_A40B; break;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a previous inoffensive bug introduced during the n_layer refactor; n_layer() excludes MTP layers so this should be 78.

Comment thread src/models/glm-dsa.cpp
return std::make_unique<graph>(*this, params);
}

llama_model_glm_dsa::graph::graph(const llama_model & model, const llm_graph_params & params) :

@pcuenca pcuenca Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same graph as Deepseek v3.2, except for the two differences noted in the PR description.

I didn't want to touch other modules, but we can generalize these changes into Deepseek's and reuse the graph here.

@github-actions github-actions Bot added model Model specific conversion labels Jul 7, 2026
@am17an

am17an commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

As I understand GLM 5.2 uses the same lighting indexer as DS3.2 but the top-k is shared across 4 layers. If this is correct then #24231 should be used

@pcuenca

pcuenca commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

As I understand GLM 5.2 uses the same lighting indexer as DS3.2 but the top-k is shared across 4 layers. If this is correct then #24231 should be used

Yes, indeed, thanks for linking to it, I had missed the recent activity there.

My understanding is that #24231 is about creating a new op that we can use from both the Deepseek and gml-dsa modeling files when it's merged, and when cuda / metal implementations are added. This PR (mine) is about contributing the remaining aspects of the architecture that were deferred from the initial GLM 5.2 implementation, and that are mostly available in the deepseek 3.2 one.

But I'm new to the project, happy to coordinate with the team as necessary!

@am17an

am17an commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Yes, my comment is for your awareness w.r.t. to the lightning indexer and not anything else different for GLM5.2

@Dorijan10

Copy link
Copy Markdown

Tested this branch on 8× RTX 5090 (sm_120), GLM-5.2 UD-IQ2_M, all layers on
GPU, fa=1, q8_0 KV. This PR vs master (dense) on identical weights and flags
via llama-bench:

depth pp512 master pp512 PR Δ tg64 master tg64 PR
0 758.8 652.7 -14.0% 52.67 49.26
2048 704.4 566.0 -19.6% 51.05 44.99
8192 564.0 426.0 -24.5% 47.21 42.79

(depth-0 decode averaged over 5 repeats; gap stable and well outside the error bars.)

So I reproduce the slowdown you flagged, and it has a clear shape: the prefill
gap widens monotonically with depth (14% → 24.5%), and there's a ~6.5% decode
hit even at depth 0, where top-k selects all tokens and the indexer should be a
near no-op. I believe we have two stacked costs: a fixed indexer overhead on the
full layers, and a depth-scaling cost from attending over the full sequence
(masked flash-attn) rather than gathering the selected keys.

I also confirmed correctness holds: greedy generations are byte-for-byte
identical to master below 2048 tokens, as expected.

The wiring looks right; from the perfomance side looks like it wants a fused
indexer/sparse-attention kernel doing an O(L·k) gather over the selected keys
instead of the generic masked path, which lines up with the dedicated-op
direction in #24231. I have the hardware to develop and validate that on
sm_120 and would like to take it on as a follow-up. Happy to share full logs /
op-placement traces, and to run KLD-vs-reference on the q3_k_m checkpoint.
cc @pcuenca @am17an

@pcuenca

pcuenca commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @Dorijan10 for the tests 🙌

Yes, this PR is about fidelity to the original modeling code, and as explained it follows exactly the same pattern Deepseek v3.2 does.

I think performance updates would have to be adopted separately, as @am17an pointed out. We could also do everything at once, but I thought it'd be easier for review this way.

@fairydreaming

fairydreaming commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

@pcuenca I'm testing this PR on my machine with GLM-5.1 as I have it on disk. Overall it looks good, but I found the following issues so far:

  • I don't think it's a good idea to use GLM 5.2 indexer usage pattern as default. This code is also used for GLM 5 and GLM 5.1, AFAIK they used indexer in all layers and this should be the default setting. @ngxson please correct me if I'm wrong.
  • after merging lightning indexer PRs indexer mask is now F16 if ggml_lightning_indexer() is available for the used backend. Unfortunately it fails with your code as you can't add F16 mask to F32 indexer scores in GGML. This can be fixed with this patch (adds ggml_lightning_indexer() call if it's available and uses previous indexer implementation if it's not), I did the same changes to DeepSeek V3.2 code few days ago:
diff --git a/src/models/glm-dsa.cpp b/src/models/glm-dsa.cpp
index f7109cbd4..2687266bd 100644
--- a/src/models/glm-dsa.cpp
+++ b/src/models/glm-dsa.cpp
@@ -328,43 +328,50 @@ llama_model_glm_dsa::graph::graph(const llama_model & model, const llm_graph_par
                 indexer_q = ggml_view_4d(ctx0, indexer_q, indexer_q->ne[0], indexer_q->ne[1], indexer_q->ne[2]/n_stream, n_stream, indexer_q->nb[1], indexer_q->nb[2], indexer_q->nb[3]/n_stream, 0);
                 indexer_weights = ggml_view_4d(ctx0, indexer_weights, indexer_weights->ne[0], indexer_weights->ne[1]/n_stream, indexer_weights->ne[2], n_stream, indexer_weights->nb[1], indexer_weights->nb[2]/n_stream, indexer_weights->nb[3]/n_stream, 0);
 
-                // calculate indexer kq
-                indexer_q = ggml_permute(ctx0, indexer_q, 0, 2, 1, 3);
-                cb(indexer_q, "indexer_q", il);
-                indexer_k = ggml_permute(ctx0, indexer_k, 0, 2, 1, 3);
-                cb(indexer_k, "indexer_k", il);
-
-                ggml_tensor * indexer_kq = ggml_mul_mat(ctx0, indexer_k, indexer_q);
-                cb(indexer_kq, "indexer_kq", il);
-
-                // ReLU requires contiguous tensors
-                indexer_kq = ggml_cont(ctx0, ggml_permute(ctx0, indexer_kq, 2, 1, 0, 3));
-                cb(indexer_kq, "indexer_kq", il);
-
-                // apply ReLU
-                ggml_tensor * indexer_score = ggml_relu(ctx0, indexer_kq);
-                cb(indexer_score, "indexer_score", il);
-
                 // pre-scale weights to avoid scaling operations on huge indexer_score tensor
                 indexer_weights = ggml_scale(ctx0, indexer_weights, 1.0f / sqrtf(float(n_embd_indexer_head * n_indexer_head)));
                 cb(indexer_weights, "indexer_weights", il);
 
-                // multiply scores by indexer weights
-                indexer_score = ggml_mul(ctx0, indexer_score, indexer_weights);
-                cb(indexer_score, "indexer_score", il);
-
-                // sum by q n_indexer_head dimension
-                indexer_score = ggml_sum_rows(ctx0, indexer_score);
-                cb(indexer_score, "indexer_score", il);
-
-                // permute result to match KQ mask
-                indexer_score = ggml_cont(ctx0, ggml_permute(ctx0, indexer_score, 2, 1, 0, 3));
-                cb(indexer_score, "indexer_score", il);
-
-                // mask indexer scores
-                ggml_tensor * indexer_kq_mask = inp_attn_dsa->get_kq_mask_lid();
-                indexer_score = ggml_add(ctx0, indexer_score, indexer_kq_mask);
-                cb(indexer_score, "indexer_score", il);
+                ggml_tensor * indexer_score = nullptr;
+                if (cparams.fused_lid) {
+                    indexer_score = ggml_lightning_indexer(ctx0, indexer_q, indexer_k, indexer_weights, inp_attn_dsa->get_kq_mask_lid());
+                    cb(indexer_score, "indexer_score", il);
+                    res->add_fused_node({LLM_FUSED_OP_LIGHTNING_INDEXER, indexer_score, il});
+                } else {
+                    // calculate indexer kq
+                    indexer_q = ggml_permute(ctx0, indexer_q, 0, 2, 1, 3);
+                    cb(indexer_q, "indexer_q", il);
+                    indexer_k = ggml_permute(ctx0, indexer_k, 0, 2, 1, 3);
+                    cb(indexer_k, "indexer_k", il);
+
+                    ggml_tensor * indexer_kq = ggml_mul_mat(ctx0, indexer_k, indexer_q);
+                    cb(indexer_kq, "indexer_kq", il);
+
+                    // ReLU requires contiguous tensors
+                    indexer_kq = ggml_cont(ctx0, ggml_permute(ctx0, indexer_kq, 2, 1, 0, 3));
+                    cb(indexer_kq, "indexer_kq", il);
+
+                    // apply ReLU
+                    indexer_score = ggml_relu(ctx0, indexer_kq);
+                    cb(indexer_score, "indexer_score", il);
+
+                    // multiply scores by indexer weights
+                    indexer_score = ggml_mul(ctx0, indexer_score, indexer_weights);
+                    cb(indexer_score, "indexer_score", il);
+
+                    // sum by q n_indexer_head dimension
+                    indexer_score = ggml_sum_rows(ctx0, indexer_score);
+                    cb(indexer_score, "indexer_score", il);
+
+                    // permute result to match KQ mask
+                    indexer_score = ggml_cont(ctx0, ggml_permute(ctx0, indexer_score, 2, 1, 0, 3));
+                    cb(indexer_score, "indexer_score", il);
+
+                    // mask indexer scores
+                    ggml_tensor * indexer_kq_mask = inp_attn_dsa->get_kq_mask_lid();
+                    indexer_score = ggml_add(ctx0, indexer_score, indexer_kq_mask);
+                    cb(indexer_score, "indexer_score", il);
+                }
 
                 // get indices of top k indexer scores
                 uint32_t n_top_k = indexer_score->ne[0] < n_indexer_top_k ? indexer_score->ne[0] : n_indexer_top_k;

Edit: but update your branch to the current master first, otherwise it won't work correctly.

@pcuenca

pcuenca commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

I don't think it's a good idea to use GLM 5.2 indexer usage pattern as default

Ah, I introduced the default to support the existing unsloth GLM 5.2 checkpoints without conversion, must have forgotten about GLM 5 and 5.1 when I did it 🤦 I'll revisit this process. Sorry!

Thanks a lot for testing and pointing out these issues @fairydreaming, I'll sync with master and adapt.

@fairydreaming

Copy link
Copy Markdown
Collaborator

I don't think it's a good idea to use GLM 5.2 indexer usage pattern as default

Ah, I introduced the default to support the existing unsloth GLM 5.2 checkpoints without conversion, must have forgotten about GLM 5 and 5.1 when I did it 🤦 I'll revisit this process. Sorry!

@pcuenca I see, indeed they miss this info. Maybe you could simply check context length, I think it's in hparams.n_ctx_train - if it's 202752 then it's GLM 5 or 5.1, if it's 1048576 then it's GLM 5.2 and provide two default arrays?

@Dorijan10

Copy link
Copy Markdown

Confirmed on the unsloth GLM-5.2 UD-IQ2_M GGUF: context_length = 1048576 and no attention.indexer.types key, so it does hit the default, e.g. the 1048576 -> GLM-5.2 checks out.

I am also set up on GLM-5.2 & 8×RTX 5090 to run the KLD vs reference you mentioned you have not done yet (dense-master ref staged over wikitext-2 at -c 8192). Ping me once you push the synced branch. @pcuenca

pcuenca and others added 2 commits July 17, 2026 17:05
@pcuenca

pcuenca commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

I synced with master and added the fused_lid path, thanks again @fairydreaming!

I'm still undecided about the 5.1 vs 5.2-without-metadata discrimination; looking at context_length (or glm-dsa.rope.freq_base) seems fragile to me. Would it be better to use the metadata general.version field? I'm not familiar enough with how similar situations have been resolved in the past.

Another solution would be for me to open PRs to the unsloth 5.2 repos to update the metadata of converted models, once this PR is directionally approved so we exactly know what fields to update. This would potentially incur in re-downloads from existing users (they would be cheap with xet, but not without).

@fairydreaming

Copy link
Copy Markdown
Collaborator

I'm still undecided about the 5.1 vs 5.2-without-metadata discrimination; looking at context_length (or glm-dsa.rope.freq_base) seems fragile to me. Would it be better to use the metadata general.version field? I'm not familiar enough with how similar situations have been resolved in the past.

@pcuenca Not really, from my understanding the metadata fields have to be explicitly set by passing a metadata file during conversion (convert_hf_to_gguf.py --metadata) and usually most of them are missing in locally converted GGUF files. So it's the metadata fields that are the fragile source of information (frequently not present). I don't remember ever seeing them used in model implementations.

One example of how it has been done in the past is here:

// lite variants include DeepSeek-V2-Lite, GigaChat3-10B-A1.8B, Kanana-2-30B-A3B
const bool is_lite = (hparams.n_layer() == 27 || hparams.n_layer() == 26 || (hparams.n_layer() == 48 && n_vocab == 128256));

As you can see field values originating from config.json are used here to discriminate between model variants. So we have a precedent in case of a trial. ;-)

Another solution would be for me to open PRs to the unsloth 5.2 repos to update the metadata of converted models, once this PR is directionally approved so we exactly know what fields to update. This would potentially incur in re-downloads from existing users (they would be cheap with xet, but not without).

I think they keep all metadata separate from the weights in a very small file, so it wouldn't be a large redownload.

But generally if we can avoid reconversion/redownload by using some default settings I'd say let's do it. All future GGUF files that need specific indexer type patterns will have glm-dsa.attention.indexer.types field present after this PR is merged, so it shouldn't be a problem.

Co-authored-by: fairydreaming <166155368+fairydreaming@users.noreply.github.com>
@pcuenca

pcuenca commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

But generally if we can avoid reconversion/redownload by using some default settings I'd say let's do it. All future GGUF files that need specific indexer type patterns will have glm-dsa.attention.indexer.types field present after this PR is merged, so it shouldn't be a problem.

Done in b6f2945, thanks again!

@fairydreaming

fairydreaming commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

I extracted sparse attention changes from my old DeepSeek V3.2 implementation and created PR #25917. I think it should work without any changes with this GLM PR, will be testing it later today.

Would be nice if someone could benchmark the current dense attention implementation vs sparse one (with applied PR #25917) for large context sizes, so for example:

./bin/llama-batched-bench -m /mnt/md0/models/GLM-5.2-Q8_0.gguf -b 2048 -ub 2048 -npl 1 -npp 2048,4096,8192,16384,32768,65536,131072,262144,524288,1048064 -ntg 128 -fa 1

Edit: on my machine (Epyc 9374F + RTX PRO 6000 Max-Q, -cmoe -ctk q8_0 -ctv q8_0) I have:

this PR:

llama_batched_bench: n_kv_max = 1048576, n_batch = 2048, n_ubatch = 2048, flash_attn = 1, is_pp_shared = 0, is_tg_separate = 0, n_gpu_layers = -1, n_threads = 32, n_threads_batch = 32

|    PP |     TG |    B |   N_KV |   T_PP s | S_PP t/s |   T_TG s | S_TG t/s |      T s |    S t/s |
|-------|--------|------|--------|----------|----------|----------|----------|----------|----------|
|  2048 |    128 |    1 |   2176 |   34.144 |    59.98 |   12.699 |    10.08 |   46.842 |    46.45 |
|  4096 |    128 |    1 |   4224 |   56.870 |    72.02 |   12.704 |    10.08 |   69.574 |    60.71 |
|  8192 |    128 |    1 |   8320 |  114.826 |    71.34 |   12.908 |     9.92 |  127.734 |    65.14 |
| 16384 |    128 |    1 |  16512 |  233.358 |    70.21 |   13.174 |     9.72 |  246.532 |    66.98 |
| 32768 |    128 |    1 |  32896 |  485.915 |    67.44 |   13.573 |     9.43 |  499.489 |    65.86 |
| 65536 |    128 |    1 |  65664 | 1046.899 |    62.60 |   14.391 |     8.89 | 1061.290 |    61.87 |
|131072 |    128 |    1 | 131200 | 2422.271 |    54.11 |   16.481 |     7.77 | 2438.752 |    53.80 |

this PR + #25917

|    PP |     TG |    B |   N_KV |   T_PP s | S_PP t/s |   T_TG s | S_TG t/s |      T s |    S t/s |
|-------|--------|------|--------|----------|----------|----------|----------|----------|----------|
|  2048 |    128 |    1 |   2176 |   28.545 |    71.75 |   12.706 |    10.07 |   41.251 |    52.75 |
|  4096 |    128 |    1 |   4224 |   56.563 |    72.41 |   12.804 |    10.00 |   69.367 |    60.89 |
|  8192 |    128 |    1 |   8320 |  115.395 |    70.99 |   12.921 |     9.91 |  128.315 |    64.84 |
| 16384 |    128 |    1 |  16512 |  234.931 |    69.74 |   13.013 |     9.84 |  247.943 |    66.60 |
| 32768 |    128 |    1 |  32896 |  477.294 |    68.65 |   13.158 |     9.73 |  490.451 |    67.07 |
| 65536 |    128 |    1 |  65664 |  972.275 |    67.40 |   13.436 |     9.53 |  985.712 |    66.62 |
|131072 |    128 |    1 | 131200 | 1973.457 |    66.42 |   14.340 |     8.93 | 1987.797 |    66.00 |

so it is faster (or rather it doesn't get slow so quickly as the prompt length increases), but with expert offloading the speedup is less pronounced.

@Dorijan10

Copy link
Copy Markdown

Tested the synced branch (b6f2945) on 8x RTX 5090 (sm_120), GLM-5.2 unsloth
UD-IQ2_M, all layers on GPU, -fa 1 -ctk q8_0 -ctv q8_0. This includes the KLD
vs reference comparison mentioned in the PR description as not yet done @pcuenca.

The fused indexer path is active. Compute buffers at -c 8192:

                  dense master       this PR
CUDA0                  304 MiB       425 MiB
CUDA1-7            316-364 MiB   388-468 MiB
CUDA_Host               56 MiB        88 MiB
graph nodes               6063          7134

So roughly +100 MiB/card for the whole indexer. The old six-op chain would need
~512 MiB just for the materialized score tensor at these dims
(8192 x 512 x 32 x 4 bytes), so the F16 mask fix is clearly doing its job here.

Greedy generation is byte identical to dense master: 512 tokens from a short
prompt, so the sequence stays well under the 2048 top_k threshold where the
indexer is still a no-op.

KLD against a dense master reference (wikitext-2 test, same weights and flags):

                    c=4096 (13 ch)   c=8192 (35 ch)
PPL (this PR)               2.9375           3.2970
PPL (dense base)            2.9353           3.2875
PPL(Q)/PPL(base)          1.000758         1.002908
RMS delta p                 4.61 %           5.34 %
Same top p                 95.21 %          94.17 %

Cor(ln PPL) is 99.14% at c=8192. The divergence from dense grows with context,
which is what you would expect as more positions go past top_k=2048 and drop a
larger fraction of the keys, while PPL stays almost flat. Same model base vs
base on this setup measures ~0.001% RMS, so the deviation is real, but it looks
like the DSA approximation doing its thing rather than wrong key selection: a
mis-wired indexer would move PPL a lot more than 0.29%, and the tokens that flip
top-1 are evidently low margin ones. Could do more KLD comparisons at even larger context lengths if needed.

Modeling looks correct to me on this model and hardware.

I also stacked #25917 on top of this branch for perf testing and hit a separate issue at longer contexts, which I might follow up on separately @fairydreaming. It does not affect this PR.

@fairydreaming fairydreaming left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my (somewhat limited) testing this is good to merge.

@ngxson Since you created the initial GLM-DSA implementation can you take a look at this PR?

Comment thread src/models/glm-dsa.cpp
ml.get_key(LLM_KV_ATTENTION_INDEXER_HEAD_COUNT, hparams.indexer_n_head);
ml.get_key(LLM_KV_ATTENTION_INDEXER_KEY_LENGTH, hparams.indexer_head_size);
ml.get_key(LLM_KV_ATTENTION_INDEXER_TOP_K, hparams.indexer_top_k);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indentation change doesn't seem to serve any purpose.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation change

haha yes, sorry, I added a line that was later removed, will fix!

Comment thread src/models/glm-dsa.cpp Outdated
@ngxson

ngxson commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

@ngxson Since you created the initial GLM-DSA implementation can you take a look at this PR?

I didn't actually added the GLM-DSA support, but rather I added a placeholder in the code. That being said, my knowledge about llama.cpp's indexer impl is very limited, so I'm running code-review skill #26042 just for an extra review.

Items number 1 and 2 are worth addressing before merging (roadblock), but the rest is optional:

Automated code review

Code Review: PR #25407 — GLM 5.2 Indexer support

Overview

This PR completes the GLM 5.2 modeling implementation by making the lightning indexer (DeepSeek sparse attention) functional. Previously the indexer tensors were loaded but ignored (GLM_DSA reused deepseek2::graph, i.e. plain MLA attention). The PR:

  • Adds a new per-layer GGUF KV {arch}.attention.indexer.types (a bool array: full=1 / shared=0) plus writer/conversion plumbing.
  • Adds hparams.is_indexer_full(il) and the is_indexer_full_impl array.
  • Gives GLM_DSA its own graph (src/models/glm-dsa.cpp), copied from deepseek32.cpp with two deltas: (1) indexer RoPE is interleaved (LLAMA_ROPE_TYPE_NORM) rather than NeoX, and (2) shared indexer layers reuse the previous full layer's top_k.
  • Routes GLM_DSA to the DSA KV cache (llama_kv_cache_dsa) and enables Hadamard rotation tensors.
  • Fixes the layer-count case 79 → 78 and an assert message.

I verified the supporting infrastructure against master: get_arr correctly handles a BOOL GGUF array → uint32_t (int8→transform path, same as is_recr_impl), the indexer RoPE type differs from deepseek32 as claimed, GLM_DSA's main-attention rope is already NORM, and GLM_DSA is not duplicated in the create_memory switch (previously handled by default).


Findings

1. (Medium) is_indexer_full_impl is never default-initialized, but the model-saver serializes it for every architecture → reads uninitialized memory (UB).

  • In llama-hparams.h:231 the new member has no initializer:
    std::array<uint32_t, LLAMA_MAX_LAYERS> is_indexer_full_impl;
  • It is only ever populated in glm-dsa.cpp::load_arch_hparams. For any other arch, it stays uninitialized.
  • llama-model-saver.cpp:283 adds it unconditionally with per_layer=true, which reads value[0..n_layer()-1]. llama_model_saver_supports_arch() is an allow-by-default blocklist, so this path runs for LLAMA, Qwen, DeepSeek, etc. → UB read and a garbage indexer.types KV written into re-saved GGUFs.
  • Contrast: is_recr_impl is safe only because it's globally std::fill-ed in llama-model.cpp for all archs; the new array has no equivalent.
  • Fix: std::array<uint32_t, LLAMA_MAX_LAYERS> is_indexer_full_impl = {}; (or gate the add_kv on the arch actually using an indexer).

2. (Low, verify) Load will throw if the converted indexer.types length ≠ n_layer().

  • get_key_or_arr(LLM_KV_ATTENTION_INDEXER_TYPES, …, hparams.n_layer(), false) throws "wrong array length" when the stored array length doesn't equal n_layer() (78, excluding the MTP/nextn layer). Conversion writes len(config["indexer_types"]). Worth confirming the HF config's indexer_types has exactly 78 entries (i.e. excludes the NextN layer); otherwise freshly converted GGUFs will fail to load. The in-code GLM_5_2_DEFAULT_INDEXER_TYPES has 78 non-default entries, which is consistent — but this coupling is implicit and load-time-fatal.

3. (Low) prev_top_k can be nullptr for a shared layer preceding any full layer.

  • In the shared branch (glm-dsa.cpp) top_k = prev_top_k; and is passed to build_attn. This is safe for real GLM 5.2 (layers 0–1 are full) and for both BC defaults, but a malformed/custom indexer.types with a leading shared layer silently passes a null tensor. A GGML_ASSERT(top_k != nullptr && "shared indexer layer before any full layer"); would fail loudly instead of downstream. Defensive nit.

4. (Low) is_pre_5_2 heuristic via n_ctx_train < 1048576 is a fragile magic number.

  • Backward-compat detection keyed on context length works for today's GLM 5/5.1 vs 5.2, but would misclassify a 5.1 checkpoint with extended context. It only matters for old checkpoints lacking the new KV (new conversions write it explicitly), so impact is limited — but consider a named constant + comment, or keying on something more robust.

5. (Info) Layer-count 79 → 78 is a real fix, display-only.

  • n_layer() = n_layer_all - n_layer_nextn = 78 for the 744B model, so the old case 79 never matched and type was always UNKNOWN. Correct fix; affects only the reported model-type label. The assert-message correction (n_layer_all) is also good.

6. (Info, author-acknowledged) Decode slowdown.

  • Expected direction: the old GLM_DSA path ran dense MLA (indexer ignored), so it did less work. The new path computes the indexer on every full layer even for sequences <2048 tokens where it's effectively a no-op. Not a regression against a correct baseline, but the "much slower" the author reports is worth profiling before merge — likely the non-fused (!cparams.fused_lid) branch with its several ggml_cont/permute ops, or the extra ggml_concats. Not blocking.

Style / conventions

  • The graph is a near-verbatim copy of deepseek32.cpp. This duplicates a lot of MLA code, but it matches the established project convention (deepseek2/32/4 each have their own full graph file), so it's acceptable. If the maintainers ever factor the shared MLA body out, this should follow.
  • GLM_5_2_DEFAULT_INDEXER_TYPES is clearly commented with a source link — good. Consider a static_assert/comment that the meaningful prefix length equals the expected n_layer() to catch drift.
  • Naming/comments are consistent with the codebase; cb(...) labels mirror deepseek32.

Correctness spot-checks that passed

  • Indexer RoPE LLAMA_ROPE_TYPE_NORM correctly implements the interleaved variant and differs from deepseek32's NeoX, matching the referenced transformers change.
  • q_pe/q_nope split strides and concat order match deepseek32.
  • BOOL→uint32 load path (get_arr) is correct; converter writes BOOL, saver writes UINT32, loader handles both.
  • GLM_DSA correctly added to the DSA KV-cache and Hadamard-rotation gates.

Recommendation

Solid, well-scoped PR. The one thing I'd fix before merge is finding #1 (trivial = {} initializer — genuine UB affecting unrelated architectures' save path). #2 is worth a quick confirmation, and #3 is a cheap defensive assert. The performance note (#6) can be a follow-up.

Comment thread src/llama-hparams.h

// Indexer is "full" (1) or "shared" (0)
// Shared indexers reuse top-k from previous full layer
std::array<uint32_t, LLAMA_MAX_LAYERS> is_indexer_full_impl;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llama-model.cpp run std::fill for all arrays like this

@fairydreaming

Copy link
Copy Markdown
Collaborator

I checked perplexity of GLM-5.1 and GLM-5.2 in mainline and this PR with wiki.test.raw and large 8k chunk size (so that indexer does some work):

  • GLM-5.1 mainline: PPL = 2.5079 +/- 0.01268
  • GLM-5.1 this PR: PPL = 2.5038 +/- 0.01261
  • GLM-5.2 mainline: PPL = 2.4710 +/- 0.01198
  • GLM-5.2 this PR: PPL = 2.4728 +/- 0.01201

Additionally I added a small test change to allow override of indexer_top_k value and observed increased perplexity with smaller top_k values (the more indexer_top_k value is reduced the higher the perplexity is). I think we can be fairly certain that the indexer part works correctly.

Regarding the automated code review, I confirmed that in the converted model indexer_types array has correct length:

     43: [BOOL]     |       78 | glm-dsa.attention.indexer.types = [True, True, True, False, False, False, ...]

So that covers point 2 from the automated review, I agree with point 1, this array shall be zero-initialized. Number 3 is easy to add. @pcuenca IMHO you can ignore the rest.

@pcuenca

pcuenca commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review (nice Skill!) 🙌

  1. Addressed via a std::fill like in previous cases.
  2. Verified while debugging and independently by @fairydreaming.
  3. Added an assertion; didn't do it before because the shared indexer models we know about were verified to be correct in this regard.
  4. Yeah, already discussed in previous comments. We could use a named constant instead, not sure it's worth it.

@pcuenca
pcuenca requested a review from ngxson July 24, 2026 17:52

@ngxson ngxson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks!! let's merge when the CI passes

@ngxson
ngxson merged commit 88bfee1 into ggml-org:master Jul 24, 2026
23 of 27 checks passed
@pcuenca
pcuenca deleted the glm-dsa-indexer branch July 24, 2026 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conversion model Model specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants