Skip to content

fix(gbq): key GatherBlockQuantized default zero point off ONNX storage type, not signedness - #609

Draft
BoarQing wants to merge 1 commit into
mainfrom
fix/gbq-storage-type
Draft

fix(gbq): key GatherBlockQuantized default zero point off ONNX storage type, not signedness#609
BoarQing wants to merge 1 commit into
mainfrom
fix/gbq-storage-type

Conversation

@BoarQing

Copy link
Copy Markdown
Contributor

Summary

GatherBlockQuantized's default zero point (used when zero_points is absent) depends on the ONNX storage type T1, which the EP currently cannot observe. tensor(uint4) and tensor(uint8)-holding-packed-nibbles both arrive at the runtime as ui8 with bits=4 and a byte-identical layout, yet the ORT CPU kernel uses 0 for the former and 2^(bits-1) for the latter:

// onnxruntime/contrib_ops/cpu/quantization/gather_block_quantized.cc
if constexpr (std::is_same_v<T1, uint8_t>) {
  ...
  const int32_t default_zero_point = 1 << (static_cast<int>(bits_) - 1);
} else {  // Int4x2 / UInt4x2
  zp_val = zero_points_ptr ? ... : 0;
}

Deciding from is_signed_data, as main does today, therefore biases one of the two by 8 * scale whichever way the condition is written. In practice uint4 embedding tables published without zero_points come back shifted by -8 * scale on every gathered row.

