Skip to content

Conversation

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Sep 22, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/163584

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 5c8e8dd with merge base 51152ef (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[ghstack-poisoned]
jansel added a commit that referenced this pull request Sep 22, 2025
Fixes #163569


ghstack-source-id: 6f25661
Pull-Request: #163584
[ghstack-poisoned]
[ghstack-poisoned]
@thenumberouscode
Copy link
Contributor

thenumberouscode commented Sep 23, 2025

I actually have a previous PR (https://github.com/pytorch/pytorch/pull/160408/files) that implements a similar approach, which could both resolve #163569 and #159462.

No offense intended - this is purely a technical discussion. I believe we should align the conv1d meta kernel with the actual convolution kernel implementation. Specifically, we could utilize _select_conv_backend and _conv_determine_backend_memory_format within the conv function in fake_impls.py to determine the correct memory format. This would eliminate the need for the current workaround in pick_memory_format and provide a unified method for memory format selection.

convolution kernel implementation:

// Expand 1d -> 2d.
// This is only done for backends that don't natively support 1d spatial input.
if (k == 3 && !input.is_mkldnn() && !input.is_xpu()) {
// avoid accidentally going through NHWC for permuted 3d input.
input = input.contiguous();
params.view1d_as_2d();
input = view4d(input);
weight = view4d(weight);
}
// Select appropriate backend to use.
auto bias_sizes_opt = bias.defined() ? std::optional<IntArrayRef>(bias.sizes()) : std::nullopt;
bool need_backward = GradMode::is_enabled() &&
(input.requires_grad() || weight.requires_grad() || (bias.defined() && bias.requires_grad()));
ConvBackend backend = _select_conv_backend(input, weight, bias, c10::OptionalIntArrayRef(bias_sizes_opt), need_backward, params);
at::MemoryFormat backend_memory_format = determine_backend_memory_format(input, weight, backend);

cc @eellison

if ndim == 3 and weight.dim() == 3:
# Unsqueeze to reuse conv2d heuristics.
return (
utils.suggest_memory_format(weight.unsqueeze(-1)) == torch.channels_last
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using only the weight memory format isn't enough to determine the correct format. Issue #159462 provides a test case that demonstrates this limitation.

@jansel
Copy link
Contributor Author

jansel commented Sep 23, 2025

@thenumberouscode let's go with your implementation then, thanks for catching this I wasn't aware of your PR

@jansel jansel marked this pull request as draft September 23, 2025 04:39
[ghstack-poisoned]
jansel added a commit that referenced this pull request Sep 23, 2025
Fixes #163569

ghstack-source-id: d501232
Pull-Request: #163584
@jansel jansel closed this Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants