Skip to content

Commit 1cd22ed

Browse files
Metal backend: honor channels-last strides in conv2d (pytorch#22952)
### Summary Fixes pytorch#22951 `aoti_torch_mps_convolution` read its operands as contiguous NCHW / OIHW regardless of their strides. Inductor's layout optimization gives conv2d channels-last input and weight and expects a channels-last output, so every conv2d reaching the kernel returned wrong values without raising an error (MobileNetV3: top-1 logit 3094 instead of 8.146). 1x1 convs were unaffected only because inductor lowers them to `mm`. The kernel now classifies the input and the weight from their strides (contiguous or channels-last, ignoring size-1 dimensions, whose stride carries no information). Channels-last buffers are declared to MPSGraph in their physical NHWC / OHWI shape and reordered inside the graph, the convolution itself is still described as NCHW / OIHW, and the result is written in channels-last order with matching strides when either operand is channels-last, as ATen does. The two layout flags are part of the graph cache key. Any other stride pattern now returns `InvalidArgument` instead of computing garbage, matching how `aoti_torch_mps_bmm_out` treats layouts it does not support. Contiguous operands, conv1d and unbatched 3D input take the same path as before. ### Test plan Added five conv2d modules to `MODULE_REGISTRY` in `backends/apple/metal/tests/test_modules.py` (plain, bias with batch 2, stride 2 on a non-square input, depthwise, and two stacked convolutions so a channels-last output feeds the next conv). Each runs in float32 and bfloat16. ``` backends/apple/metal/tests/run_metal_test.sh --build python -m unittest backends.apple.metal.tests.test_modules.TestMetalBackendModules ``` - Before the change: the 10 new `conv2d_*_output_consistency` tests fail with `max_atol` between 1.6 and 2.7; everything else passes. - After: `Ran 134 tests ... OK`. - The previously wrong MobileNetV3-small `.pte` (exported by the 1.5.0 release, layout optimization on) now matches eager through `executor_runner` on an all-ones input: first logits `-0.0298, -0.1163, 0.2346, -0.1151, 0.2841, 1.3327, -1.2021, -0.4182` on both sides. - `lintrunner` clean on the touched files.
1 parent e0912bd commit 1cd22ed

8 files changed

Lines changed: 492 additions & 31 deletions

File tree

‎backends/apple/metal/runtime/ops/op_bmm.mm‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,9 @@ AOTITorchError aoti_torch_mps_bmm_out(
110110
auto mat2_strides = mat2_tensor->strides();
111111
auto out_strides = out_tensor->strides();
112112

113-
// Check self tensor is contiguous [B, M, K] with strides [M*K, K, 1]
114-
if (self_strides[2] != 1 || self_strides[1] != K || self_strides[0] != M * K) {
113+
// Check self tensor is contiguous [B, M, K] with strides [M*K, K, 1]. As
114+
// in PyTorch, the stride of a dimension of size 1 is not looked at.
115+
if (!is_row_major_dense(*self_tensor)) {
115116
ET_LOG(Error, "aoti_torch_mps_bmm_out: self tensor must be contiguous. "
116117
"Only dense row-major layout supported; transposed/view tensors are unsupported. "
117118
"Expected strides=[%lld,%lld,1] for shape=[%lld,%lld,%lld], got strides=[%lld,%lld,%lld].",
@@ -121,7 +122,7 @@ AOTITorchError aoti_torch_mps_bmm_out(
121122
}
122123

123124
// Check mat2 tensor is contiguous [B, K, N] with strides [K*N, N, 1]
124-
if (mat2_strides[2] != 1 || mat2_strides[1] != N || mat2_strides[0] != K * N) {
125+
if (!is_row_major_dense(*mat2_tensor)) {
125126
ET_LOG(Error, "aoti_torch_mps_bmm_out: mat2 tensor must be contiguous. "
126127
"Only dense row-major layout supported; transposed/view tensors are unsupported. "
127128
"Expected strides=[%lld,%lld,1] for shape=[%lld,%lld,%lld], got strides=[%lld,%lld,%lld].",
@@ -131,7 +132,7 @@ AOTITorchError aoti_torch_mps_bmm_out(
131132
}
132133

133134
// Check out tensor is contiguous [B, M, N] with strides [M*N, N, 1]
134-
if (out_strides[2] != 1 || out_strides[1] != N || out_strides[0] != M * N) {
135+
if (!is_row_major_dense(*out_tensor)) {
135136
ET_LOG(Error, "aoti_torch_mps_bmm_out: out tensor must be contiguous. "
136137
"Only dense row-major layout supported; transposed/view tensors are unsupported. "
137138
"Expected strides=[%lld,%lld,1] for shape=[%lld,%lld,%lld], got strides=[%lld,%lld,%lld].",

‎backends/apple/metal/runtime/ops/op_convolution.mm‎

Lines changed: 164 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,88 @@
1212
namespace backends {
1313
namespace metal {
1414

15+
namespace {
16+
17+
// How a 4D convolution operand is laid out in memory. Inductor's layout
18+
// optimization hands conv2d channels-last input and weight, and expects a
19+
// channels-last output back.
20+
enum class ConvMemoryFormat { Contiguous, ChannelsLast };
21+
22+
// Mirrors c10's is_channels_last_strides_2d, which is what PyTorch's
23+
// suggest_memory_format() runs on a 4D tensor.
24+
bool suggests_channels_last(const Tensor& tensor) {
25+
const auto sizes = tensor.sizes();
26+
const auto strides = tensor.strides();
27+
if (strides[1] == 0) {
28+
return false;
29+
}
30+
int64_t min = 0;
31+
for (int d : {1, 3, 2, 0}) {
32+
if (sizes[d] == 0 || strides[d] < min) {
33+
return false;
34+
}
35+
// Tells N111 (contiguous) apart from NC11 stored channels-last.
36+
if (d == 0 && min == strides[1]) {
37+
return false;
38+
}
39+
min = strides[d];
40+
if (sizes[d] > 1) {
41+
min *= sizes[d];
42+
}
43+
}
44+
return true;
45+
}
46+
47+
// Classifies a 4D tensor from its strides. A dimension of size 1 does not
48+
// change where the elements are, so it is ignored when checking that the data
49+
// is dense in one of the two layouts. A tensor with such a dimension can fit
50+
// both; its elements read the same either way, but the layout still decides
51+
// the layout of the output, and the generated wrapper expects the one PyTorch
52+
// would pick. Returns false for any other stride pattern.
53+
bool get_conv_memory_format(const Tensor& tensor, ConvMemoryFormat* format) {
54+
const auto sizes = tensor.sizes();
55+
const auto strides = tensor.strides();
56+
const int64_t d1 = sizes[1], d2 = sizes[2], d3 = sizes[3];
57+
const int64_t contiguous[4] = {d1 * d2 * d3, d2 * d3, d3, 1};
58+
const int64_t channels_last[4] = {d2 * d3 * d1, 1, d3 * d1, d1};
59+
60+
bool is_contiguous = true;
61+
bool is_channels_last = true;
62+
for (int i = 0; i < 4; i++) {
63+
if (sizes[i] == 1) {
64+
continue;
65+
}
66+
is_contiguous = is_contiguous && strides[i] == contiguous[i];
67+
is_channels_last = is_channels_last && strides[i] == channels_last[i];
68+
}
69+
70+
if (is_contiguous && is_channels_last) {
71+
*format = suggests_channels_last(tensor) ? ConvMemoryFormat::ChannelsLast
72+
: ConvMemoryFormat::Contiguous;
73+
} else if (is_contiguous) {
74+
*format = ConvMemoryFormat::Contiguous;
75+
} else if (is_channels_last) {
76+
*format = ConvMemoryFormat::ChannelsLast;
77+
} else {
78+
return false;
79+
}
80+
return true;
81+
}
82+
83+
// Reorders a 4D graph tensor from `N, d1, d2, C` to `N, C, d1, d2`.
84+
MPSGraphTensor* channels_last_to_first(MPSGraph* graph, MPSGraphTensor* tensor) {
85+
tensor = [graph transposeTensor:tensor dimension:1 withDimension:3 name:nil];
86+
return [graph transposeTensor:tensor dimension:2 withDimension:3 name:nil];
87+
}
88+
89+
// Reorders a 4D graph tensor from `N, C, d1, d2` to `N, d1, d2, C`.
90+
MPSGraphTensor* channels_first_to_last(MPSGraph* graph, MPSGraphTensor* tensor) {
91+
tensor = [graph transposeTensor:tensor dimension:1 withDimension:2 name:nil];
92+
return [graph transposeTensor:tensor dimension:2 withDimension:3 name:nil];
93+
}
94+
95+
} // namespace
96+
1597
extern "C" {
1698

1799
AOTITorchError aoti_torch_mps_convolution(
@@ -132,6 +214,29 @@ AOTITorchError aoti_torch_mps_convolution(
132214
}
133215
}
134216

217+
// Inductor picks channels-last for conv2d graphs, so the operands cannot
218+
// be assumed to be contiguous. As in ATen, the output is channels-last
219+
// when either operand is.
220+
ConvMemoryFormat input_format = ConvMemoryFormat::Contiguous;
221+
ConvMemoryFormat weight_format = ConvMemoryFormat::Contiguous;
222+
if (!is_conv1d && is_input_4d) {
223+
if (!get_conv_memory_format(*input_tensor, &input_format)) {
224+
ET_LOG(Error, "aoti_torch_mps_convolution: input must be contiguous or channels-last, got strides [%d, %d, %d, %d]",
225+
(int)input_tensor->strides()[0], (int)input_tensor->strides()[1],
226+
(int)input_tensor->strides()[2], (int)input_tensor->strides()[3]);
227+
return Error::InvalidArgument;
228+
}
229+
if (!get_conv_memory_format(*weight_tensor, &weight_format)) {
230+
ET_LOG(Error, "aoti_torch_mps_convolution: weight must be contiguous or channels-last, got strides [%d, %d, %d, %d]",
231+
(int)weight_tensor->strides()[0], (int)weight_tensor->strides()[1],
232+
(int)weight_tensor->strides()[2], (int)weight_tensor->strides()[3]);
233+
return Error::InvalidArgument;
234+
}
235+
}
236+
const bool input_channels_last = input_format == ConvMemoryFormat::ChannelsLast;
237+
const bool weight_channels_last = weight_format == ConvMemoryFormat::ChannelsLast;
238+
const bool output_channels_last = input_channels_last || weight_channels_last;
239+
135240
// Get weight dimensions
136241
int64_t C_out = weight_tensor->sizes()[0]; // output channels
137242
int64_t kernel_h = is_conv1d ? 1 : weight_tensor->sizes()[2]; // kernel height
@@ -220,13 +325,26 @@ AOTITorchError aoti_torch_mps_convolution(
220325
int64_t weight_C_in = weight_tensor->sizes()[1]; // This handles grouped convs correctly
221326

222327
// Define tensor shapes for placeholders (needed for both cache hit and miss)
223-
NSArray<NSNumber*>* inputShape = @[@(N), @(C_in), @(H_in), @(W_in)];
224-
NSArray<NSNumber*>* weightShape = @[@(C_out), @(weight_C_in), @(kernel_h), @(kernel_w)];
328+
// These describe the buffers as they sit in memory, so a channels-last
329+
// operand is declared NHWC / OHWI and reordered inside the graph.
330+
NSArray<NSNumber*>* inputShape = input_channels_last
331+
? @[@(N), @(H_in), @(W_in), @(C_in)]
332+
: @[@(N), @(C_in), @(H_in), @(W_in)];
333+
NSArray<NSNumber*>* weightShape = weight_channels_last
334+
? @[@(C_out), @(kernel_h), @(kernel_w), @(weight_C_in)]
335+
: @[@(C_out), @(weight_C_in), @(kernel_h), @(kernel_w)];
225336

226337
// Create cache key for this convolution
227338
GraphCacheKey cache_key;
228339
cache_key.op_name = "conv";
229-
cache_key.shape_params = {N, C_in, H_in, W_in, C_out, kernel_h, kernel_w, stride_h, stride_w, pad_h, pad_w, dil_h, dil_w, groups};
340+
// The layouts and the bias change the graph's topology, not just its
341+
// shapes, and a transposed convolution's output padding changes its
342+
// descriptor, so they all have to be part of the key.
343+
const int64_t key_output_pad_h = transposed && output_padding && output_padding_len_ > 0 ? output_padding[0] : 0;
344+
const int64_t key_output_pad_w = transposed && output_padding && output_padding_len_ > 1 ? output_padding[1] : 0;
345+
cache_key.shape_params = {N, C_in, H_in, W_in, C_out, kernel_h, kernel_w, stride_h, stride_w, pad_h, pad_w, dil_h, dil_w, groups,
346+
input_channels_last, weight_channels_last, bias_tensor != nullptr,
347+
key_output_pad_h, key_output_pad_w};
230348
cache_key.dtype = dtype;
231349
cache_key.transpose_flag = (transposed != 0);
232350

@@ -274,6 +392,14 @@ AOTITorchError aoti_torch_mps_convolution(
274392

275393
ET_LOG(Debug, "aoti_torch_mps_convolution: Created input and weight placeholders");
276394

395+
// The convolution below is described as NCHW / OIHW.
396+
MPSGraphTensor* convInput = input_channels_last
397+
? channels_last_to_first(mpsGraph, inputPlaceholder)
398+
: inputPlaceholder;
399+
MPSGraphTensor* convWeight = weight_channels_last
400+
? channels_last_to_first(mpsGraph, weightPlaceholder)
401+
: weightPlaceholder;
402+
277403
// Create convolution descriptor
278404
MPSGraphConvolution2DOpDescriptor* convDesc = [MPSGraphConvolution2DOpDescriptor descriptorWithStrideInX:stride_w
279405
strideInY:stride_h
@@ -318,14 +444,14 @@ AOTITorchError aoti_torch_mps_convolution(
318444
dataLayout:MPSGraphTensorNamedDataLayoutNCHW
319445
weightsLayout:MPSGraphTensorNamedDataLayoutOIHW];
320446

321-
convOutput = [mpsGraph convolution2DWithSourceTensor:inputPlaceholder
322-
weightsTensor:weightPlaceholder
447+
convOutput = [mpsGraph convolution2DWithSourceTensor:convInput
448+
weightsTensor:convWeight
323449
descriptor:transposedConvDesc
324450
name:@"transposed_convolution"];
325451
} else {
326452
ET_LOG(Debug, "aoti_torch_mps_convolution: Using regular convolution");
327-
convOutput = [mpsGraph convolution2DWithSourceTensor:inputPlaceholder
328-
weightsTensor:weightPlaceholder
453+
convOutput = [mpsGraph convolution2DWithSourceTensor:convInput
454+
weightsTensor:convWeight
329455
descriptor:convDesc
330456
name:@"convolution"];
331457
}
@@ -342,16 +468,25 @@ AOTITorchError aoti_torch_mps_convolution(
342468
dataType:mps_dtype
343469
name:@"bias"];
344470

345-
// Add bias to convolution output
471+
// Add bias to convolution output. MPSGraph broadcasts from the
472+
// trailing dimension, so the rank-1 bias has to be shaped to line up
473+
// with the channels of the NCHW result rather than with its width.
474+
MPSGraphTensor* biasPerChannel = [mpsGraph reshapeTensor:biasPlaceholder
475+
withShape:@[@1, @(C_out), @1, @1]
476+
name:@"bias_per_channel"];
346477
finalOutput = [mpsGraph additionWithPrimaryTensor:convOutput
347-
secondaryTensor:biasPlaceholder
478+
secondaryTensor:biasPerChannel
348479
name:@"add_bias"];
349480

350481
ET_LOG(Debug, "aoti_torch_mps_convolution: Added bias placeholder to graph");
351482
} else {
352483
finalOutput = convOutput;
353484
}
354485

486+
if (output_channels_last) {
487+
finalOutput = channels_first_to_last(mpsGraph, finalOutput);
488+
}
489+
355490
// Cache the compiled graph and tensor references for reuse
356491
CachedGraph cached_graph;
357492
cached_graph.graph = mpsGraph;
@@ -408,7 +543,9 @@ AOTITorchError aoti_torch_mps_convolution(
408543
id<MTLBuffer> output_buffer = allocate_mtl_buffer(&output_contents_ptr, output_size_bytes);
409544

410545
// Create results dictionary (MPSGraph output is 4D)
411-
NSArray<NSNumber*>* outputShape = @[@(N), @(C_out), @(H_out), @(W_out)];
546+
NSArray<NSNumber*>* outputShape = output_channels_last
547+
? @[@(N), @(H_out), @(W_out), @(C_out)]
548+
: @[@(N), @(C_out), @(H_out), @(W_out)];
412549
MPSGraphTensorData* outputData = [[MPSGraphTensorData alloc] initWithMTLBuffer:output_buffer
413550
shape:outputShape
414551
dataType:mps_dtype];
@@ -438,13 +575,23 @@ AOTITorchError aoti_torch_mps_convolution(
438575
std::vector<int64_t> output_strides;
439576
if (!is_conv1d && is_input_4d) {
440577
output_sizes_int64 = {N, C_out, H_out, W_out};
441-
// Contiguous NCHW strides
442-
output_strides = {
443-
C_out * H_out * W_out,
444-
H_out * W_out,
445-
W_out,
446-
1
447-
};
578+
if (output_channels_last) {
579+
// Channels-last (NHWC) strides
580+
output_strides = {
581+
H_out * W_out * C_out,
582+
1,
583+
W_out * C_out,
584+
C_out
585+
};
586+
} else {
587+
// Contiguous NCHW strides
588+
output_strides = {
589+
C_out * H_out * W_out,
590+
H_out * W_out,
591+
W_out,
592+
1
593+
};
594+
}
448595
} else if (!is_conv1d) {
449596
output_sizes_int64 = {C_out, H_out, W_out};
450597
// Contiguous CHW strides

‎backends/apple/metal/runtime/ops/op_linear_4bit.mm‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1642,8 +1642,9 @@ AOTITorchError aoti_torch_mps__linear_fp_act_4bit_weight(
16421642
ET_LOG(Error, "aoti_torch_mps__linear_fp_act_4bit_weight: expect A to be 32-bit or 16-bit float tensor, got dtype %d", (int)a_dtype);
16431643
return Error::InvalidArgument;
16441644
}
1645-
// Check A is contiguous (stride[1] == 1 and stride[0] == size[1])
1646-
if (a_tensor->strides()[1] != 1 || a_tensor->strides()[0] != a_tensor->sizes()[1]) {
1645+
// Check A is contiguous (stride[1] == 1 and stride[0] == size[1], ignoring
1646+
// the stride of a dimension of size 1)
1647+
if (!is_row_major_dense(*a_tensor)) {
16471648
ET_LOG(Error, "aoti_torch_mps__linear_fp_act_4bit_weight: expect A to be contiguous, strides=[%lld, %lld]",
16481649
(long long)a_tensor->strides()[0], (long long)a_tensor->strides()[1]);
16491650
return Error::InvalidArgument;
@@ -1660,7 +1661,7 @@ AOTITorchError aoti_torch_mps__linear_fp_act_4bit_weight(
16601661
return Error::InvalidArgument;
16611662
}
16621663
// Check B is contiguous
1663-
if (b_tensor->strides()[1] != 1 || b_tensor->strides()[0] != b_tensor->sizes()[1]) {
1664+
if (!is_row_major_dense(*b_tensor)) {
16641665
ET_LOG(Error, "aoti_torch_mps__linear_fp_act_4bit_weight: expect B to be contiguous, strides=[%lld, %lld]",
16651666
(long long)b_tensor->strides()[0], (long long)b_tensor->strides()[1]);
16661667
return Error::InvalidArgument;
@@ -1705,7 +1706,7 @@ AOTITorchError aoti_torch_mps__linear_fp_act_4bit_weight(
17051706
(int)a_dtype, (int)s_tensor->scalar_type());
17061707
return Error::InvalidArgument;
17071708
}
1708-
if (s_tensor->strides()[1] != 1 || s_tensor->strides()[0] != s_tensor->sizes()[1]) {
1709+
if (!is_row_major_dense(*s_tensor)) {
17091710
ET_LOG(Error, "aoti_torch_mps__linear_fp_act_4bit_weight: expect S to be contiguous, strides=[%lld, %lld]",
17101711
(long long)s_tensor->strides()[0], (long long)s_tensor->strides()[1]);
17111712
return Error::InvalidArgument;
@@ -1722,7 +1723,7 @@ AOTITorchError aoti_torch_mps__linear_fp_act_4bit_weight(
17221723
(int)a_dtype, (int)z_tensor->scalar_type());
17231724
return Error::InvalidArgument;
17241725
}
1725-
if (z_tensor->strides()[1] != 1 || z_tensor->strides()[0] != z_tensor->sizes()[1]) {
1726+
if (!is_row_major_dense(*z_tensor)) {
17261727
ET_LOG(Error, "aoti_torch_mps__linear_fp_act_4bit_weight: expect Z to be contiguous, strides=[%lld, %lld]",
17271728
(long long)z_tensor->strides()[0], (long long)z_tensor->strides()[1]);
17281729
return Error::InvalidArgument;

0 commit comments

Comments
 (0)