Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add transposed case for at::convolution #917

Merged
merged 1 commit into from
Aug 24, 2022
Merged

Add transposed case for at::convolution #917

merged 1 commit into from
Aug 24, 2022

Conversation

gpetters94
Copy link
Collaborator

Here's transposed support for convolution, both strided and unstrided.

@dan-garvey
Copy link
Collaborator

Can you squash the commits

silvasean
silvasean reviewed Jun 13, 2022
View reviewed changes
Copy link
Contributor

@silvasean silvasean left a comment

Choose a reason for hiding this comment

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

@ramiro050 could you PTAL

python/torch_mlir/dialects/torch/importer/jit_ir/build_tools/shape_lib_gen.py Outdated Show resolved Hide resolved
ramiro050
ramiro050 requested changes Jun 13, 2022
View reviewed changes
Copy link
Collaborator

@ramiro050 ramiro050 left a comment

Choose a reason for hiding this comment

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

A few initial comments

lib/Conversion/TorchToLinalg/Linear.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/Linear.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/Linear.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/Linear.cpp Outdated Show resolved Hide resolved
python/torch_mlir/dialects/torch/importer/jit_ir/build_tools/shape_lib_gen.py Outdated Show resolved Hide resolved
@gpetters94 gpetters94 mentioned this pull request Jun 15, 2022
Closed
ramiro050
ramiro050 requested changes Jun 22, 2022
View reviewed changes
Copy link
Collaborator

@ramiro050 ramiro050 left a comment

Choose a reason for hiding this comment

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

A few more comments/questions

lib/Conversion/TorchToLinalg/Linear.cpp Show resolved Hide resolved
lib/Conversion/TorchToLinalg/Linear.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/Linear.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/Linear.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/Linear.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/Utils.cpp Outdated Show resolved Hide resolved
lib/Dialect/Torch/Transforms/DecomposeComplexOps.cpp Outdated Show resolved Hide resolved
python/torch_mlir/dialects/torch/importer/jit_ir/build_tools/torch_ods_gen.py Show resolved Hide resolved
python/torch_mlir_e2e_test/test_suite/conv.py Outdated Show resolved Hide resolved
python/torch_mlir_e2e_test/test_suite/conv.py Outdated Show resolved Hide resolved
@gpetters94 gpetters94 force-pushed the convolution_transpose branch 2 times, most recently from e683a3c to a035e6c Compare June 25, 2022 06:57
ramiro050
ramiro050 approved these changes Jun 27, 2022
View reviewed changes
Copy link
Collaborator

@ramiro050 ramiro050 left a comment

Choose a reason for hiding this comment

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

LGTM! I just have one question

lib/Conversion/TorchToLinalg/Linear.cpp Show resolved Hide resolved
@gpetters94 gpetters94 mentioned this pull request Jul 5, 2022
Closed
vivekkhandelwal1
vivekkhandelwal1 requested changes Aug 5, 2022
View reviewed changes
Copy link
Collaborator

@vivekkhandelwal1 vivekkhandelwal1 left a comment

Choose a reason for hiding this comment

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

@gpetters94 Could you please resolve the conflicts?

@gpetters94
Copy link
Collaborator Author

@gpetters94 Could you please resolve the conflicts?

Done.

@ZihengJiang
Copy link
Collaborator

Can we approve and merge this PR? @vivekkhandelwal1 @silvasean

@silvasean
Copy link
Contributor

It looks like the shape function is not correct (failing PyTorch tests), see pytorch/pytorch#80860

@silvasean
Copy link
Contributor

Oh, sorry, it is an "incorrect shape compute mapping function schema name" not incorrect shape function. Yes, we in general wait for these change to land upstream. Is there an urgent reason to merge it now?

@ZihengJiang
Copy link
Collaborator

We are working on the transposed convolution on the Torch2MHLO lowering side so checked this PR to see whether we can merge it. It's ok to wait the upstream PR to be merged though.

