[QNN EP] Fix HardSigmoidMul fusion missing reversed Mul input ordering - #535
Open
qti-chuteng wants to merge 4 commits into
Open
qti-chuteng wants to merge 4 commits into
qti-chuteng wants to merge 4 commits into
Conversation
qti-chuteng
requested review from
qti-ashwshan,
qti-jkilpatrick,
qti-kromero,
qti-yuduo,
tirupath-qti and
yath1
as code owners
June 16, 2026 05:55
|
|
qti-chuteng
force-pushed
the
dev/chuteng/fix-hardsigmoid-mul-fusion
branch
from
June 16, 2026 07:48
6c67578 to
9ca710c
Compare
qti-yuduo
approved these changes
Jun 18, 2026
The HardSigmoidMulFusion pattern matcher checks that the HardSigmoid input is
also the other input to the Mul (so the pair forms HardSwish(x) = x * HardSigmoid(x)).
The check had a copy-paste bug: both sides of the || compared Mul.Inputs()[0],
so it only ever inspected the first Mul input:
const bool same_root_input = mul->Inputs()[0].name == hs_input ||
mul->Inputs()[0].name == hs_input; // [0] twice
As a result, when the graph used Mul(hardsigmoid_output, root_input) -- i.e. the
HardSigmoid output as Inputs()[0] and the root as Inputs()[1] -- the matcher
failed and the pattern was silently left unfused, falling back to separate ops.
Fix the second comparison to Inputs()[1] so both input orderings fuse.
Add hardsigmoid_mul_fusion_test.cc covering both orderings and asserting, via the
dumped QNN JSON graph, that the standalone Mul (ElementWiseMultiply) is gone and a
single ElementWiseNeuron (HardSwish) remains. Existing accuracy tests already use
the reversed ordering but only assert EP assignment -- which passes whether or not
fusion happens -- so the missed fusion was not caught.
- Remove the outer #if defined(_WIN32) around SKIP_HTP_TEST_ON_ARCH_...: the macro already guards platforms internally, and the extra guard prevented Linux aarch64 devices (arch <= V68) from skipping correctly. Now consistent with all sibling node_group fusion tests. - Switch the new test file to the Qualcomm copyright header per repo convention for newly added files.
qti-yuduo
force-pushed
the
dev/chuteng/fix-hardsigmoid-mul-fusion
branch
from
June 18, 2026 20:34
9ca710c to
673ef20
Compare
| @@ -0,0 +1,110 @@ | |||
| // Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. | |||
Collaborator
There was a problem hiding this comment.
Should existing testcases moved inside?
|
|
||
| // HardSigmoid -> Mul(input, hsig_out): HardSigmoid output is the SECOND Mul input. | ||
| // This is the ordering the original same_root_input check already handled. | ||
| TEST_F(QnnHTPBackendTests, HardSigmoidMulFusion_NormalOrder_Fuses) { |
Collaborator
There was a problem hiding this comment.
CI failed for all HardSigmoidMulFusion testcases in accuracy.
qti-chuteng
requested review from
qti-shubham and
yuhuchua-qti
as code owners
September 29, 2026 09:58
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
HardSigmoidMulFusionpattern matcher recognizesHardSwish(x) = x * HardSigmoid(x)and fuses theHardSigmoid+Mulpair into a single QNNElementWiseNeuron(HardSwish) op. To accept the pair, it verifies that theHardSigmoidinput is also one of the twoMulinputs. That check had a copy-paste bug — both sides of the||comparedMul.Inputs()[0]:So only the first Mul input was ever inspected. When a graph uses
Mul(hardsigmoid_output, root_input)— HardSigmoid output asInputs()[0], the root asInputs()[1]— the matcher fails to recognize the root, and the pattern is silently left unfused, degrading to separateHardSigmoid+Mulops on the backend.Fix
Change the second comparison to
Mul.Inputs()[1]so both input orderings —Mul(root, hsig_out)andMul(hsig_out, root)— fuse correctly.Test
Adds
qnn_node_group/hardsigmoid_mul_fusion_test.ccwith two cases (normal and reversed Mul input ordering). Each dumps the QNN JSON graph and asserts the fusion actually happened: the standaloneElementWiseMultiply(Mul) is gone (count = 0) and a singleElementWiseNeuron(HardSwish) remains (count = 1).Why this wasn't already caught
Existing accuracy tests (
HardSigmoidFusedIntoHardSwish_FP32_as_FP16,HardSigmoidFusedIntoHardSwish_FP16) already use the reversed orderingMul(hsig_out, input), but they only assertExpectedEPNodeAssignment::All. That passes whether or not the fusion happens — both ops are still assigned to the QNN EP either way — so the missed fusion went undetected. The new test asserts the graph topology, not just EP assignment.Scope / risk
[0]→[1]) plus a new test file.Testing
lintrunner— clean.