[Fix][Relax][Frontend][Torch] Keep zero-sized dims when reshaping - #20255
[Fix][Relax][Frontend][Torch] Keep zero-sized dims when reshaping#20255hiyufan wants to merge 5 commits into
Conversation
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
_torch_reshape_dims() only rewrites the first literal zero. PyTorch treats every zero in the target as a real zero, so an empty (0, 3) tensor can validly reshape(0, 0) to (0, 0). This helper produces [-1, 0]; Relax then interprets the remaining 0 as “copy dim 1”, yielding (0, 3). Could we preserve all zero positions, or otherwise handle multiple-zero targets, and add a regression for this case?
|
You are right, and it is worse than a missed case — both of these were silently wrong rather than an error: Working through your example sharpened the rule for me. It is not that only the first zero is rewritten — it is that which zero needs rewriting depends on the input. A literal That also explains why Pushed
Targets needing at most one rewrite — every case I have seen in practice — still emit a single reshape. Longest chain over everything I tested is 3. Verified against numpy over 2132 valid reshapes (7 input shapes including Regression added as (0, 3).reshape(0, 0) (3, 0).reshape(0, 0)
(0, 3, 5).reshape(0, 0, 0) (2, 0, 4).reshape(0, 0, 4)It fails against the previous head of this PR and passes now, so the specific hole you found is pinned. Full suites: 24 failed / 416 passed against 24 failed / 412 passed on clean This is the third distinct symptom of the same root cause, after the raise and the |
|
The For example, Please allow the rewrite when the input contains a known zero even if its other dimensions are symbolic, and add a dynamic-batch regression test for this case. |
|
Confirmed and fixed in The guard was The symbolic dimensions do still matter one level down, where a position that needs holding is filled in. A literal there would be re-read as a copy, so the position is held as the expression it already is, which is not a literal and so is not substituted; a later step rewrites it once it has become a real zero. I also removed a no-op the loop was emitting: after a rewrite it appended the target once more, so your case lowered to two reshapes. Once nothing needs rewriting the previous step has already produced the target, so the extra one is only emitted when no rewrite happened at all. Your case is a single reshape now: lv: R.Tensor((0, 4), dtype="float32") = R.reshape(x, R.shape([0, 4]))VerificationI swept it rather than checking your example alone, since hand-picked cases are what let the previous hole through — a case that passed by coincidence read as confirmation. 7 input layouts mixing symbolic and static dims around a known zero, against 9 targets, compared to torch: The 2132-case static sweep is unchanged at zero mismatches, so the symbolic path did not cost the static one anything. Test
Full suites: 24 failed / 417 passed against 24 failed / 412 passed on clean |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The reshape handling now preserves multiple literal-zero target dimensions by chaining rewrites, and the symbolic-input case no longer bails merely because another dimension is unknown when a statically known zero already fixes the element count. The multiple-zero and dynamic-batch regressions cover both issues raised in review.
tlopex
left a comment
There was a problem hiding this comment.
None in target in _torch_reshape_chain still skips the fix when the target contains a symbolic dimension. For input (batch, 2, 0, 4), x.flatten(1, 2) imports as (batch, 2, 4) instead of (batch, 0, 4). Could you drop that check, preserve the symbolic dimensions, and add a regression where the target shape also contains batch?
d9b542d to
2ac8744
Compare
|
Confirmed and fixed in The third one I found while checking your case and it is worse: both dimensions come back as The changeDropping the check is the whole fix, exactly as you said: - if 0 not in target or -1 in target or None in target:
+ if 0 not in target or -1 in target:The rewrite already leaves a non-literal alone at any position it does not touch, so the symbol is preserved without further work. The reasoning I got wrong is worth naming: I wrote that guard to mean "the target is not fully known, so stay out of the way", but relax only reads a literal as "copy the input dimension". A symbol is not a literal, so it was never at risk — and standing aside left the literal zero next to it being read as a copy, which is the one thing the rewrite exists to prevent. Regression
What I checked this timeThree rounds of this helper have each been "the case I happened to try passes", so I built the sweep around targets that can reach a symbol rather than around examples. 938 comparable cases — The 46 left are not this change:
I should also say that my first version of that sweep reported 938/938 matched, which was wrong twice over: Also
Rebased onto |
|
Retracting the caveat in my last comment: my build tree is current now (it needed the Nothing new turned up, so there is nothing to correct. Jenkins so far: |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Rechecked the current revision. The reshape helper now handles multiple zero-sized dimensions and symbolic targets, with numerical regressions covering the added cases. Current checks are green. Looks good from my side.
tlopex
left a comment
There was a problem hiding this comment.
RemoveRedundantReshape breaks the new reshape chain: for input (0, 3, 5), x.reshape(0, 0, 0) returns (0, 3, 5) after the pass. Combining the calls reinterprets the zeros as input-dimension copies and removes the reshape entirely. Could you have a change for that?
PyTorch reads a literal `0` in a target shape as a real zero-sized
dimension. `relax.op.reshape` reads it as "copy the corresponding input
dimension", which is ONNX `Reshape` with `allowzero=0`. The torch
frontend forwards torch's shape unchanged, so any target shape holding a
literal `0` is silently reinterpreted:
x = torch.randn(2, 0, 4)
x.reshape(0, 4) # torch (0, 4) -> relax raises
x.view(0, 4) # torch (0, 4) -> relax raises
torch.flatten(x) # torch (0,) -> relax raises
x.unflatten(0, ...) # rank grows -> IndexError from the zero-dim path
`torch.flatten` on a `(0, 3)` input happens to work, because copying
input dim 0 gives the same 0 the literal asked for. That coincidence is
what hides the rest.
When the input is statically empty, the dimension torch asks for can be
written as `-1`, whose inference yields 0. Add `_torch_reshape_dims` and
use it where a torch-supplied target shape reaches `relax.op.reshape`:
`_reshape`, `_reshape_as`, `_flatten_impl`, `_unflatten`, `_as_strided`.
Sites that derive the target from the input's own shape are unaffected,
since "copy input dim" and the literal agree there.
The rewrite is deliberately narrow. For a non-empty input torch rejects a
zero in the target outright, and rewriting it to `-1` would silently
produce a shape instead of surfacing that error, so those shapes are left
alone.
The three tests run the imported module rather than comparing against an
expected TVMScript module: an `IRModule` holding `R.reshape(x, R.shape([0,
4]))` cannot be written as TVMScript, because re-parsing it applies the
copy rule again and infers a different shape. That round-trip gap belongs
to `relax.op.reshape` itself and is not addressed here.
Co-authored-by: Claude <noreply@anthropic.com>
…dims
Review catch: rewriting only the first literal zero is not enough. A literal
zero survives relax's copy rule exactly at a position whose input dimension is
itself zero, so which zero needs rewriting depends on the input, and there can
be more than one.
(0, 3).reshape(0, 0) torch (0, 0) was (0, 3)
(0, 3, 5).reshape(0, 0, 0) torch (0, 0, 0) was (0, 0... 3, 5)
Both were silently wrong rather than an error.
Rewrite the helper to pick the positions that cannot survive rather than the
first zero, and to split the rewrite when several of them need `-1`: each step
turns one such position into a real zero, which lets the next step spell it as
a literal. Targets needing at most one rewrite -- every case seen in practice --
still emit a single reshape.
Checked against numpy over 2132 valid reshapes (7 input shapes, ranks 1-3):
no mismatches, longest chain 3 steps.
Co-authored-by: Claude <noreply@anthropic.com>
…dims are known
Review catch: the guard declined the rewrite whenever any input dimension was
symbolic, so a statically known zero next to a dynamic batch went unhandled.
(batch, 0, 4).reshape(0, 4) torch (0, 4) was (s77, 4)
Silently wrong again, and the wrong shape is not even empty. One known zero fixes
the element count at zero whatever the symbols turn out to be, so require only
that -- symbolic dimensions elsewhere are held as they stand, since a non-literal
is not read as a copy, and rewritten in a later step once they have become real
zeros.
Also drop the trailing no-op reshape the loop emitted after a rewrite: once no
position needs rewriting, the previous step has already produced the target.
The dynamic case above now lowers to a single reshape rather than two.
Checked against torch over 63 combinations of symbolic and static dims carrying a
known zero (7 input layouts, 9 targets): 63 matched, against 27 before this
change. The 2132-case static sweep is unchanged at no mismatches.
Co-authored-by: Claude <noreply@anthropic.com>
…mbolic
The guard also bailed out when the target held a symbolic dimension. That is the
same mistake as the input-side one fixed in the previous commit, on the other half
of the argument: only a literal is read as "copy the input dimension", so a symbol
in the target is not one. It is carried through untouched, and the literal zero
beside it still has to be rewritten.
input (batch, 2, 0, 4) torch before
x.flatten(1, 2) (batch, 0, 4) (batch, 2, 4)
x.reshape(x.shape[0], 0, 4) (batch, 0, 4) (batch, 2, 4)
x.reshape(0, x.shape[0]) (0, batch) (batch, batch)
Dropping the check is the whole fix: the rewrite already leaves a non-literal alone
at a position it does not touch, so preserving the symbol needs nothing else.
Tests: test_reshape_zero_sized_dim_symbolic_target covers a symbol ahead of the
zero, a target built from x.shape[0], and a zero ahead of the symbol. All three
fail against the previous head.
Swept 938 comparable cases whose target can reach a symbolic dimension -- reshape
and view over every target drawn from {0, 2, 4, x.shape[k]} at lengths 2 and 3,
every valid flatten(start, end), and unflatten mixing zeros, -1 and symbols -- each
against the shape torch.export itself recorded:
before 578 matched, 360 mismatched
after 892 matched, 46 mismatched
no case fails after this change that did not fail before it
Of the 46 left, 40 are relax raising ValueError("Cannot use and / or / not operator
to Expr") for a target mixing a symbol with a literal, which reproduces identically
without this change, and 6 are the sweep printing s0*2 where torch prints 2*s0.
Co-authored-by: Claude <noreply@anthropic.com>
`relax.op.reshape` resolves its target when the call is built: a literal 0 becomes the input dimension at that position. `RemoveRedundantReshape` rewrites reshape(reshape(x, s1), s2) to reshape(x, s2) without accounting for that, so moving a target that holds a literal zero onto a different input silently asks for something else. For x: (0, 3, 5), reshape(reshape(x, [0, 0, 5]), [0, 0, 0]) (0, 0, 0) reshape(x, [0, 0, 0]) (0, 3, 5) all three copied back and the no-op rule then drops the surviving call, so the pass returns x unchanged. This is not specific to empty tensors -- any second target holding a literal zero over a differing intermediate dimension is affected -- but the reshape chain added in this branch makes it reachable from a plain torch import. Validate the rewrite instead of reasoning about which zeros are safe: re-resolve the target against the new input and keep the fusion only when it comes back unchanged. Tests: the zero-sized chain has to survive the pass, and an ordinary chain with no zeros still has to collapse to one reshape, so the guard cannot quietly disable the pass. The first fails without this change, the second passes either way. Swept the same 938 frontend cases with RemoveRedundantReshape applied after import: pass as it stands 868 matched, 70 mismatched pass fixed 892 matched, 46 mismatched no case fails with the fixed pass that did not fail without it 892/46 is exactly the result without the pass, so it no longer changes any outcome. The 24 cases it was breaking include the one reported in review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2ac8744 to
bebfc56
Compare
|
Confirmed and fixed in Why it happens
The no-op rule then sees a reshape to the input's own shape and drops it, which is how both calls disappear. This is not specific to the chain in this PR — any second target holding a literal zero over a differing intermediate dimension hits it — but the chain is what makes it reachable from a plain torch import, so it belongs here. The changeRather than reason about which zeros are safe to move, validate the rewrite: re-resolve the target against the new input and keep the fusion only when it comes back unchanged. def _can_reparent(arg: Expr, output_shape: Expr) -> bool:
try:
reparented = relax.op.reshape(arg, output_shape)
except Exception:
return False # reshape cannot resolve a 0 or -1 without a known input shape
return tvm_ffi.structural_equal(reparented.args[1], output_shape)It was 24 cases, not oneYour report was one case, so I re-ran the 938-case sweep from the last round with 892/46 is exactly the no-pass result, so the pass no longer changes any outcome — and the 46 left are the same pre-existing ones from the last round (40 Tests
Removed the file-level On the rebased treeRebased onto One thing worth passing on, since it cost me a confusing hour: after that rebase the numeric tests all failed with |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Re-reviewed current bebfc56 after the RemoveRedundantReshape regression fix. The pass now re-resolves the target against the proposed new input and only reparents when the resolved shape is structurally unchanged, which preserves literal-zero semantics while still collapsing ordinary reshape chains. The added zero-sized-chain and ordinary-chain regressions cover both sides. No blocker from me on this head.
Problem
PyTorch reads a literal
0in a target shape as a real zero-sized dimension.relax.op.reshapereads it as "copy the corresponding input dimension" — ONNXReshapewithallowzero=0. The torch frontend forwards torch's shape unchanged, so any target shape holding a literal0is silently reinterpreted.On
mainthese import as:mainx.reshape(0, 4)(2, 0, 4)(0, 4)ValueError: Reshape expects the new shape to be convertible…x.view(0, 4)(2, 0, 4)(0, 4)ValueErrorx.reshape(0)(2, 0, 4)(0,)ValueErrorx.reshape(3, 0)(0, 3)(3, 0)ValueErrortorch.flatten(x)(2, 0, 4)(0,)ValueErrortorch.flatten(x)(2, 3, 0)(0,)ValueErrorx.unflatten(0, (2, -1))(2, 0)(2, 1, 0)IndexError: Index 2 out of bounds 2torch.flatten(x)(0, 3)(0,)(0,)— happens to workThe last row is why this is easy to miss: copying input dim 0 there gives back the same
0the literal asked for, so the one case people usually try looks fine.The
IndexErrorcomes from the same rule.ConvertNewShapeToExprresolves a zero witharray_ref.Set(i, shape_ty->values.value()[i]), indexing the input shape at the new shape's position, so a target of higher rank than the input reads past the end.Zero-sized tensors are not exotic in exported models — a detector with no proposals, an empty batch, an empty mask — and they reach
reshape/view/flattenon ordinary code paths.Fix
When the input is statically empty, the dimension torch asks for can be written as
-1instead, whose inference yields0._torch_reshape_dimsdoes that rewrite, applied where a torch-supplied target shape reachesrelax.op.reshape:_reshape,_reshape_as,_flatten_impl,_unflatten,_as_strided.The other
relax.op.reshapecall sites in the frontend derive their target from the input's own shape, where "copy input dim" and the literal agree, so they are left alone.The rewrite is deliberately narrow. It only fires when the input is statically empty. For a non-empty input torch rejects a zero in the target outright, and rewriting it to
-1there would turn an error into a silently wrong shape:Verification
All 17 shape cases I exercised now agree with PyTorch (6 previously raised). Zero-sized behaviour of
squeeze,permute,expand,catandsumwas already correct and is unchanged.Built with LLVM and ran the imported module:
x.reshape(0, 4)on(2, 0, 4)builds, runs, and returns shape(0, 4).tests/python/relax/test_frontend_from_fx.py+tests/python/relax/test_frontend_from_exported_program.py:main: 24 failed, 412 passed, 3 skippedThe 24 failures are pre-existing on
mainin my environment (test_dtypesand friends), identical before and after. The three additional passes are the new tests, which fail onmainand pass with the fix.ruff format --checkandruff checkare clean.One thing I want to flag
The three tests run the imported module instead of comparing against an expected TVMScript module, because the resulting
IRModulecannot be written as TVMScript. The frontend emitswhich executes correctly, but re-parsing it applies the copy rule again and infers
(2, 4), so the annotation no longer matches and the module is rejected as not well-formed. That round-trip gap lives inrelax.op.reshape, not in the frontend, and this PR does not try to close it.So this fixes the observable behaviour but leaves the underlying ambiguity in place. The more complete fix is probably an
allowzero-style option onrelax.op.reshape(the ONNX frontend already carriesallowzeroand works around the same rule by routing through a dynamic shape expression), with the torch frontend opting in — that would also make the emitted IR round-trip. That is a change to a core op's interface, so I did not want to make that call unilaterally. If you would prefer that shape, I am happy to implement it instead and close this.Also worth noting:
_flatten_implis touched here and also by #20245. The hunks are independent and should merge cleanly; happy to rebase either way.This change was prepared with AI assistance (Claude). I have reviewed and verified it, and can speak to it in review.