The correct rule is already written down in three places in this repo — HipOps.td ("the default is 0 for int4/uint4 and 2^(bits-1) for uint8", from #368), hipdnn_ep_runtime.h, and the ORT schema doc string — but is not what the code implements.

Approach

Carry the ONNX element type explicitly rather than trying to re-derive it:

  1. Import (mlir-graph.cpp) records onnx.element_type on every onnx.Constant. This attribute was already listed in the GBQ legalizer's preserve-set but nothing ever set it.
  2. convert-onnx-to-hip pins quant_storage_bits from that marker, before legalizeInt4ConstantIfNeeded rewrites the constant and erases the shape evidence that would otherwise distinguish the two cases.
  3. convert-hip-to-llvm forwards it to the runtime (22nd parameter).
  4. Runtime selects default_zp = (storage_bits == 8) ? 1 << (bits - 1) : 0.

Signedness no longer participates in the zero-point decision at all. It is still needed for nibble sign extension, which is a separate concern — the previous code used one boolean for both jobs and got the second one wrong.

quant_storage_bits is declared in ODS with a verifier rather than attached as a discardable attribute, so producer and consumer share a generated accessor and illegal widths are rejected at verification. It is optional and falls back to bits, keeping hand-written IR valid.

Also routes the prepare-pattern attribute edits through rewriter.modifyOpInPlace instead of mutating the op behind the driver's back.

Verification

Built main and this branch separately and ran both against the same models with the same ORT CPU reference (hip-onnx-runner -L, EP vs -n CPU):

Model Storage / bits zero_points main L2 this PR L2
uint4_nozp uint4 / 4 no 166.277 0
uint4_nozp_100352x2048 uint4 / 4, external no 940.604 0
int4_nozp int4 / 4 no 0 0
uint8_nozp uint8 / 8 no 0 0
u8bits4_nozp uint8 / 4 packed no 0 0
uint4_zp uint4 / 4 yes 0 0
uint4_zp_blk16 uint4 / 4, block 16 yes 0 0
int4_zp int4 / 4 yes 0 0
uint8_zp uint8 / 8 yes 0 0
uint4_zp_100352x2048 uint4 / 4, external yes 0 0

The failure signature on main is a pure additive shift, not noise or a layout error:

ORT CPU (truth) : [0.   0.5  1.   1.5  2.   2.5  3.   3.5]
main            : [-4.  -3.5 -3.  -2.5 -2.  -1.5 -1.  -0.5]
diff            : [-4.  -4.  -4.  -4.  -4.  -4.  -4.  -4.]   == -8 * scale

u8bits4_nozp is the case that separates the two candidate rules: it is byte-identical to uint4_nozp but requires the opposite default. It passes on both builds, confirming the fix keys off storage type and not off bits.

361/361 lit tests pass. ORCA-2bit runs end to end; it is unaffected either way because it ships zero_points, so the default is never consulted.

Affected models

Only when all three hold: storage is tensor(uint4), bits == 4, and zero_points is absent. Anything shipping zero_points is unaffected.

Not addressed here

Deliberately out of scope, flagged for follow-up:

  • bits == 2 is legal for tensor(uint8) per the schema, but convert-onnx-to-hip still gates on bits in {4, 8}, so such models fall back rather than converting.
  • quantize_axis is still re-derived from shape invariants when the attribute is absent, rather than using the ONNX default of 1.
  • legalizeInt4ConstantIfNeeded and recreateExternalConstant still return false / nullptr silently instead of emitting diagnostics.
  • The CI L2 suite still contains no GBQ model. This is the gap that let a wrong default zero point ship. The 10 models above are currently local only; wiring them (or at least uint4_nozp and u8bits4_nozp) into the gate is the change that prevents a recurrence.

Test plan

  • CI L2 suite green
  • lit suite green
  • Reviewer sanity-check the storage-width rule against the ORT CPU kernel
  • Decide whether the GBQ models should be added to the CI L2 gate in this PR or a follow-up

@github-actions

Copy link
Copy Markdown

Thanks for opening a PR!

This project follows LLVM's incremental-development and AI-tool-use
guidance. See CONTRIBUTING.md
for the project workflow.

Before requesting review, please check that:

  1. The change is focused. Substantial work links the relevant issue
    or design discussion.
  2. The PR documents relevant test results and updates affected
    documentation.
  3. If AI tools provided substantial assistance, the description
    explains what was assisted and how it was validated, and commit
    trailers identify the tool. The contributor has reviewed and
    understands the result.

Reviewers are assigned through
CODEOWNERS where ownership
is configured.

@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown

L2 Accuracy Results (EP vs CPU)

Model Combined L2 Total Elems Skipped NaN/Inf
conv_test_hybrid 4.8668E-07 64 0
GroupQueryAttention_seq256 25.2366 2621440 0
MatMulNBits_o_seq128 259.906 368640 0
QMoE_seq128 34.957 368640 0

Threshold: 0.01 | Run: 3486 - Commit: 143c44e

@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown

MorphiZen EP Performance Results

Model QPS Session (s) 1st Infer (ms) CPU% Mem (MB)
full_model_seq128 7.51 6.10 365 3 1244
GroupQueryAttention_seq128 4384.48 1.80928 11 6 311
matmul_down_seq128 521.26 2.39 75 3 351

EPContext Export Performance

Model QPS Session (s) 1st Infer (ms) CPU% Mem (MB)
full_model_seq128 7.52 45.87 362 3 15590

EPContext Import Performance

Model QPS Session (s) 1st Infer (ms) CPU% Mem (MB)
full_model_seq128 7.50 9.89 359 3 15760

OGA Benchmark Results

Model Warmup Reps Prompt Len Gen Tokens TTFT (ms) TPS Peak Mem (GB) GPU Mem (GB)
gpt-oss-20b-webgpu-int4-rtn-block-32 1 5 128 128 173.1 78.9 1.33 13.54
Llama-3.1-8B-awq-g128-int4-asym-fp16-onnx-dml 1 5 128 128 375.7 35.3 1.22 6.43

OGA Wheel Smoke (Python benchmark_e2e.py)

Model TTFT (ms) TPS
Llama-3.1-8B-awq-g128-int4-asym-fp16-onnx-dml 195 35.5

Run: 3486 - Commit: 143c44e

@BoarQing
BoarQing force-pushed the fix/gbq-storage-type branch from 676d2ef to 4b259d8 Compare July 31, 2026 09:29
GatherBlockQuantized picks its implicit zero point from the T1 storage type,
not from whether that type is unsigned. The ORT CPU kernel branches on the
storage type alone: uint8 uses 2^(bits-1) and both int4 and uint4 use 0, so
uint4 defaults to 0 despite being unsigned. Selecting on signedness biases
every uint4 embedding without explicit zero points by +(8 * scale).

uint4 and uint8-holding-packed-nibbles are byte-identical in MLIR -- both
arrive as ui8 tensors with the same `bits` -- so the distinction has to be
carried explicitly. The importer records the ONNX element type on each
constant, the conversion turns that into a `quant_storage_bits` attribute
declared in ODS with a verifier, and the lowering forwards it to the runtime
as a new trailing argument.

Measured against ORT CPU on the single-op variants, uint4 without zero points
goes from L2 166.277 to 0 (and 940.604 to 0 on the 100352x2048 version).
uint8 with bits=4 stays at 0, confirming the rule keys off storage width
rather than `bits`: that case still needs a zero point of 8.

Tests cover each layer the width travels through: the element-type mark
selecting 8 for uint8 and 4 for uint4/int4 on otherwise identical IR, the
lowering placing that value in the trailing runtime argument, and the
verifier rejecting widths that are illegal or narrower than `bits`.
@BoarQing
BoarQing force-pushed the fix/gbq-storage-type branch from 4b259d8 to 143c44e Compare July 31, 2026 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant