Fix ReshapeReshape initializer name collisions when resolving shared -1 shapes#2947
Fix ReshapeReshape initializer name collisions when resolving shared -1 shapes#2947Copilot wants to merge 2 commits into
-1 shapes#2947Conversation
-1 shapes
|
@copilot was there a way from the rewrite rule class to get a unique name? No worries if there is not. Just wanted to double check in case there is |
I double-checked: not from |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2947 +/- ##
==========================================
+ Coverage 72.64% 72.66% +0.02%
==========================================
Files 259 259
Lines 31766 31793 +27
Branches 3007 3008 +1
==========================================
+ Hits 23075 23102 +27
Misses 7672 7672
Partials 1019 1019 ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Pull request overview
This PR addresses a correctness issue in the ReshapeReshape rewrite rule where synthesizing new concrete shape initializers could reuse an existing shape initializer name across multiple matches, leading to initializer-map clobbering and invalid value references after subsequent passes.
Changes:
- Update
ReshapeReshapeto name synthesized shape initializers using a per-match derived name instead of reusing the incoming shape value’s name. - Add a regression test covering two
Reshape -> Reshapechains sharing a singleshared_shape=[2, -1]initializer where-1resolves differently per chain. - Exercise the optimizer sub-pipeline used in the repro and assert rewritten Reshape shape inputs are distinct and backed by registered initializers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| onnxscript/rewriter/rules/common/_basic_rules.py | Adjusts synthesized shape initializer naming for ReshapeReshape to avoid collisions across multiple matches. |
| onnxscript/rewriter/rules/common/_basic_rules_test.py | Adds regression coverage for shared -1 shape initializer rewrites producing distinct concrete shape initializers. |
|
|
||
| # Constraints for shape. | ||
| self._allowzero = context.nodes[0].attributes.get_int("allowzero", 0) | ||
| self._new_shape_name = f"{context.nodes[1].name or shape.name}/shape" |
ReshapeReshapecould rewrite multiple matches that share the same shape initializer ([2, -1]) into distinct concrete shapes, but reuse the same initializer name. This caused initializer map clobbering and left rewrittenReshapeinputs pointing to non-registered values after name-fixup.Rewrite rule change (
_basic_rules.py)graph.initializerswhen multiple chains share one original shape input.Regression coverage (
_basic_rules_test.py)Reshape -> Reshapechains sharingshared_shape=[2, -1], where-1resolves differently per chain.