hiyufan commented on PR #20255:
URL: https://github.com/apache/tvm/pull/20255#issuecomment-5520335994
You are right, and it is worse than a missed case — both of these were
silently wrong rather than an error:
```
torch before
(0, 3).reshape(0, 0) (0, 0) (0, 3)
(0, 3, 5).reshape(0, 0, 0) (0, 0, 0) (0, 3, 5)
```
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 `0` survives the copy rule exactly at a position whose
input dimension is itself `0`, because copying reproduces the zero that was
asked for. So in your `(0, 3)` case position 0 is already fine and position
**1** is the one that has to change; rewriting position 0 both wasted the
single `-1` and left the real problem in place.
That also explains why `(2, 0, 4).reshape(0, 0, 4)` passed in my original
testing and gave me false confidence: there the input dimension under the
second zero happens to be `0`, so the copy rule reproduced it by luck. Same
coincidence as the `flatten` on `(0, 3)` case in the PR body.
Pushed `0e11655`:
- pick the positions that *cannot* survive, rather than the first zero;
- when several of them need `-1`, split the rewrite — each step turns one
such position into a real `0`, which lets the next step spell it as a literal.
Only one `-1` per reshape, so this is the part that needs more than one step.
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 `(0,3)`, `(3,0)`, `(0,3,5)`, `(2,0,4)`, `(0,0,4)`; ranks 1–3; dims
drawn from `{0,1,2,3,4,5,6,12,15,20,24}`), keeping only the targets numpy
itself accepts: **no mismatches**.
Regression added as `test_reshape_multiple_zero_sized_dims`, covering your
case plus the others in that family:
```python
(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 `main` — the 24 are pre-existing in my
environment and identical either way, the four extra passes are these tests.
This is the third distinct symptom of the same root cause, after the raise
and the `IndexError`, which I think strengthens the note at the bottom of the
PR description: the durable fix is an `allowzero`-style option on
`relax.op.reshape` so the frontend can state the intent directly instead of
encoding it in `-1`. That would also close the round-trip gap, which this does
not. Happy to implement that instead if you would prefer it over this
workaround — just say which shape you want and I will rework it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
