You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
wrap_qmoe hard-codes zp_elem_size = 1 (uint8 packed nibbles) for both FC1 and FC2 calls into hip_matmul_nbits. This silently mis-unpacks zero_points if a future QMoE export emits fp16 zero_points (which wrap_matmul_nbits already supports via zp_elem_size = 2, added in #162).
Today every QMoE model on disk that we run uses uint8 packed-nibble zero_points (the gpt-oss-120b-AWQ family is the motivating one), so this works in practice. But:
The MS QMoE spec permits fp16 zero_points (same dtype as scales) in addition to packed uint8.
The plumbing for QMoE is not symmetric: there is no place in wrap_qmoe that inspects the ONNX *_zero_points tensor's element type, so even if a model exported fp16 ZPs they would be reinterpreted as packed bytes and produce silent garbage outputs (no assertion, no diagnostic).
The recently-fixed packed-nibble per-expert stride in qmoe.cpp (PR #168, commit 0918fdf) also assumes zp_elem_size == 1 (the stride is e * N * ((k_blocks + 1) / 2)); a generic fix would need to compute the stride as e * N * k_blocks * zp_elem_size_in_bytes with the correct unpacking for the byte-element case.
Suggested fix
wrap_qmoe already takes void *fc1_zero_points / void *fc2_zero_points as raw pointers, so the dtype information is lost by the time control reaches the runtime. The fix lives one level up:
Plumb the zero_points dtype through the wrapper signature. Either add an int64_t zp_elem_size parameter to wrap_qmoe (mirroring wrap_matmul_nbits), or split into fc1_zp_elem_size / fc2_zp_elem_size for completeness.
At lowering time (lib/Conversion/HipToLLVM/QMoEToLLVM.cpp or wherever wrap_qmoe is called), inspect the element type of the fc{1,2}_zero_points operand: ui8 → 1, f16 → 2. Pass that constant down.
In wrap_qmoe, replace the hard-coded 1 at lines 369 and 399 with the per-FC parameter.
Update the per-expert stride computation to use e * N * ((k_blocks + 1) / 2) only when zp_elem_size == 1, and e * N * k_blocks * 2 when zp_elem_size == 2 (no packing for fp16).
This issue is not a regression introduced by #168 - it is pre-existing behaviour surfaced during the final review. The packed-nibble stride fix in #168 is correct for every model we currently care about. The fix above is appropriate as a follow-up so that QMoE achieves full parity with MatMulNBits re: optional ZP dtype.
Summary
wrap_qmoehard-codeszp_elem_size = 1(uint8 packed nibbles) for both FC1 and FC2 calls intohip_matmul_nbits. This silently mis-unpacks zero_points if a future QMoE export emits fp16 zero_points (whichwrap_matmul_nbitsalready supports viazp_elem_size = 2, added in #162).Where
lib/Runtime/real/qmoe.cpp:369— FC1 call:/*zp_elem_size=*/1lib/Runtime/real/qmoe.cpp:399— FC2 call:/*zp_elem_size=*/1Why it matters
Today every QMoE model on disk that we run uses uint8 packed-nibble zero_points (the gpt-oss-120b-AWQ family is the motivating one), so this works in practice. But:
wrap_matmul_nbitsalready differentiates these viazp_elem_size(1= byte/packed-nibble,2= fp16) — see fix: support uint8 packed nibble zero_points in MatMulNBits WMMA/GEMV paths #162.wrap_qmoethat inspects the ONNX*_zero_pointstensor's element type, so even if a model exported fp16 ZPs they would be reinterpreted as packed bytes and produce silent garbage outputs (no assertion, no diagnostic).The recently-fixed packed-nibble per-expert stride in
qmoe.cpp(PR #168, commit0918fdf) also assumeszp_elem_size == 1(the stride ise * N * ((k_blocks + 1) / 2)); a generic fix would need to compute the stride ase * N * k_blocks * zp_elem_size_in_byteswith the correct unpacking for the byte-element case.Suggested fix
wrap_qmoealready takesvoid *fc1_zero_points/void *fc2_zero_pointsas raw pointers, so the dtype information is lost by the time control reaches the runtime. The fix lives one level up:int64_t zp_elem_sizeparameter towrap_qmoe(mirroringwrap_matmul_nbits), or split intofc1_zp_elem_size/fc2_zp_elem_sizefor completeness.lib/Conversion/HipToLLVM/QMoEToLLVM.cppor whereverwrap_qmoeis called), inspect the element type of thefc{1,2}_zero_pointsoperand:ui8→1,f16→2. Pass that constant down.wrap_qmoe, replace the hard-coded1at lines 369 and 399 with the per-FC parameter.e * N * ((k_blocks + 1) / 2)only whenzp_elem_size == 1, ande * N * k_blocks * 2whenzp_elem_size == 2(no packing for fp16).test/lit/Conversion/onnx-to-hip/test_qmoe_packed_zp.mlir(added in fix(gemm,qmoe,gqa,matmul): unblock gpt-oss-120b end-to-end (4 fixes) #168) with anf16ZP variant and CHECK that the lowering picks upzp_elem_size = 2.Out of scope for #168
This issue is not a regression introduced by #168 - it is pre-existing behaviour surfaced during the final review. The packed-nibble stride fix in #168 is correct for every model we currently care about. The fix above is appropriate as a follow-up so that QMoE achieves full parity with
MatMulNBitsre: optional ZP dtype.