@gpetters94
Copy link
Collaborator Author

It's holding up U-Net support, but it isn't the only thing blocking that.

pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request Aug 13, 2022
@gpetters94 @pytorchmergebot
Add shape functions for conv_transpose2d.input and convolution (#80860)
a7e7fba 
As @silvasean requested in [this issue](llvm/torch-mlir#917 (comment)) here is the shape code from Torch-MLIR for conv_transpose2d.input and convolution (updated for the transposed case).
Pull Request resolved: #80860
Approved by: https://github.com/Gamrix
@gpetters94 gpetters94 force-pushed the convolution_transpose branch 2 times, most recently from 5906761 to 168fe6d Compare August 15, 2022 05:31
@ZihengJiang
Copy link
Collaborator

The PR on the PyTorch side is accepted. Can we merge this PR now? @vivekkhandelwal1 @silvasean

facebook-github-bot pushed a commit to pytorch/pytorch that referenced this pull request Aug 15, 2022
@gpetters94 @facebook-github-bot
Add shape functions for conv_transpose2d.input and convolution (#80860)…
1880fb1 
… (#80860)

Summary:
As silvasean requested in [this issue](llvm/torch-mlir#917 (comment)) here is the shape code from Torch-MLIR for conv_transpose2d.input and convolution (updated for the transposed case).

Pull Request resolved: #80860
Approved by: https://github.com/Gamrix

Test Plan:
contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/a7e7fbab82d0badaad116eab5a552f6246c3a5ac

Original Phabricator Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Reviewed By: atalman

Differential Revision: D37674534

Pulled By: atalman

fbshipit-source-id: 0747b91d5c43065b86e3d1ab1b4c06b9dc046b9f
@gpetters94
Copy link
Collaborator Author

gpetters94 commented Aug 16, 2022 •
edited
Loading

@ZihengJiang @silvasean I made a small change in the shape logic (I forgot that two of the dims needed transposing) so I need to make another PR in Pytorch that will hopefully not take long to get upstreamed this time. Otherwise this is passing all the tests.

@vivekkhandelwal1
Copy link
Collaborator

The PR on the PyTorch side is accepted. Can we merge this PR now? @vivekkhandelwal1 @silvasean

Sure. Once this PR (pytorch/pytorch#83557) is merged, we can get this patch merged.

@gpetters94
Copy link
Collaborator Author

@vivekkhandelwal1 @silvasean The upstream shape code is merged, so this one should be good to go.

vivekkhandelwal1
vivekkhandelwal1 requested changes Aug 23, 2022
View reviewed changes
Copy link
Collaborator

@vivekkhandelwal1 vivekkhandelwal1 left a comment

Choose a reason for hiding this comment

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

@gpetters94 Could you please resolve the conflicts?

@gpetters94
Copy link
Collaborator Author

@vivekkhandelwal1 Done.

vivekkhandelwal1
vivekkhandelwal1 approved these changes Aug 24, 2022
View reviewed changes
Copy link
Collaborator

@vivekkhandelwal1 vivekkhandelwal1 left a comment

Choose a reason for hiding this comment

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

LGTM

@gpetters94 gpetters94 merged commit f012279 into llvm:main Aug 24, 2022
@tanyokwok tanyokwok mentioned this pull request Sep 21, 2022
Closed
qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
Suppress warnings in third_party subprojects. (llvm#917)
3d7afed 
Signed-off-by: Ettore Tiotto <etiotto@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramiro050 ramiro050 ramiro050 approved these changes

@silvasean silvasean silvasean left review comments

@vivekkhandelwal1 vivekkhandelwal1 vivekkhandelwal1 approved these changes

@cathyzhyi cathyzhyi Awaiting requested review from cathyzhyi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gpetters94 @dan-garvey @ZihengJiang @silvasean @vivekkhandelwal1 @ramiro050