Fix XNNPACK Clip/Relu fusion when the producer output has other consumers#28814
Conversation
…mers (microsoft#28543) Reject the fusion if the Conv/MaxPool/AveragePool output is a graph output or has a non-activation consumer; the fused node only exposes the activation output. Add a regression test.
|
Hi @tianleiwu @justinchuby, could anyone help review this PR? |
There was a problem hiding this comment.
Pull request overview
This PR addresses an invalid-graph bug in XNNPACK activation fusion: Conv/MaxPool/AveragePool + Clip/Relu fusion previously ignored producer-output fanout, which could drop the pre-activation value when it also feeds a side consumer or graph output, causing session initialization to fail. The fix rejects fusion unless the producer’s output is consumed only by the activation node, and adds a regression test.
Changes:
- Add a consumer/fanout guard in
ClipReluCheckerto only allow activation fusion when the producer output has no side consumers and is not a graph output. - Add an XNNPACK regression test that builds a
MaxPool -> Relupattern with an additional side consumer of the pool output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| onnxruntime/test/providers/xnnpack/xnnpack_basic_test.cc | Adds a regression test for MaxPool + Relu fanout where fusion should be rejected to prevent invalid graphs. |
| onnxruntime/core/providers/xnnpack/detail/node_support_checker.cc | Adds a “single-consumer and not a graph output” check to prevent unsafe Clip/Relu fusion when the producer output fans out. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @justinchuby, I've addressed the Copilot review comment, could you please take a look? |
|
@tianleiwu Hi, could you take a look? Thanks. |
tianleiwu
left a comment
There was a problem hiding this comment.
Clean, correct, minimal fix.
ClipReluChecker previously fused Conv/MaxPool/AveragePool with a following Clip/Relu without checking whether the pre-activation output had other consumers. Since the fused XNNPACK node only exposes the activation output, a side consumer (or graph output) of the pre-activation value was left dangling, breaking session creation (#28543, #28544). The new HasOnlyActivationConsumer guard rejects the fusion in that case.
Notes:
- The guard mirrors the established convention already used in
core/framework/node_unit.cc(!NodeProducesGraphOutput(node) && GetOutputEdgesCount() == 1 && single edge -> target), so it correctly covers both the graph-output and the multi-consumer fan-out cases. It is placed early in thedo/while(false)cascade, so rejection short-circuits cleanly. - The third condition (
OutputEdgesBegin()->GetNode().Index() == activation.Index()) is technically redundant given thatinput0is by construction the producer of the activation's input 0 andGetOutputEdgesCount() == 1already pins the single consumer to it. Harmless and defensively clear, fine to keep. - The new
TestPoolReluFusionRejectedWithSideConsumerreproduces the exact failing topology and asserts via the graph verifier that an unfusedReluremains on the CPU EP, addressing the earlier review feedback. - No public API/ABI change; behavior change is strictly more conservative, so no regression risk for valid graphs.
LGTM.
Description
ClipReluCheckerfuses Conv/MaxPool/AveragePool with a following Clip/Relu without checking other consumers of the producer output. The fused node only exposes the activation output, so a side consumer (or graph output) of the pre-activation value is left dangling and session creation fails withInvalid model. Node input '...' is not a graph input, initializer, or output of a previous node. Reject the fusion in that case.Adds
XnnpackEP.TestPoolReluFusionRejectedWithSideConsumer.Motivation and Context
Fixes #28543, fixes #28544.