Repository navigation
Support 3-D convolution in the portable convolution kernel - #24385
Open
shoumikhin wants to merge 22 commits into
Open
shoumikhin wants to merge 22 commits into
shoumikhin wants to merge 22 commits into
Conversation
check_convolution_args meant to compare the bias length with the number
of output channels, but the conditional operator binds less tightly
than ==, so
bias.size(0) == transposed ? groups * weight.size(1) : weight.size(0)
compared the length with transposed and then used a channel count as the
condition. The check passed for any weight with channels, so a bias
shorter than the number of output channels got through, and the kernel
read past its end.
Add the parentheses. The new test gives a regular and a transposed
convolution a bias of the wrong length and expects both to fail.
check_convolution_args computed weight.size(0) % groups without rejecting groups = 0 first, so it divided by zero, which crashes on x86. Reject groups below 1, as PyTorch does. The new test passes groups = 0 and expects the call to fail.
A program with an nn.Conv3d could be lowered with the XNNPACK
partitioner and saved, but then failed at run time:
Check failed (in.dim() == 3 || in.dim() == 4): Expect input tensor
to be 3-D or 4-D, but got, 5.
XNNPACK does not take convolutions with more than 2 spatial dims, or any
bfloat16 convolution, so they run on the portable convolution kernel,
which only handled 1-D and 2-D. Vision encoders that embed image patches
with a Conv3d, such as Qwen2.5-VL's, hit this.
The kernel now runs every convolution as a 3-D one. It views a 1-D or
2-D convolution as 3-D, with leading spatial dims of size 1, stride 1,
padding 0 and dilation 1, the same way it used to view 1-D as 2-D.
- The regular loop adds up the depth taps outside the channel loop, so a
depth of 1 adds no work per channel. 1-D and 2-D convolutions have one
depth tap, so they add up their terms in the same order as before and
give the same results. Its innermost loop steps the indices by the
strides, and it reads the bias once per output channel of each batch
item instead of once per output value.
- The transposed loop handles one output depth at a time.
- The argument checks and the output size take the number of spatial
dims from the weight, so 3-D arguments get the same checks as 1-D and
2-D ones. get_unsqueezed_sizes and get_unsqueezed_dim_order are no
longer used and are removed.
- The Cortex-M Conv3d test no longer expects a failure.
Tests: a grouped 3-D convolution and a grouped transposed 3-D
convolution match PyTorch. Depth has its own stride, padding and
dilation in both, and the input and weight values do not repeat along
any dim, so mixing up dims or indices changes the result. Both fail
without this change. The invalid input test also checks that a 3-D
convolution rejects a stride, padding or dilation list of the wrong
length, and that a 6-D input is rejected.
shoumikhin
added
the
release notes: ops & kernels
shoumikhin
added
the
release notes: ops & kernels
🔗 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. ✅ No FailuresAs of commit 4631d0d with merge base aed328d ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
Contributor
Author
|
@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
|
The bias length check of a transposed convolution multiplies groups by weight.size(1). It ran before groups was checked, so a program with a huge groups value overflowed int64_t there, which is undefined behavior, even though a later check rejects the call. Run the bias check after groups is known to be between 1 and weight.size(0). The InvalidGroups test now also passes INT64_MAX as groups to a transposed convolution with a bias. Without this change, UndefinedBehaviorSanitizer reports the overflow.
The 3-D convolution change stopped using these two helpers and deleted them. They belong to kernel_ops_util, a public library that other kernels and backends link, so code outside this repository may still call them. Deleting them is a separate cleanup, so restore them unchanged.
The 3-D convolution tests used contiguous tensors only, so nothing checked that a channels-last (NDHWC) tensor with more than one channel is read through its strides. Both 3-D tests now also run with channels-last input, weight and output, like the 2-D tests. For this, TensorFactory::channels_last_like accepts 5-D tensors: depth and height are next to each other in both layouts, so a 5-D tensor is permuted like a 4-D one whose height covers both. The checks that reject 3-D stride, padding and dilation lists of the wrong length also passed before 3-D support, because the rank check rejected every 5-D input. A call with single values now runs first and must succeed, so those checks fail only because of the list length. It also covers single values that apply to every spatial dim.
For a regular convolution, PyTorch raises an error when the dilated kernel is larger than the padded input in any spatial dim. The portable kernel did not check this. Its output size math rounds a negative quotient toward zero, so with a stride above 1 the output got size 1 and was computed from only part of the kernel. For example, an input depth of 2, a kernel depth of 3 and a stride of 2 gave an output depth of 1. 1-D and 2-D convolutions had the same problem. check_convolution_args now applies the PyTorch rule. Transposed convolutions are not affected.
GCC at -Os inlines transposed_kernel_range and transposed_input_range at every call site, once per spatial dim in every dtype instantiation of the transposed convolution. With the depth loop, that made op_convolution.o 14 percent larger than before 3-D support when built for a Cortex-M55 with the Arm GNU toolchain at -Os. The helpers do not depend on the dtype, so one out-of-line copy can serve them all, and the object is now 3 percent larger than before. The helpers run once per output tile or kernel tap, outside the multiply-add loops.
…istakes The call that checks single stride, padding and dilation values used 1, 0 and 1, which the kernel also uses for a dim that gets no value, so it could not catch a single value that is not applied to the depth. It now uses stride 2, padding 1 and dilation 2, and the calls that must fail use four-element lists of the same values. The transposed 3-D test had one output channel per group, so its weight had the same layout in contiguous and channels-last order, and a wrong weight stride went unnoticed. It now has two output channels per group, with new PyTorch reference values.
Contributor
Author
|
Thanks for the review. Pushed:
|
Contributor
Author
|
@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:
|
Stride, padding and dilation had only lower bounds. A huge value overflowed int64_t in the new kernel size check and in the output size math. For example, with dilation 2^62 the dilated kernel size wrapped around to a negative number and passed the check, and the output size that came out was narrowed to a valid 32-bit size, so a 1-D convolution that should fail ran. check_convolution_args now rejects values that do not fit in 32 bits, like the 32-bit tensor sizes, so this int64_t math cannot overflow. A new test passes a huge dilation, padding and stride. It skips ATen, which does not reject these values.
check_convolution_args compared the dtypes of the input, weight and output, but never looked at the bias dtype. For a bias outside the real, half and bfloat16 types, for example Bool, the kernel failed to get a function that loads the bias, then ran anyway and called a null function pointer. Reject such a bias in the argument checks. A new test passes a Bool bias, which crashed without this change. It skips ATen, which accepts a Bool bias.
With the old check order, the INT64_MAX groups case still failed the bias length check, because the overflowed product wrapped to a wrong length. So the test only notices the overflow in an UndefinedBehaviorSanitizer build that halts on errors. CI builds the sanitizer without halting, so say that in the comment.
PyTorch's CPU conv3d returns a contiguous output even for a channels_last_3d input, so the exported program gave the convolution a channels-last input and a contiguous output. The portable kernel needs both in the same layout, so the run on the Arm simulator failed. With one channel the two layouts were the same, which is why the old case passed. The C++ tests cover the kernel's channels-last 3-D path.
The 32-bit bound on stride, padding and dilation left two gaps: - An output size above 2^31 - 1 still fit int64_t, but was then cut to a 32-bit tensor size. Padding 2^31 - 1 on an input of 3 with a kernel of 1 gives 2^32 + 1, which became 1, so a call that should fail ran. - An empty spatial dim made the transposed output size math compute 0 - 1 in size_t. On 32-bit targets that overflowed int64_t. PyTorch rejects empty spatial dims, and now check_convolution_args does too. It also computes each output size and rejects one that does not fit in the 32-bit tensor size. The tests cover both.
Contributor
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]; |
A transposed convolution with a large padding can have a negative output size. For example, an input of 3, a kernel of 1 and padding 2^31 - 1 give 5 - 2^32. The output size check only had an upper bound, and the size became a positive 5 when cut to 32 bits, so the call ran. PyTorch rejects it as too small. Now each output size must be at least 1. The test now covers that case instead of a stride above the 32-bit bound, which the output size check catches too, so it is renamed to HugePaddingAndDilation.
Contributor
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
With a kernel of 3, dilation 2^62 gave a negative output size once the bound was removed, so the new check on output sizes rejected the call too, and the test passed without the bound. With a kernel of 5 the dilated kernel size wraps to exactly 1 and the output size to a valid 3, so only the bound rejects the call.
Contributor
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
PyTorch's CPU conv3d returns a contiguous output even for a channels_last_3d input, so an exported Conv3d with a channels-last input gives the kernel a channels-last input and a contiguous output. The kernel rejected that pair, so such a model failed at run time. The kernel reads the input, weight and output through their own strides, and check_convolution_args already limits the input, the output and a non-transposed weight to the contiguous or the channels-last dim order, so the input and output do not need to match. Drop that check. Both 3-D tests now also run a channels-last input and weight with a contiguous output, and the Cortex-M conv3d test uses a channels_last_3d input again.
check_convolution_args read the kernel size from the weight but never checked it. With a kernel dim of 0, the dilated kernel size became 1 - dilation, so even the kernel size check passed, and an input of 3 gave an output of 4 that no kernel tap ever wrote. PyTorch rejects a kernel size of 0. Check it with kernel_size_is_valid, like the pooling checks do.
The new check for empty spatial dims also rejected inputs whose batch or channel dim is empty, which main and PyTorch accept and turn into an empty output. Allow those again, like PyTorch. The transposed output size math now subtracts 1 from the input size in int64_t instead of size_t, so an empty spatial dim cannot make it wrap on 32-bit targets.
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.
Contributor
Author
|
Thanks. Pushed:
|
Contributor
Author
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 23m 45s —— View job PR Review: #24385Approve once #24380 lands: the reviewers found no bugs in the 3-D loops or the new checks. The only remaining finding is a minor test gap caused by an Arm test-runner bug that has no issue filed. Detailed reviewSummaryThis PR runs 1-D, 2-D and new 3-D convolutions in portable Testing
RecommendationApprove. Merge only after #24380 lands on main, or together with it. The PR's Notes say that landing this change alone makes a static int8 Review steps
Not done:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, bit for bit. The input and output may now have different dim orders, because
torch.exportgives aConv3dwith a channels-last input a contiguous output.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.ofor a Cortex-M55 (Arm GNU toolchain 15.2) grows from 32,484 to 33,263 bytes at-Os, and from 48,477 to 61,457 at-O3.Notes
Conv3dlowered with XNNPACK returns wrong values with no error, or fails to load. Please merge Keep ops above 6 dims out of XNNPACK and fix quantized ops it leaves out #24380 first or together with this change.