ggml : fix uninitialized is_on_grid in quantize_row_iq3_xxs_impl

[CISC]

See https://github.com/ggml-org/llama.cpp/actions/runs/17615331967/job/50046823487#step:5:87

[slaren]

If the goal is only to silence a compiler warning, then I would just zero-initialize the variable where it is declared. If you suspect that this is actually a bug that is leading to wrong results, then I think it needs more explanation.

[CISC]

It's fixing an actual bug, depending on the condition. It will potentially use uninitialized (or initialized from previous loop) data depending on this being done or not:

llama.cpp/ggml/src/ggml-quants.c

Lines 3750 to 3754 in f50c60d

	if (sumq2 > 0 && sumqx*sumqx > best*sumq2) {
	scale = sumqx/sumq2; best = scale*sumqx;
	for (int i = 0; i < 32; ++i) L[i] = Laux[i];
	for (int k = 0; k < 8; ++k) is_on_grid[k] = is_on_grid_aux[k];
	}

See the other quants for reference:

llama.cpp/ggml/src/ggml-quants.c

Line 3283 in f50c60d

is_on_grid[0] = is_on_grid[1] = true;

llama.cpp/ggml/src/ggml-quants.c

Line 3931 in f50c60d

for (int k = 0; k < bs4; ++k) is_on_grid[k] = false;

llama.cpp/ggml/src/ggml-quants.c

Line 4883 in f50c60d

is_on_grid[0] = is_on_grid[1] = true;

[slaren]

To me, it is not obvious that this will change the results, or if it does, that it needs to be initialized to false instead of true (or other values).

[CISC]

It is indeed not obvious, it should perhaps be set to true, or maybe even do as quantize_row_iq3_s_impl where it is set to false:

llama.cpp/ggml/src/ggml-quants.c

Line 3968 in f50c60d

//if (is_on_grid[k]) continue;

[CISC]

Either way it is potentially using uninitialized data right now. The safest choice AFAICT is initializing it to falsetrue.

[slaren]

Again, what is it fixing? Is the change meaningful, or is it just adding more noise?

[CISC]

It's fixing uninitialized data, nothing more or less, exactly like in all the other quants.

[CISC]

..and yes, this will only happen when the weights are zero, but this has happened enough times that we have added several checks against it:

llama.cpp/ggml/src/ggml-quants.c

Line 493 in f50c60d

float scale = suml2 ? sumlx/suml2 : 0.0f;

llama.cpp/ggml/src/ggml-quants.c

Line 569 in f50c60d

return suml2 > 0.0f ? sumlx / suml2 : 0.0f;

llama.cpp/ggml/src/ggml-quants.c

Line 969 in f50c60d

return suml2 > 0.0f ? sumlx / suml2 : 0.0f;

[slaren]

It seems that if all weights are zero, scale will also be zero, and the branch that uses is_on_grid will be ignored.

[CISC]

It seems that if all weights are zero, scale will also be zero, and the branch that uses is_on_grid will be ignored.

Not necessarily, only if the whole block is zero, however that may not be the case, also scale is based off the original weights, while the weights in question are the ones after imatrix is applied (ie, the imatrix may cause non-zero parts to go to zero).

[compilade]

L with all zeros is on grid for IQ3_XXS, so initializing is_on_grid to true makes sense.

(EDIT: but L doesn't seem to be initialized either...)

[CISC]

(EDIT: but L doesn't seem to be initialized either...)

If the original weights are all zero (or close) L will get cleared, however not if the imatrixed weights are zero. :(

[taronaeo]

Hi! Any update on this? It's one of the items preventing #15925 from passing CI tests due to LLAMA_FATAL_WARNINGS=ON

[CISC]

Hi! Any update on this? It's one of the items preventing #15925 from passing CI tests due to LLAMA_FATAL_WARNINGS=ON

I will double check L for the other quants first later today.

[CISC]

Since imatrix weights are unlikely to be just partially zero it means the whole block will be on grid in the event of weights being zeroed by imatrix, and then L will not be used.