-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix ClipQuantFusion crash when Clip has multiple input edges #26923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
From AISummaryThis PR fixes a runtime exception in the Key Changes
Review AnalysisCorrectness
Performance
ConclusionThis is a straightforward and correct bug fix. The inclusion of a regression test gives high confidence in the solution. LGTM. |
|
/azp run Linux QNN CI Pipeline,Win_TRT_Minimal_CUDA_Test_CI,Windows ARM64 QNN CI Pipeline,Windows GPU Doc Gen CI Pipeline |
|
Azure Pipelines successfully started running 4 pipeline(s). |
adrianlizarraga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
|
hm, there appears to be an invisible unresolved comment that's blocking the merge |
|
can't figure out how to merge this one. replacing with #27016. |
## Motivation `ClipQuantFusion in clip_quantizelinear.cc` calls `graph_utils::RemoveNode()` without first checking `graph_utils::CanRemoveNode()`. When a Clip node has min/max inputs from DequantizeLinear nodes (instead of initializers), it has multiple input edges. `RemoveNode()` throws exception: ``` [ONNXRuntimeError] : 6 : RUNTIME_EXCEPTION : Exception during initialization: graph_utils.cc:650 bool onnxruntime::graph_utils::RemoveNode(onnxruntime::Graph&, onnxruntime::Node&) Should be unreachable if CanRemoveNodeAndMergeEdges is in sync with the logic here. ``` ## Fix: Added `CanRemoveNode()` check to `ClipQuantFusion::SatisfyCondition()` to skip nodes that cannot be safely removed. ## Test: Added `ClipQuantFusion_MultipleInputEdges` test that creates a Clip node with min from a DQ node (2 input edges) and verifies the optimizer doesn't crash. ## Note: This PR is a duplicate of #26923 which GitHub is preventing us from merging. Credit goes to original author @qti-yuduo. --------- Co-authored-by: Yuduo Wu <yuduow@qti.qualcomm.com>
…ft#27016) ## Motivation `ClipQuantFusion in clip_quantizelinear.cc` calls `graph_utils::RemoveNode()` without first checking `graph_utils::CanRemoveNode()`. When a Clip node has min/max inputs from DequantizeLinear nodes (instead of initializers), it has multiple input edges. `RemoveNode()` throws exception: ``` [ONNXRuntimeError] : 6 : RUNTIME_EXCEPTION : Exception during initialization: graph_utils.cc:650 bool onnxruntime::graph_utils::RemoveNode(onnxruntime::Graph&, onnxruntime::Node&) Should be unreachable if CanRemoveNodeAndMergeEdges is in sync with the logic here. ``` ## Fix: Added `CanRemoveNode()` check to `ClipQuantFusion::SatisfyCondition()` to skip nodes that cannot be safely removed. ## Test: Added `ClipQuantFusion_MultipleInputEdges` test that creates a Clip node with min from a DQ node (2 input edges) and verifies the optimizer doesn't crash. ## Note: This PR is a duplicate of microsoft#26923 which GitHub is preventing us from merging. Credit goes to original author @qti-yuduo. --------- Co-authored-by: Yuduo Wu <yuduow@qti.qualcomm.com>
## Motivation `ClipQuantFusion in clip_quantizelinear.cc` calls `graph_utils::RemoveNode()` without first checking `graph_utils::CanRemoveNode()`. When a Clip node has min/max inputs from DequantizeLinear nodes (instead of initializers), it has multiple input edges. `RemoveNode()` throws exception: ``` [ONNXRuntimeError] : 6 : RUNTIME_EXCEPTION : Exception during initialization: graph_utils.cc:650 bool onnxruntime::graph_utils::RemoveNode(onnxruntime::Graph&, onnxruntime::Node&) Should be unreachable if CanRemoveNodeAndMergeEdges is in sync with the logic here. ``` ## Fix: Added `CanRemoveNode()` check to `ClipQuantFusion::SatisfyCondition()` to skip nodes that cannot be safely removed. ## Test: Added `ClipQuantFusion_MultipleInputEdges` test that creates a Clip node with min from a DQ node (2 input edges) and verifies the optimizer doesn't crash. ## Note: This PR is a duplicate of #26923 which GitHub is preventing us from merging. Credit goes to original author @qti-yuduo. --------- Co-authored-by: Yuduo Wu <yuduow@qti.qualcomm.com> (cherry picked from commit cc2b01b)
Motivation
ClipQuantFusion in clip_quantizelinear.cccallsgraph_utils::RemoveNode()without first checkinggraph_utils::CanRemoveNode(). When a Clip node has min/max inputs from DequantizeLinear nodes (instead of initializers), it has multiple input edges.RemoveNode()throws exception:Fix:
Added
CanRemoveNode()check toClipQuantFusion::SatisfyCondition()to skip nodes that cannot be safely removed.Test:
Added
ClipQuantFusion_MultipleInputEdgestest that creates a Clip node with min from a DQ node (2 input edges) and verifies the optimizer doesn't crash.