[QNN EP] Add 2-bit BQ STD Symmetric Support - #787
qti-ashwshan wants to merge 4 commits into
Conversation
…encoding Use QNN_QUANTIZATION_ENCODING_BW_BLOCK_MAPPED with STANDARD_SYMMETRIC mapping for 2-bit weights with symmetric zero-points and 16-bit quantized activations. This keeps activations in int16 natively without the int16->fp16 Dequantize workaround needed by the BW_FLOAT_BLOCK path. - Add BwBlockMapped() factory and deep-copy support in QnnQuantParamsWrapper - Add IsBwBlockMapped() predicate and include in IsBlockQuantized() - Wire BW_BLOCK_MAPPED path in MatMulNBits op builder (ProcessInputs) - Skip DQ insertion and use native output dtype for BW_BLOCK_MAPPED (ProcessAttributesAndOutputs) - Add unit tests for W2A16 std symmetric with/without explicit ZP tensor
- Add unit test coverage
dbbab47 to
e8e4368
Compare
1duo
left a comment
1duo
left a comment
There was a problem hiding this comment.
A few comments inline. #1 (matmulnbits_op_builder.cc, asymmetric W2A16 path) is blocking: it breaks the build on QAIRT ≥ 2.51 after the merge from main.
Also: BwBlockMapped() and the BW_BLOCK_MAPPED deep copy in Init (copy constructor and operator=) have no host-side tests in qnn_quant_params_wrapper_test.cc. The only coverage is the HTP integration tests, which need aarch64/Linux with V69+. A small host test should check that copies own their own blockSize/scaleOffset buffers, and that bitwidth and mapping survive the copy.
| RETURN_IF_NOT(zp_tensor_proto != nullptr, "MatMulNBits zero_points must be a constant initializer."); | ||
| RETURN_IF_ERROR(qnn_model_wrapper.UnpackInitializerData(zp_tensor_proto, per_block_uint8_zp)); | ||
| std::vector<float> per_block_float_zp; | ||
| UnpackDataToDatatype<float>(per_block_uint8_zp, bits, num_zp_per_uint8, per_block_float_zp); |
There was a problem hiding this comment.
Blocking: this line won't compile after the merge from main. #856 removed UnpackDataToDatatype<T> and num_zp_per_uint8, and neither name exists in the tree anymore. CI builds with QAIRT 2.50.40 (qcom/packages.yml), which removes this block before compiling, so CI stays green anyway.
Restoring the helper wouldn't be correct either. The flat unpack ignores the per-row zero-point padding that #856 added. For bits=2, K=160, block_size=32: k_blocks=5, so each row's zero points take 2 bytes. That gives 8 unpacked values per row against 5 scales, and trips the scales.size() == offsets.size() assert in BwBlockMapped.
per_block_float_zp (from step 2.3 above) already holds these values, with padding handled and the shift and negate applied. Suggest dropping the re-unpack:
} else {
mapping = QNN_QUANTIZATION_ENCODING_MAPPING_ASYMMETRIC_PLUS_ONE;
per_block_int32_offset.assign(per_block_float_zp.begin(), per_block_float_zp.end());
}|
|
||
| // W2A16 Standard Symmetric BW_BLOCK_MAPPED (bits=2, 16-bit activation, symmetric zero-point). | ||
| // On SDK >= 2.51 uses BW_BLOCK_MAPPED encoding; on older SDKs falls back to BW_FLOAT_BLOCK. | ||
| TEST_F(QnnHTPBackendTests, MatMulNBits_BwBlockMapped_W2A16_StdSym_M1_N4_K64_BS16) { |
There was a problem hiding this comment.
These four tests pass std::nullopt for expect_native_bq, so the QNN graph JSON is never dumped or checked. They pass with any encoding, including a silent fallback to BW_FLOAT_BLOCK. On the current CI SDK (2.50) the new code path doesn't run at all. Could you check the graph as the native-BQ tests do? For example, expect only the Quantize and Dequantize nodes from the QDQ model, with no extra INT16→FP16 Dequantize.
| // (BLOCK) both produce the actual output data type (e.g. uint16/int16 for QDQ models) directly. | ||
| // Only BW_FLOAT_BLOCK forces the kernel to compute in FP16; LPBQ (BLOCKWISE_EXPANSION), native BQ | ||
| // (BLOCK), and BW_BLOCK_MAPPED all produce the actual output data type (e.g. uint16/int16) directly. | ||
| // NOTE: IsBlockQuantized() is true for both BLOCK and BW_FLOAT_BLOCK, so match the encoding directly. |
There was a problem hiding this comment.
Nit: this NOTE is out of date. IsBlockQuantized() now also covers BW_BLOCK_MAPPED. The same comment appears in conv_op_builder.cc (in ProcessAttributesAndOutputs).
| params_.quantizationEncoding == QNN_QUANTIZATION_ENCODING_BW_BLOCK_MAPPED); | ||
| } | ||
|
|
||
| bool IsBwBlockMapped() const { |
There was a problem hiding this comment.
Nothing calls IsBwBlockMapped(). Either use it (for example, in a test that checks the chosen encoding) or remove it.
|
|
||
| // W2A16 Asymmetric BW_BLOCK_MAPPED (bits=2, 16-bit activation, asymmetric zero-point). | ||
| // Uses ASYMMETRIC_PLUS_ONE mapping: {0,1,2,3} → {-1,0,1,2}. | ||
| TEST_F(QnnHTPBackendTests, MatMulNBits_BwBlockMapped_W2A16_AsymPlusOne_M1_N4_K64_BS16) { |
There was a problem hiding this comment.
Nit: this test sets has_zero_point = true but doesn't end in _ZP, unlike the other zero-point tests in this file (StdSym_..._BS32_ZP above follows the convention).
| RunHtpQDQMatMulNBitsTest<2, int16_t>(params, std::nullopt, ExpectedEPNodeAssignment::All, QDQTolerance(0.02f)); | ||
| } | ||
|
|
||
| TEST_F(QnnHTPBackendTests, MatMulNBits_BwBlockMapped_W2A16_AsymPlusOne_M1_N8_K128_BS32) { |
There was a problem hiding this comment.
Nit: this test sets has_zero_point = true but doesn't end in _ZP, unlike the other zero-point tests in this file (StdSym_..._BS32_ZP above follows the convention).
This branch has not been deployed
2 participants
Description
Motivation and Context