Repository navigation
Support 3-D convolution in the portable convolution kernel - #24385
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24385
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 1 Unrelated FailureAs of commit 33241ab with merge base e68a824 ( NEW FAILURE - The following job has failed:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 32m 34s —— View job PR Review: #24385Needs Discussion: the PR removes two helpers from a public util target, multiplies by Detailed reviewSummaryThe PR sends 1-D, 2-D and the new 3-D convolutions through a single 3-D implementation in the portable Code Quality
Testing
Backward Compatibility
Performance
RecommendationNeeds Discussion The reviewers found no blocking bug in the kernel. Three decisions are needed before merge:
The
|
|
Thanks for the review. Pushed:
|
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 35m 37s —— View job PR Review: #24385Approve once #24380 lands: I found no bugs in the new 3-D loops, and the remaining items don't block: an overflow that gets past the new kernel-size check, an unchecked bias dtype, and two tests that check less than they appear to. Detailed reviewSummaryThis PR sends 1-D, 2-D and new 3-D convolutions in portable
Code Quality
Testing
RecommendationApprove. Merge only after #24380 lands on main, or together with it. This branch is based on Review steps
Not done:
|
There was a problem hiding this comment.
🟡 Changes recommended
Negative transposed-convolution output sizes can narrow to valid depths and silently produce incorrect results, while tensors without stored strides can trigger invalid memory access or crashes.
2 open issues
Review effort: Balanced
| for (const auto d : c10::irange(t.dim())) { | ||
| const ssize_t view_d = d < 2 ? d : d + added_dims; | ||
| sizes[view_d] = t.size(d); | ||
| strides[view_d] = t.strides()[d]; |
There was a problem hiding this comment.
Thanks. Tensors are required to have strides: the TensorImpl constructor documents that strides must contain one entry per dim. The program loader and make_tensor_ptr both fill them in. Many portable kernels already read strides directly, including the shared broadcast and reduction helpers, so a tensor without strides would fail in those too. So I kept this kernel reading strides directly, like those kernels do.
There was a problem hiding this comment.
🔵 Needs a closer look
Maintainers must validate the internal jobs that caused the prior Conv3d revert and coordinate landing with still-open #24380 to prevent incorrect quantized XNNPACK outputs.
1 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Internal-job validation remains unavailable after the prior Conv3d regression-driven revert, and merge ordering with #24380 must be coordinated to prevent silent quantized XNNPACK output corruption.
Review effort: Balanced
With a channels_last_3d input the case now ran on the Arm simulator but gave wrong values. The Arm test runner writes an input in its logical order unless its dim order is NHWC or the runner's own 5-D order, so a channels_last_3d input reached the program in the wrong byte order. The kernel handles a channels-last input with a contiguous output, as the C++ 3-D tests show, so give the Cortex-M case a contiguous input, with a comment that says why.
The kernel size check fixes 1-D and 2-D convolutions as well, but its test used only a 5-D input, which main rejected at the rank check anyway. Add a 1-D case: an input of 2 with a kernel of 3 and stride 2, which main ran from two of the three kernel taps.
Say why the bias length check comes after the groups checks: they keep groups * weight.size(1) from overflowing. Before this, only a test comment said so. Also fix three comments that were hard to read or not quite right: the output size math can round a too-large kernel up to 1, but only when the kernel is too large by less than the stride; the range helper comment is reworded; and the depth loop comment now says the loop runs once for 1-D and 2-D inputs.
Three cases had no test, so the code that handles them could break without any test failing: - An empty spatial dim with an empty channel dim, which PyTorch accepts. Only the empty batch case was tested. - A transposed convolution with a huge stride or padding, where the 64-bit output size math would wrap around to a valid size. Only the dilation part of the 32-bit bound was tested. - A channels-last 5-D input with a contiguous weight and a contiguous output, which is what export gives when only the input is channels-last. The mixed layout calls used a channels-last weight.
The backend test suite skipped every model with a 5-D convolution, because the portable kernel could not run one. Now it can, so remove the skip, and the suite's Conv3d and ConvTranspose3d cases run in every flow.
caf610b to
33241ab
Compare
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 25m 40s —— View job Approve: the three reviewers found no bugs in the 3-D kernel, the argument checks or the tests, so what remains is to merge after #24380 and to check the Conv3d results of this head's backend test jobs, which are still running and pass even when cases fail. Detailed reviewSummaryThis PR runs 1-D, 2-D and 3-D convolutions in the portable RecommendationApprove
Review steps
Moved to the Recommendation
Dropped
Not done
|
|
@pytorchbot cherry-pick --onto release/1.6 -c critical |
Cherry picking #24385The cherry pick PR is at #24460 and it is recommended to link a critical cherry pick PR with an issue. Details for Dev Infra teamRaised by workflow job |
…out (#24445) ## What is wrong today XNNPACK supports tensors with at most 6 dimensions, but the XNNPACK partitioner, which decides which ops run on XNNPACK, does not check this. A permute or another op on a 7-D or 8-D tensor then lowers fine but fails to load: ``` Failed to define tensor 0 with code: xnn_status_unsupported_parameter ``` Some vision models hit this in a "patchify" step that cuts an image into patches: ```python y = x.reshape(2, 6, 8, 2, 16, 8, 2, 16).permute(0, 2, 5, 3, 6, 1, 4, 7) y = y.reshape(2, 64, -1) * 2 ``` With the 3-D convolution kernel from #24385, a statically quantized `Conv3d` is worse: it fails to load or, without a fused ReLU, returns one wrong value repeated everywhere, with no error. XNNPACK has no 3-D convolution, so the partitioner leaves the conv out, but only after marking the quantize and dequantize nodes around it as folded into it. Those nodes then land in delegates that skip them, so nothing writes the outputs. A statically quantized `Conv2d` or `Linear` whose output is not quantized hits the same bug, even without #24385. ## What this change does The partitioner now leaves out any op with an input or output above 6 dimensions, and it checks every node that a partition takes. A float linear on such an input still keeps its 2-D matrix multiply on XNNPACK. Quantize and dequantize nodes are now marked as folded only once the op they belong to is really partitioned, so a rejected op keeps them as separate ops. `force_non_static_weights_for_f32_linear` no longer changes how a quantized linear is partitioned. With it, a quantized linear decomposed into a matrix multiply (for example with `to_backend`) used to lose its quantize and dequantize nodes: a static one failed to lower or load, and a dynamic one ran as a float matrix multiply. Now it is partitioned as without the flag. The XNNPACK quantizer now leaves 3-D convolutions in float. The model file gets larger, but there are no extra quantize and dequantize steps and no quantization error. They run on the CPU kernels, which support 3-D convolution once #24385 lands. Without it, such a model fails with "Expect input tensor to be 3-D or 4-D". Still not covered: a linear on such an input, quantized with torchao's affine ops. Those ops have no portable kernels, so the model now fails in `to_executorch()` instead of at load. And a linear on such an input quantized with the XNNPACK quantizer runs outside XNNPACK, because its quantized input has too many dims. ## Testing - `test_permute.py` checks which ops are delegated for 6-D and 7-D permutes, and for a linear on an 8-D input: float (also with a ReLU), static and dynamic quantization, each in both partitioner modes. - New tests cover a statically quantized Conv2d and Linear with a float output, a statically and a dynamically quantized linear lowered through `to_backend` with `force_non_static_weights_for_f32_linear`, and 3-D convolutions that the quantizer leaves unquantized. - The per-channel transposed conv test now also runs the convs that XNNPACK rejects. Before this change they returned all zeros. - Each new test fails without the part of the change it covers. - On macOS arm64, the patchify example and other 7-D and 8-D cases load and match eager PyTorch. A Conv3d model quantized with the XNNPACK quantizer matches eager PyTorch with #24385, and fails without it. Co-authored-by: Anthony Shoumikhin <anthony@shoumikh.in> Co-authored-by: Jacob Szwejbka <jakeszwe@meta.com>

What is wrong today
A model with
torch.nn.Conv3dlowers without an error but fails at run time on CPU, for example with XNNPACK:XNNPACK does not take 3-D or bfloat16 convolutions, so they run on the portable convolution kernel, which only supported 1-D and 2-D. This blocks vision encoders with a
Conv3dpatch embedding, such as Qwen2-VL and Qwen2.5-VL. It is the failure in #11704.What this change does
The portable convolution kernel now runs 3-D convolutions, regular and transposed. 1-D and 2-D convolutions run as 3-D ones with extra dims of size 1 and add up their terms in the same order as before, so their results stay the same (except that, with fused multiply-add, a result of -0 can now be +0, as in PyTorch). The input and output may now have different dim orders, because
torch.exportgives aConv3dwith a channels-last input a contiguous output. The backend test suite no longer skips 3-D convolutions.It also fixes older bugs in the argument checks, which affect 1-D and 2-D too:
groups = 0divided by zero, and a hugegroupscould overflow in the bias check.Testing
conv3dtest, now with several channels, a 2x3x3 kernel, stride and padding, no longer expects a failure and passes in CI on the Arm simulator.Conv3dpatch embedding lowered with XNNPACK, and random convolutions on the portable kernel, match eager PyTorch in float32 and bfloat16.-Os, and 18 percent faster to 11 percent slower with Clang at-O2and-O3. Transposed ones are 5 percent faster to 17 percent slower.op_convolution.ogrows from 32,484 to 33,263 bytes at-Osand from 48,477 to 61,457 at-O3.kernel_ops_util.o, which holds the argument checks, grows from 15,746 to 17,743 bytes at-Osand from 25,932 to 28,979 at-O3.Notes
Conv3dlowered with XNNPACK returns wrong values with no error, or fails to load, and the backend test suite's static int8 XNNPACKConv3dcases fail. Please merge Keep ops above 6 dims out of XNNPACK and fix quantized ops it leaves out #24380 first or together with this change.