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.

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

Implement Expand/Collapse Functionality for Aten.View (Re-Opened_ #1353

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

JakopinA
Copy link
Contributor

@JakopinA JakopinA commented Sep 8, 2022

Second PR for this functionality after being opened with #1309 and closed with #1347 . Issues raised were with the exclusive use of static sizes and lack of unit tests.

@silvasean
Copy link
Contributor

What is the relationship between this PR and #1357 -- Are they independent?

cc @gpetters94

@JakopinA JakopinA force-pushed the jakopina-view2 branch 2 times, most recently from 7a9e1f9 to 5602c25 Compare September 21, 2022 13:55
@JakopinA
Copy link
Contributor Author

What is the relationship between this PR and #1357 -- Are they independent?

cc @gpetters94

I've reached out to him to try to figure something out. I will say that this commit has support for inferring a dynamic dim along with a couple of unit tests, so it should be as ready to go as I could get it.

@qedawkins
Copy link
Collaborator

I added a comment on #1357, but I believe this is the correct approach to the view cases that are neither expand nor collapse. It would be much clearer with unit tests and/or samples for what the IR looks like with each PR though. Dynamic support for [x, y] -> [y, x] is still blocked by tensor.expand_shape in this PR as well. This is also shown in #1390.

test/Conversion/TorchToLinalg/flatten.mlir Outdated Show resolved Hide resolved
test/Conversion/TorchToLinalg/flatten.mlir Outdated Show resolved Hide resolved
@JakopinA JakopinA force-pushed the jakopina-view2 branch 5 times, most recently from 4376d2f to 0a733f4 Compare September 23, 2022 15:18
@JakopinA
Copy link
Contributor Author

@silvasean Anything here you need me to do? Everything should be working now.

test/Conversion/TorchToLinalg/view.mlir Outdated Show resolved Hide resolved
test/Conversion/TorchToLinalg/view.mlir Outdated Show resolved Hide resolved
test/Conversion/TorchToLinalg/view.mlir Outdated Show resolved Hide resolved
test/Conversion/TorchToLinalg/view.mlir Outdated Show resolved Hide resolved
test/Conversion/TorchToLinalg/view.mlir Outdated Show resolved Hide resolved
test/Conversion/TorchToLinalg/view.mlir Show resolved Hide resolved
@JakopinA JakopinA force-pushed the jakopina-view2 branch 4 times, most recently from 7150ddd to 1e785eb Compare September 26, 2022 17:05
@JakopinA
Copy link
Contributor Author

Alright, I've removed a lot of the redundant e2e tests and added two more important unit tests. Should be good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants