Skip to content

[QNN EP] Add DQLayerNormFusion - #756

Open
ankipand-qti wants to merge 7 commits into
mainfrom
dev/ankipand/DQLayerNormIntegerFusion
Open

ankipand-qti wants to merge 7 commits into
mainfrom
dev/ankipand/DQLayerNormIntegerFusion

Conversation

@ankipand-qti

Copy link
Copy Markdown
Collaborator

Adds DQLayerNormFusion, a new IQnnNodeGroup that fixes a signed/unsigned 8-bit dtype mismatch between a QDQ LayerNormalization node's X and its static scale/bias, avoiding a LayerNorm I/O combination QNN HTP does not natively support.

Description

Pattern matched (ONNX, starting at LayerNormalization):
x_q --> DequantizeLinear --> x_f32
scale_q(int8) --> DequantizeLinear --> scale_f32 (sign mismatched with x_q)
[bias_q] --> [DequantizeLinear] --> [bias_f32]
x_f32, scale_f32, [bias_f32] --> LayerNormalization --> y_f32 --> QuantizeLinear --> y_q

Rewrite (QNN):
x_q --------------------------------------------+
| (input[0] of LayerNorm)
v
scale_q --> [resign +/-128 zero-point shift] --> LayerNorm --> [bias_q as-is] --> y_q

@ankipand-qti

Copy link
Copy Markdown
Collaborator Author

Reviewed at commit: 9af5760

✅ Fixed Since Last Review

[M-1] onnxruntime/core/providers/qnn/builder/qnn_node_group/dq_layernorm_fusion.cc:L161
Resolved in v2 and re-verified in v3: two tests (DQLayerNormFusion_UnsignedX_MismatchedBias, DQLayerNormFusion_SignedX_MismatchedBias) exercise the previously-untested 8-bit mismatched-bias resign branch. v3 renamed the second test (from DQLayerNormFusion_Skip_SignedX_MismatchedBias) and flipped its expectation from CPU EP fallback to a successful fusion, based on real-hardware verification that SFIXED_POINT_8/SFIXED_POINT_8/SFIXED_POINT_8 runs correctly — branch reachability remains proven either way.

[N-1] onnxruntime/core/providers/qnn/builder/qnn_node_group/dq_layernorm_fusion.cc:L112
Fixed in v3: the comment was reworded to name LayerNormalizationOpBuilder::IsOpSupported (NPU backends only) as the code that enforces the last-axis restriction, instead of the inaccurate generic "default op-builder path" phrasing.

[T-1] onnxruntime/test/providers/qnn/qnn_node_group/dq_layernorm_fusion_test.cc:L149
Fixed in v3: the continuation-line indentation was corrected to 49 spaces, aligning with clang-format -style=file.


🔴 Major

[M-2] onnxruntime/test/providers/qnn/qnn_node_group/dq_layernorm_fusion_test.cc:L195-327 (5 tests)
This round changed the 5 tests that actually exercise the fusion's resign math (DQLayerNormFusion_UnsignedX_SignedScale, DQLayerNormFusion_SignedX_UnsignedScale, DQLayerNormFusion_Skip_AlreadyMatchingSign, DQLayerNormFusion_UnsignedX_MismatchedBias, DQLayerNormFusion_SignedX_MismatchedBias) to pass /*verify_outputs=*/false. The added comment's technical claim — that VerifyOutput does an exact EXPECT_EQ byte compare for INT8/UINT8 outputs, with ElementwiseAbsoluteVerifier tolerance only applying to FLOAT/FLOAT16 — is accurate (verified against test/util/test_utils.cc's VerifyOutput switch), and there genuinely is no integer-tolerant TensorVerifier variant in shared test infra today. But disabling verify_outputs entirely removes all numeric checking for exactly the tests meant to prove the fusion's core correctness claim (the lossless +/-128 zero-point resign). After this change, a bug in the resign arithmetic that still produces a validator-accepted single-LayerNorm graph would pass all 5 tests silently — only EP assignment and op presence are checked now. This differs from the codebase's existing verify_outputs=false pattern for random-input tests ([C46]), which pairs the disabled check with chaining into a downstream op because random inputs make exact comparison meaningless; here the inputs are deterministic and a correct expected value is in principle computable. Suggested fix: add an integer-tolerant TensorVerifier variant (e.g. +/-1 count for INT8/UINT8) to shared test_utils.h/.cc and use it here instead of disabling verification outright; if that's out of scope for this PR, at minimum add a couple of hand-computed spot-check assertions for the resigned scale/bias bytes and final output, and file a follow-up ticket for the shared-infra gap.


🔵 Minor

[N-2] onnxruntime/core/providers/qnn/builder/qnn_node_group/dq_layernorm_fusion.h:L25-28
Still open (file unchanged since v3). v3 changed DQLayerNormFusion_SignedX_MismatchedBias's expected outcome to a successful fusion for SFIXED_POINT_8/SFIXED_POINT_8/SFIXED_POINT_8, based on real-hardware verification. The class header's doc comment still lists only three supported INT8 combinations and doesn't include this one, so it's now stale relative to what the test suite proves. Suggested fix: add the all-SFIXED_POINT_8 combination to the header's list, noting it's absent from the validator's own printed "Supported I/O datatype sets" but verified on real hardware.

@ankipand-qti

Copy link
Copy Markdown
Collaborator Author

Reviewed at commit: f7ee7cc2

[T-3] onnxruntime/test/providers/qnn/qnn_node_group/dq_layernorm_fusion_test.cc:L207, L221-222, L230-231
Fixed (commit f7ee7cc2 "Lint fix"). The 3 lines with a 1-space indentation mismatch in RunAndVerifyLayerNormOutput (introduced alongside the M-2 fix in commit 18b7b164) were re-formatted; re-verified against clang-format -style=file — byte-identical, no remaining diff.

Note: M-2 (the Major finding that the 5 resign-math tests had lost all numeric verification) was also fixed in commit 18b7b164 "Merged main and review comments" — see the earlier M-2 comment on this thread for details. No new comment is posted for M-2/N-2 since those issue IDs were already discussed above; N-2 (stale supported-combination list in dq_layernorm_fusion.h) and T-2 (test helper's unreliable-by-default bias_kind) remain open as non-blocking follow-ups.

This branch has not been deployed

No deployments
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