Mr-Neutr0n commented on PR #18774:
URL: https://github.com/apache/tvm/pull/18774#issuecomment-5125692922
Correction first: my comment yesterday said "Rebased and resolved the
conflicts." That was wrong — the push never landed, and the branch sat at
`c0061069`, 640 commits behind `main` and still conflicting. Sorry for the
noise; it's actually done now and the PR shows `MERGEABLE`.
While redoing it properly I found a real bug **in my own refactor commit**,
so this is worth more than a rebase note.
## `c0061069` was broken
That commit applied @gemini-code-assist's suggestion to reuse the `axis`
local, and I got it wrong in both `_impl_v1` and `_impl_v13`:
```python
else:
axis = attr.get("axis", 0) # only bound HERE
...
return relax.op.split(inputs[0], indices, axis) # used
unconditionally
```
`axis` is assigned only inside the `else`, so any Split node **with** an
explicit split list — the common path, and every `pass_split=True` test case —
would raise `UnboundLocalError`. It slipped through because the `cpu/pr-head`
build was already failing for unrelated reasons, so nothing ever exercised it.
Fixed by hoisting `axis = attr.get("axis", 0)` to the top of both methods.
Good argument for not applying a suggested refactor without a green run behind
it.
## The fix is now narrower
Original version returned explicit indices for *every* static `num_outputs`
split, so even splits changed from `indices_or_sections=3` to `[2, 4]` —
semantically identical, but it churned the IR (and the expectations) for cases
that already worked. It now keeps the integer form unless the split is
genuinely uneven:
```python
if dim % num_outputs:
block_size = math.ceil(dim / num_outputs)
return [block_size * i for i in range(1, num_outputs)]
return num_outputs
```
So the emitted IR is byte-identical to `main` for everything except the
uneven case this PR exists to fix.
## Test conflict
`test_frontend_onnx.py` was restructured on `main` since February —
`verify_split` now takes an `expected` IRModule from a `make_expected` helper,
driven by a `split_cases` table. I took `main`'s version wholesale and
re-expressed my two cases as table entries:
```python
# 10 / 3 -> [4, 4, 2]
(10, [[4], [4], [2]], False, 0, False, 18),
# 7 / 3 along axis 1 -> [3, 3, 1]
((4, 7), [[4, 3], [4, 3], [4, 1]], False, 1, False, 18),
```
`make_expected` needed teaching about this, since its `pass_split=False`
branch always produced `indices_or_sections=len(outdata_shapes)`, which is
exactly the wrong expectation for an uneven split. It now mirrors the frontend:
explicit indices when the axis is static and uneven, the integer otherwise.
## What I could and couldn't verify
Being upfront, as before: **I still cannot build TVM locally**, so I have
not run the suite. What I did instead was re-implement both sides of the
contract in plain Python and check they agree across every entry in
`split_cases`, both `dynamic` values:
```
shape=6 axis=0 dynamic=False frontend=3 expected=3
shape=3 axis=0 dynamic=False frontend=3 expected=3
shape=10 axis=0 dynamic=False frontend=[4, 8] expected=[4, 8]
shape=(4, 7) axis=1 dynamic=False frontend=[3, 6] expected=[3, 6]
...all agree: True
```
and that the declared output shapes match a ceil-based split (`10 ->
[4,4,2]`, `7 -> [3,3,1]`). That is a check of the arithmetic, not of TVM — CI
remains the real verdict.
## Known limitation
For a **dynamic** axis the converter still falls back to the integer form,
so an uneven split with an unknown dimension remains incorrect. That can't be
lowered to a static index list; it would need shape-dependent lowering. Out of
scope here, and unchanged from `main` — flagging it so the fix isn't read as
more complete than it is.
--
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]
