[GGUF FE] Fix dequant/op correctness bugs and Q2_K/Q3_K packing

mvafin · claude · mvafin · commit 3dafa1b8c77f · 2026-07-09T18:12:40.000+02:00
Correctness fixes found in code review:
- Q4_0 requant read u4 nibbles as unsigned, ignoring the 0x88 XOR sign
  encoding; token_embd/output Q4_0 weights were corrupted. Read as signed
  i4 when there is no zero-point.
- GELU/GEGLU used OpenVINO's default (ERF) Gelu; ggml uses the tanh
  approximation. Pass GeluApproximationMode::TANH.
- MUL_MAT type-mismatch Convert re-read the original input(0), discarding
  the op_case 2/3 reprocessing of B and targeting the wrong element type.
- SCALE aborted when the "bias" attribute was absent; give scale/bias
  defaults like SOFT_MAX.
- IMROPE read data_node-&gt;get_shape()[3], throwing on a dynamic dim; use the
  decoder output_shape like the NORMAL/NEOX branches.
- VIEW op_case 3 could leave slice_dim == -1 and index the shape at
  SIZE_MAX; return the input unchanged for an identity view.
- SetRows stateless lowering only guarded dim 2 against dynamic; emit -1 for
  any dynamic batch/n_seq/emb dim.
- FLASH_ATTN_EXT did not convert k/v to f16, so f32 K/V hit SDPA's
  mixed-type check; convert explicitly.
- Q4_K integer zero-point (u8) was unclamped and could wrap mod 256; clamp
  to [0,255] via a shared helper.

Q2_K / Q3_K dequant (previously DISABLED as known-failing):
- Q2_K: block layout is scales-first then qs (offsets were swapped), and qs
  is not in element order, so replace the memcpy with a per-element unpack
  into LSB-packed u2 matching ggml's dequantize_row_q2_K.
- Q3_K: rebuild the 16 signed 6-bit sub-scales with ggml's kmask1/kmask2
  interleave (-32 bias) and unpack qs/hmask in ggml's strided element order.
- dequant_extracted_to_f32 (Q8_0_C requant path) gains a u2 branch for Q2_K
  and reads no-zp 4-bit weights as signed (Q3_K centered i4 / Q4_0).
- Re-enable the Q2_K/Q3_K dequant and weight tests; both now match the ggml
  reference within f16 tolerance.

Infra / hygiene:
- Drop the duplicate ov::WeightlessCacheAttribute definition and include the
  canonical core dev_api header (matches the ONNX frontend), fixing an ODR
  hazard.
- Rename public headers frontend.h/decoder.h to .hpp so ov_add_frontend
  (which installs *.hpp only) ships them for direct linkers.
- Parallelize fill_q6_k (the only serial fill) and reuse the requant flag
  instead of recomputing needs_q8_0_c_requant.
- Correct the stale features.cmake / src CMakeLists comments (the FE is
  installed and exports entry points; it is hidden via m_hidden), and remove
  leftover commented-out debug code.

Co-Authored-By: Claude Opus 4.8 &lt;noreply@anthropic.com&gt;
diff --git a/cmake/features.cmake b/cmake/features.cmake
@@ -155,8 +155,10 @@ ov_option(ENABLE_OV_PYTORCH_FRONTEND "Enable PyTorch FrontEnd" ON)
 ov_option(ENABLE_OV_JAX_FRONTEND "Enable JAX FrontEnd" ON)
 ov_option(ENABLE_OV_TF_FRONTEND "Enable TensorFlow FrontEnd" ON)
 ov_option(ENABLE_OV_TF_LITE_FRONTEND "Enable TensorFlow Lite FrontEnd" ON)
-# GGUF frontend is an MVP development/testing component; disabled by default and never
-# installed into the release package (see SKIP_INSTALL in its CMakeLists.txt).
+# GGUF frontend is an MVP development/testing component, disabled by default. When enabled it is
+# built and installed as a linkable frontend (LINKABLE_FRONTEND), but it registers itself as
+# hidden (FrontEndPluginInfo::m_hidden) so FrontEndManager never lists or auto-selects it -- it is
+# usable only by direct linkers. See the discoverability note in frontend.cpp.
 ov_option(ENABLE_OV_GGUF_FRONTEND "Enable GGUF FrontEnd" OFF)
 
 if(WIN32 AND AARCH64 AND CMAKE_CL_64)
diff --git a/src/frontends/gguf/include/openvino/frontend/gguf/decoder.hpp b/src/frontends/gguf/include/openvino/frontend/gguf/decoder.hpp
diff --git a/src/frontends/gguf/include/openvino/frontend/gguf/frontend.hpp b/src/frontends/gguf/include/openvino/frontend/gguf/frontend.hpp
diff --git a/src/frontends/gguf/src/CMakeLists.txt b/src/frontends/gguf/src/CMakeLists.txt
@@ -7,12 +7,13 @@
 # frontends (LINKABLE_FRONTEND, no SKIP_INSTALL) so callers -- the llama.cpp ggml-openvino
 # backend and OpenVINO GenAI -- can link openvino::frontend::gguf directly.
 #
