Reconnect the producer edge when DivMulFusion substitutes the Mul's input - #32703
Open
Aditya Malkar (adimalkar) wants to merge 1 commit into
Open
Aditya Malkar (adimalkar) wants to merge 1 commit into
Aditya Malkar (adimalkar) wants to merge 1 commit into
Conversation
…nput DivMulFusion rewrites `1 / x1 * x2` into `x2 / x1` by moving the Mul's other input onto the Div with graph_utils::ReplaceNodeInput. That helper rewrites the input definition and creates no node edge, which is correct for substituting an initializer or graph input - what the other seven ReplaceNodeInput call sites under core/optimizer/ do. This one can substitute a node-produced value, and the edge carrying it ends at the Mul that FinalizeNodeFusion then deletes, leaving the Div consuming the value by name alone with no incoming edge. That stays invisible until another rule removes the producer. graph_utils:: RemoveNode rewires a node's edge-connected consumers, so it misses the Div, and the producer disappears while the Div still references its output. Both are Level1 rewrite rules running in the same RuleBasedGraphTransformer pass, so the graph is not re-resolved in between, and the pass-ending Graph::Resolve() fails with "Node input 'mid' is not a graph input, initializer, or output of a previous node". The model loads at ORT_DISABLE_ALL and fails from ORT_ENABLE_BASIC onward. Capture the Mul's input edge before the substitution and recreate it on the Div. The Div's input 0 is a constant initializer (required by SatisfyCondition), so it carries no edge and is free as the destination index. When the Mul's other input is an initializer or graph input there is no edge to recreate and behaviour is unchanged. Add a regression test per producer - a same-dtype Cast removed by CastElimination, and an inference-mode Dropout removed by EliminateDropout - with the Mul's operands commuted in the second so the substituted input is covered at both index 0 and index 1. Both fail on unpatched main with the error above. Fixes microsoft#32416 Fixes microsoft#32414
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Author
|
@microsoft-github-policy-service agree |
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
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.
Description
DivMulFusionrewrites1 / x1 * x2intox2 / x1by moving theMul's other input onto theDivwithgraph_utils::ReplaceNodeInput. That helper rewrites the input definition and deliberately creates no node edge — its contract covers substituting an initializer or graph input, which is what the other sevenReplaceNodeInputcall sites undercore/optimizer/do.div_mul_fusion.ccis the one that can substitute a node-produced value, and the edge carrying that value ends at theMulthatFinalizeNodeFusionthen deletes. TheDivis left consuming the value by name alone, with no incoming edge.That is invisible until another rewrite rule removes the producer.
graph_utils::RemoveNoderewires a node's edge-connected consumers, so it does not see theDiv, and the value's producer disappears while theDivstill references it. Both rules are Level1 rewrite rules that run in the sameRuleBasedGraphTransformerpass, so the graph is never re-resolved in between; the pass-endingGraph::Resolve()then fails with:The model loads at
ORT_DISABLE_ALLand fails fromORT_ENABLE_BASIConward.The fix captures the
Mul's input edge before the substitution and recreates it on theDiv. TheDiv's input 0 is required bySatisfyConditionto be a constant initializer, so it carries no edge and is free as the destination index. When theMul's other input is an initializer or graph input there is no edge to recreate and behaviour is unchanged.Motivation and Context
Fixes #32416 — a same-dtype
Castremoved byCastElimination.Fixes #32414 — an inference-mode
Dropoutremoved byEliminateDropout.Both reports are the same defect reached through different producers, and both reproduce on the 1.30.0 CPU wheel. Each issue's own ablation points at the pair (
DivMulFusionplus the elimination rule); disabling either one makes the model load.Verified against a local Release build (Linux x64, CPU EP): both reproducers fail before the change and load and run correctly after it, with the fusion still applied.
Two regression tests are added in
graph_transform_test.cc, one per producer, with theMul's operands commuted in the second so the substituted input is exercised at both index 0 and index 1 (#32414 notes the commuted form reproduces as well). Both fail on unpatchedmainwith the exact error above and pass with the change. The existingGraphTransformationTestssuite passes (406 passed, 17 skipped — WebGPU, not built here, 0 failed), as do the*Fusion*,*Optimizer*and*RuleBased*filters.I used an AI assistant while investigating and preparing this change; the diagnosis, the fix and the test results above were verified by me against a local source build.