Repository navigation
Commit 9cd0c12
authored
Infer XNNPACK conv dimensionality from the weight rank (pytorch#21588)
### Summary
Fixes pytorch#10965.
ATen lets `stride`, `padding`, `dilation` and `output_padding` be a
single value that is broadcast over every spatial dim, so `len(stride)`
is not the convolution's dimensionality. `torch.nn.Conv2d(3, 3, 5, [2])`
keeps `stride=(2,)` because `_pair` passes an iterable through
unchanged, and that reaches the edge graph as `aten.convolution.default`
with a length-1 stride on a rank-4 weight.
The XNNPACK backend infers dimensionality from that length in three
places, so a 2d conv with a length-1 stride is mistaken for a 1d conv:
`Conv1dUnsqueezePass` skips a node unless `len(stride) == 1`, so it
picked up the 2d conv, unsqueezed the already-4d weight to 5d and then
failed inside the fake tensor convolution rule. That is the traceback in
the issue.
`ConvolutionConfig.check_constraints` rejects 3d convs with
`len(conv_stride) > 2`. A `Conv3d` built with `stride=[2]` passes that
guard, gets partitioned, and then hits the same unsqueeze path. The
dynamic-quant guard right below it has the same problem in reverse: a
genuine 2d conv with a length-1 stride is refused for no reason.
`Conv2d.define_node` already normalizes `padding` and `output_padding`
from length 1 to length 2, but not `stride` or `dilation`, so
`check_or_raise(len(stride) == 2)` fires and `dilation[1]` would be out
of range. That is the second failure the issue describes, the one
reached by adding `dilation=[2]`.
The fix takes the dimensionality from the weight rank, which is
unambiguous, and normalizes `stride` and `dilation` in the node visitor
the same way the two neighbouring params already are. `_get_act_deps`
moves to the same weight-rank derivation, which also stops a length-1
`padding` from making a 2d conv look 1d and silently blocking the
`constant_pad_nd` fusion.
Review order: `_passes/conv1d_unsqueeze_pass.py`, then
`partition/config/gemm_configs.py`, then `operators/op_conv2d.py`, then
the tests.
### Test plan
Added to `backends/xnnpack/test/ops/test_conv2d.py`:
- `test_fp32_conv2d_single_element_spatial_params`, transposed and not,
covering `stride=(2,)` with `padding=(1,)`
- `test_fp32_conv2d_single_element_dilation`, covering `dilation=(2,)`
- `test_fp32_conv3d_single_element_stride_doesnt_partition`, asserting a
3d conv with `stride=(2,)` still produces zero delegates
I do not have an ExecuTorch C++ runtime built on this machine, so I
could not run `run_method_and_compare_outputs`, and the three tests
above were not executed as unit tests. I verified the export and
lowering half of them directly against this branch and against
`upstream/main` at efd6b55, driving the same modules through
`torch.export.export` plus
`to_edge_transform_and_lower([XnnpackPartitioner()])` and counting
delegate and leftover convolution nodes.
On `upstream/main`:
```
conv2d stride=[2] -> Exception: An error occurred when running the 'Conv1dUnsqueezePass' pass
conv2d stride=[2] dilation=[2] -> Exception: An error occurred when running the 'Conv1dUnsqueezePass' pass
conv3d stride=[2] -> Exception: An error occurred when running the 'Conv1dUnsqueezePass' pass
```
The conv3d row is the partitioner half of the bug. Today that conv is
accepted by `ConvolutionConfig` and then dies in the same pass, so the
"Only support 1D + 2D Conv" guard is not actually holding.
On this branch:
```
conv2d stride=(2,) padding=(1,) transpose=True delegates=1 undelegated_conv=0
conv2d stride=(2,) padding=(1,) transpose=False delegates=1 undelegated_conv=0
conv2d stride=(1,) padding=(1,) dilation=(2,) delegates=1 undelegated_conv=0
conv2d stride=2 (unchanged baseline) delegates=1 undelegated_conv=0
conv1d stride=2 (unchanged baseline) delegates=1 undelegated_conv=0
conv3d stride=(2,) delegates=0 undelegated_conv=1
```
The conv1d and plain conv2d rows are there to show the unsqueeze path
and the ordinary 2d path are untouched.
Because `Conv1dUnsqueezePass` now resolves the weight node before
deciding, I also ran the same check through `XNNPACKQuantizer` plus
`prepare_pt2e` and `convert_pt2e`, to exercise the dequant-unwrap
branch. All four still fully delegate on this branch:
```
q conv1d baseline delegates=1 undelegated_conv=0
q conv2d stride=[2] delegates=1 undelegated_conv=0
q conv2d baseline delegates=1 undelegated_conv=0
q conv1d per-channel delegates=1 undelegated_conv=0
```
And because `_get_act_deps` changed how it decides 1d versus 2d, I
re-ran the even-kernel `same`-padding cases that exercise the
`constant_pad_nd` fusion. The pad and the conv are still both absorbed
into one delegate:
```
qs8 conv2d even-kernel same-padding delegates=1 conv=0 pad=0
qs8 conv1d even-kernel same-padding k=4 delegates=1 conv=0 pad=0
qs8 conv1d same-padding k=4 dilation=2 delegates=1 conv=0 pad=0
```
This PR was authored with AI assistance using Claude Code.
cc @GregoryComer @digantdesai @cbilgin @JakeStevens
Signed-off-by: Aditya Singh <adisin650@gmail.com>1 parent bc5dfe7 commit 9cd0c12
4 files changed
Lines changed: 72 additions & 14 deletions
File tree
- backends/xnnpack
- _passes
- operators
- partition/config
- test/ops
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
| 10 | + | |
10 | 11 | | |
11 | 12 | | |
12 | 13 | | |
| |||
127 | 128 | | |
128 | 129 | | |
129 | 130 | | |
130 | | - | |
131 | | - | |
132 | | - | |
133 | | - | |
134 | | - | |
135 | 131 | | |
136 | 132 | | |
137 | 133 | | |
138 | 134 | | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
139 | 142 | | |
140 | 143 | | |
141 | 144 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
141 | 141 | | |
142 | 142 | | |
143 | 143 | | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
144 | 148 | | |
145 | 149 | | |
| 150 | + | |
| 151 | + | |
146 | 152 | | |
147 | 153 | | |
148 | 154 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
361 | 361 | | |
362 | 362 | | |
363 | 363 | | |
364 | | - | |
365 | | - | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
366 | 371 | | |
367 | 372 | | |
368 | 373 | | |
369 | | - | |
370 | | - | |
371 | 374 | | |
372 | 375 | | |
373 | 376 | | |
374 | 377 | | |
375 | 378 | | |
376 | 379 | | |
377 | | - | |
378 | | - | |
| 380 | + | |
379 | 381 | | |
380 | 382 | | |
381 | 383 | | |
| |||
409 | 411 | | |
410 | 412 | | |
411 | 413 | | |
412 | | - | |
413 | | - | |
414 | | - | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
415 | 417 | | |
416 | 418 | | |
417 | 419 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
173 | 173 | | |
174 | 174 | | |
175 | 175 | | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
176 | 188 | | |
177 | 189 | | |
178 | 190 | | |
| |||
290 | 302 | | |
291 | 303 | | |
292 | 304 | | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
293 | 340 | | |
294 | 341 | | |
295 | 342 | | |
| |||
0 commit comments