fix(optimizer): prevent Shape/Gather from folding to constants when shape has negative (unknown) dims - #2964
fix(optimizer): prevent Shape/Gather from folding to constants when shape has negative (unknown) dims#2964Justin Chu (justinchuby) with Copilot wants to merge 4 commits into
Conversation
…re negative (-1) When a tensor shape has `-1` as an integer (representing an unknown/dynamic dimension), the `Shape` and `Gather` handlers were incorrectly folding to constants containing `-1`. This caused downstream `Reshape` ops to receive multiple `-1` values (e.g., `[-1, -1]`), which is invalid in ONNX (at most one `-1` is allowed in a Reshape shape). The fix adds a non-negativity check: only fold `Shape`/`Gather` to a constant when all dimension values are non-negative integers (i.e., all are known static positive values). Fixes #2963
There was a problem hiding this comment.
Pull request overview
This PR hardens the optimizer’s FoldConstantsPass against ONNX-IR shapes that encode unknown dimensions as the integer -1, preventing invalid constant-folding that can propagate multiple -1 values into Reshape shapes.
Changes:
- Add a non-negativity guard to
Shapeconstant folding so-1(unknown) dims do not become literal constants. - Add the same non-negativity guard to the
Gather-from-symbolic-shape folding path to avoid folding negative gathered dims. - Add a regression test covering the
Shape→Gather→Concat→Reshapepattern when the input shape contains integer-1dims.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| onnxscript/optimizer/_constant_folding.py | Prevents folding Shape/Gather results to constants when any folded dimension is negative (unknown). |
| onnxscript/optimizer/_constant_folding_test.py | Adds regression coverage for the -1-dim folding bug scenario involving Shape/Gather/Reshape. |
| if all(isinstance(d, int) and d >= 0 for d in shape_slice): | ||
| return op.Constant(value_ints=ir.AttrInt64s("value_ints", list(shape_slice))) |
There was a problem hiding this comment.
Added the non-negativity guard to Size folding so it only folds when all dims are known non-negative ints (commit ba4d21e).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2964 +/- ##
==========================================
+ Coverage 72.61% 72.63% +0.01%
==========================================
Files 263 265 +2
Lines 32034 32219 +185
Branches 3013 3042 +29
==========================================
+ Hits 23263 23402 +139
- Misses 7748 7782 +34
- Partials 1023 1035 +12 ☔ View full report in Codecov by Harness. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
FoldConstantsPassincorrectly foldsShape→Gather→Concat→Reshapepatterns when the input tensor has unknown dimensions stored as integer-1in the IR (as opposed toir.SymbolicDim). Since-1is anint, the all-integer check passes, causingShapeto fold intoConstant(value_ints=[-1, 512, 1, ...]).Gatherthen returns a constant-1, which propagates into theReshapeshape — producing an invalid shape like[-1, -1](ONNX allows at most one-1in a Reshape shape).Changes
Shapehandler: add non-negativity guard — only fold to a constant when all shape dims are known positive integers:Gatherhandler: same guard on the symbolic-shape gather path, preventing folding when any gathered dimension value is negative.test_shape_gather_concat_reshape_with_neg1_dim— verifies thatShapeis preserved andReshapenever receives multiple-1values when the input shape contains-1integer dims.The root cause: ONNX IR deserializes unknown dims (
dim_value=-1in the proto) as integer-1, notir.SymbolicDim. The existingSymbolicDimpath was safe; the-1-as-int path was not.