-# It is intentionally NOT discoverable via FrontEndManager: it exports no plugin-registration
-# entry points (get_api_version / GetFrontEndData), so it never appears in available_front_ends()
-# and is never auto-selected by load_by_model / load_by_framework, even though it is installed in
-# the frontend directory. See the discoverability note in frontend.cpp. This keeps the
-# not-yet-production frontend hidden from the generic model-loading API while remaining usable by
-# direct linkers.
+# It is intentionally NOT discoverable via FrontEndManager: it DOES export the plugin-registration
+# entry points (get_api_version / get_front_end_data, required so FrontEndManager can load it
+# without error while scanning the frontend directory), but it registers itself as hidden
+# (FrontEndPluginInfo::m_hidden = true), so it never appears in available_front_ends() and is never
+# auto-selected by load_by_model / load_by_framework. See the discoverability note in frontend.cpp.
+# This keeps the not-yet-production frontend hidden from the generic model-loading API while
+# remaining usable by direct linkers.
 ov_add_frontend(NAME gguf
                 LINKABLE_FRONTEND
                 FILEDESCRIPTION "FrontEnd to convert GGUF models"
diff --git a/src/frontends/gguf/src/frontend.cpp b/src/frontends/gguf/src/frontend.cpp
@@ -2,13 +2,13 @@
 // SPDX-License-Identifier: Apache-2.0
 //
 
-#include "openvino/frontend/gguf/frontend.h"
+#include "openvino/frontend/gguf/frontend.hpp"
 
 #include "input_model.h"
 #include "op_table.h"
 #include "openvino/core/so_extension.hpp"
 #include "openvino/frontend/extension/decoder_transformation.hpp"
-#include "openvino/frontend/gguf/decoder.h"
+#include "openvino/frontend/gguf/decoder.hpp"
 #include "openvino/frontend/manager.hpp"
 #include "translate_session.h"
 
diff --git a/src/frontends/gguf/src/input_model.cpp b/src/frontends/gguf/src/input_model.cpp
@@ -4,7 +4,7 @@
 
 #include "input_model.h"
 
-#include "openvino/frontend/gguf/decoder.h"
+#include "openvino/frontend/gguf/decoder.hpp"
 
 namespace ov {
 namespace frontend {
diff --git a/src/frontends/gguf/src/input_model.h b/src/frontends/gguf/src/input_model.h
@@ -6,7 +6,7 @@
 
 #include <openvino/frontend/input_model.hpp>
 
-#include "openvino/frontend/gguf/decoder.h"
+#include "openvino/frontend/gguf/decoder.hpp"
 #include "openvino/frontend/gguf/visibility.hpp"
 
 namespace ov {
diff --git a/src/frontends/gguf/src/node_context.h b/src/frontends/gguf/src/node_context.h
@@ -8,7 +8,7 @@
 #include <openvino/frontend/node_context.hpp>
 #include <string>
 
-#include "openvino/frontend/gguf/decoder.h"
+#include "openvino/frontend/gguf/decoder.hpp"
 
 namespace ov {
 namespace frontend {
diff --git a/src/frontends/gguf/src/op/flash_attn_ext.cpp b/src/frontends/gguf/src/op/flash_attn_ext.cpp
@@ -80,6 +80,16 @@ OutputVector translate_flash_attn_ext(const NodeContext& context) {
     k = tile_kv(q_shape[1], k_shape[1], q_shape[3], k);
     v = tile_kv(q_shape[1], k_shape[1], q_shape[3], v);
 
+    // q/mask/scale are f16; SDPA requires all real inputs share an element type. k/v usually come
+    // from an f16 KV-cache, but a current-token K/V that did not pass through the cache can be f32,
+    // so convert explicitly to avoid a "Mixed input types are not supported" validation failure.
+    if (k.get_element_type() != ov::element::f16) {
+        k = std::make_shared<ov::op::v0::Convert>(k, ov::element::f16);
+    }
+    if (v.get_element_type() != ov::element::f16) {
+        v = std::make_shared<ov::op::v0::Convert>(v, ov::element::f16);
+    }
+
     auto sdpa = std::make_shared<ov::op::v13::ScaledDotProductAttention>(q, k, v, mask_sliced, scale_node, false);
     res = std::make_shared<ov::op::v1::Transpose>(sdpa,
                                                   ov::op::v0::Constant::create(ov::element::i64, {4}, {0, 2, 1, 3}));
diff --git a/src/frontends/gguf/src/op/glu_geglu.cpp b/src/frontends/gguf/src/op/glu_geglu.cpp
@@ -51,7 +51,8 @@ OutputVector translate_glu_geglu(const NodeContext& context) {
         std::swap(src0, src1);
     }
 
-    auto gelu = std::make_shared<ov::op::v7::Gelu>(src0);
+    // ggml's GEGLU uses the tanh GELU approximation (ggml_gelu_f32); v7::Gelu defaults to ERF.
+    auto gelu = std::make_shared<ov::op::v7::Gelu>(src0, ov::op::GeluApproximationMode::TANH);
     auto res = std::make_shared<ov::op::v1::Multiply>(gelu, src1);
 
     return rename_outputs_with_suffix({res}, context.get_name());
diff --git a/src/frontends/gguf/src/op/mulmat.cpp b/src/frontends/gguf/src/op/mulmat.cpp
@@ -46,7 +46,10 @@ OutputVector translate_mulmat(const NodeContext & context) {
         A = process_view_input(context, 1);
     }
     if (A.get_element_type() != B.get_element_type()) {
-        B = std::make_shared<ov::op::v0::Convert>(context.get_input(0), context.get_input_type(1));
+        // Convert the already-processed B (op_case 2 unwrap / op_case 3 view slice), not the
+        // original input(0), and target A's actual node type -- re-reading context.get_input(0)
+        // here would silently discard the op_case rebinding above.
+        B = std::make_shared<ov::op::v0::Convert>(B, A.get_element_type());
     }
 
     auto B_shape = context.get_input_shape(0).to_shape();
diff --git a/src/frontends/gguf/src/op/rope.cpp b/src/frontends/gguf/src/op/rope.cpp
@@ -126,7 +126,9 @@ OutputVector translate_rope(const NodeContext& context) {
 
         res = std::make_shared<ov::op::v0::Concat>(ov::OutputVector{first_half_node, second_half_node}, -1);
     } else if (mode == TYPE_IMROPE) {
-        int64_t n_dims = data_node->get_shape()[3];
+        // Use the decoder-provided output_shape (like the NORMAL/NEOX branches) instead of the live
+        // node's get_shape(), which throws if the data input has any dynamic dimension.
+        int64_t n_dims = output_shape[3];
         auto cos_sin_shape = std::make_shared<ov::op::v0::Constant>(ov::element::i64,
                                                                     ov::Shape{4},
                                                                     std::vector<int64_t>{1, -1, 1, (n_dims >> 1)});
diff --git a/src/frontends/gguf/src/op/scale.cpp b/src/frontends/gguf/src/op/scale.cpp
@@ -19,8 +19,11 @@ namespace op {
 OutputVector translate_scale(const NodeContext& context) {
     num_inputs_check(context, 1, 1);
 
-    float scale = context.get_attribute<float>("scale");
-    float bias = context.get_attribute<float>("bias");
+    // Defaults so a decoder that omits an op-param does not abort conversion (ggml SCALE has
+    // historically carried only a scale; bias is optional). Matches how translate_soft_max reads
+    // its optional "scale"/"max_bias" attributes.
+    float scale = context.get_attribute<float>("scale", 1.0f);
+    float bias = context.get_attribute<float>("bias", 0.0f);
 
     auto scale_node = std::make_shared<ov::op::v0::Constant>(ov::element::f32, ov::Shape{}, std::vector<float>{scale});
     auto scaled = std::make_shared<ov::op::v1::Multiply>(context.get_input(0), scale_node);
diff --git a/src/frontends/gguf/src/op/softmax.cpp b/src/frontends/gguf/src/op/softmax.cpp
@@ -67,14 +67,13 @@ OutputVector translate_soft_max(const NodeContext& context) {
         mask_node_sliced = std::make_shared<ov::op::v0::Convert>(mask_node_sliced, context.get_output_type());
     }
 
-    Output<Node> slope_mask;
+    // ALiBi slope (max_bias > 0) is not yet implemented/tested; bail out rather than silently
+    // dropping it. (The previous code computed a slope-scaled mask here but then unconditionally
+    // overwrote it with the unscaled mask, so the scaling was dead in every path.)
     if (slope != 1.0f) {
-        auto slope_node =
-            std::make_shared<ov::op::v0::Constant>(ov::element::f32, ov::Shape{}, std::vector<float>{slope});
-        slope_mask = std::make_shared<ov::op::v1::Multiply>(mask_node_sliced, slope_node);
         throw std::runtime_error("Slope != 1.0f in softmax has not been tested, verify it before use.");
     }
-    slope_mask = mask_node_sliced;
+    Output<Node> slope_mask = mask_node_sliced;
 
     auto input_slope_mask_node = std::make_shared<ov::op::v1::Add>(scaled_input, slope_mask);
 
diff --git a/src/frontends/gguf/src/op/unary_gelu.cpp b/src/frontends/gguf/src/op/unary_gelu.cpp
@@ -18,7 +18,9 @@ OutputVector translate_unary_gelu(const NodeContext & context) {
     num_inputs_check(context, 1, 1);
 
     auto input = context.get_input(0);
-    auto res = std::make_shared<ov::op::v7::Gelu>(input);
+    // ggml's GGML_UNARY_OP_GELU is the tanh approximation (ggml_gelu_f32), not exact erf.
+    // v7::Gelu defaults to ERF, so request TANH explicitly to match ggml numerically.
+    auto res = std::make_shared<ov::op::v7::Gelu>(input, ov::op::GeluApproximationMode::TANH);
 
     return rename_outputs_with_suffix({res}, context.get_name());
 }
diff --git a/src/frontends/gguf/src/op/view.cpp b/src/frontends/gguf/src/op/view.cpp
@@ -39,6 +39,11 @@ OutputVector translate_view(const NodeContext & context) {
                 break;
             }
         }
+        // No differing dimension: the view is an identity, nothing to slice. Guard against
+        // indexing dst_shape with slice_dim == -1 (an out-of-bounds SIZE_MAX read).
+        if (slice_dim < 0) {
+            return {input};
+        }
 
         auto begin = ov::op::v0::Constant::create(ov::element::i64, {1}, {0});
         auto end = ov::op::v0::Constant::create(ov::element::i64, {1}, {dst_shape[slice_dim]});
diff --git a/src/frontends/gguf/src/pass/lower_set_rows_stateless.cpp b/src/frontends/gguf/src/pass/lower_set_rows_stateless.cpp
@@ -38,9 +38,12 @@ LowerSetRowsStateless::LowerSetRowsStateless() {
         // original [1, n_seq, ctx_per_seq, emb] layout (ctx_per_seq stays dynamic for llama-bench).
         if (auto dst_reshape = ov::as_type_ptr<ov::op::v1::Reshape>(dst.get_node_shared_ptr())) {
             auto dst_ps = dst_reshape->get_input_partial_shape(0);
-            std::vector<int64_t> shape = {dst_ps[0].get_length(), dst_ps[1].get_length(),
-                                          dst_ps[2].is_static() ? dst_ps[2].get_length() : -1,
-                                          dst_ps[3].get_length()};
+            // Emit -1 for any dynamic dim: Dimension::get_length() throws on a dynamic dimension,
+            // so a dynamic batch/n_seq/emb (not just ctx_per_seq) would otherwise abort lowering.
+            auto dim_or_dynamic = [&](size_t i) -> int64_t {
+                return dst_ps[i].is_static() ? dst_ps[i].get_length() : -1;
+            };
+            std::vector<int64_t> shape = {dim_or_dynamic(0), dim_or_dynamic(1), dim_or_dynamic(2), dim_or_dynamic(3)};
             res = std::make_shared<ov::op::v1::Reshape>(
                 res, ov::op::v0::Constant::create(ov::element::i64, {4}, shape), false);
         }
diff --git a/src/frontends/gguf/src/quant/gguf_quants.cpp b/src/frontends/gguf/src/quant/gguf_quants.cpp
diff --git a/src/frontends/gguf/src/quant/weights.cpp b/src/frontends/gguf/src/quant/weights.cpp
diff --git a/src/frontends/gguf/tests/op_test_utils.hpp b/src/frontends/gguf/tests/op_test_utils.hpp
diff --git a/src/frontends/gguf/tests/test_dequant_vs_ggml.cpp b/src/frontends/gguf/tests/test_dequant_vs_ggml.cpp
diff --git a/src/frontends/gguf/tests/test_weights.cpp b/src/frontends/gguf/tests/test_weights.cpp

Original file line number	Diff line number	Diff line change
`@@ -51,7 +51,8 @@ OutputVector translate_glu_geglu(const NodeContext& context) {`
`51`	`51`	`std::swap(src0, src1);`
`52`	`52`	`}`
`53`	`53`
`54`		`- auto gelu = std::make_shared<ov::op::v7::Gelu>(src0);`
	`54`	`+ // ggml's GEGLU uses the tanh GELU approximation (ggml_gelu_f32); v7::Gelu defaults to ERF.`
	`55`	`+ auto gelu = std::make_shared<ov::op::v7::Gelu>(src0, ov::op::GeluApproximationMode::TANH);`
`55`	`56`	`auto res = std::make_shared<ov::op::v1::Multiply>(gelu, src1);`
`56`	`57`
`57`	`58`	`return rename_outputs_with_suffix({res}, context.get_name());`