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

Allow expanding and collapsing in aten::view #1082

Merged
merged 1 commit into from
Jul 20, 2022

Conversation

qedawkins
Copy link
Collaborator

  • Supports cases where the view op expands and collapses dims
    simultaneously. This does not handle the case where it is neither
    expanding nor collapsing (e.g. [2, 3] -> [3, 2])
  • Additionally fixes a previous bug with adding 1-sized dims on both
    sides of a tensor with aten.view

@qedawkins
Copy link
Collaborator Author

Also @silvasean is there a place I can request write access? This seemed like the best channel to ask in.

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.

Thanks for working on this! Adding functionality to this op is very tricky. Below are some preliminary comments. I will do a more detailed pass in a bit

lib/Conversion/TorchToLinalg/DataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/DataMovement.cpp Outdated Show resolved Hide resolved
@qedawkins qedawkins force-pushed the view-expand-collapse branch from 0250a70 to c872f05 Compare July 19, 2022 16:30
@silvasean
Copy link
Contributor

Also @silvasean is there a place I can request write access? This seemed like the best channel to ask in.

Added you!

@qedawkins qedawkins requested a review from ramiro050 July 20, 2022 14:22
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.

I just have a few small comments

lib/Conversion/TorchToLinalg/DataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/DataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/DataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/DataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/DataMovement.cpp Outdated Show resolved Hide resolved
@qedawkins qedawkins force-pushed the view-expand-collapse branch from c872f05 to 82f0574 Compare July 20, 2022 20:02
 - Supports cases where the view op expands and collapses dims
simulataneously. This does not handle the case where it is neither
expanding nor collapsing (e.g. [2, 3] -> [3, 2])
 - Additionally fixes a previous bug with adding 1-sized dims on both
sides of a tensor with aten.view
@qedawkins qedawkins force-pushed the view-expand-collapse branch from 82f0574 to 244ba2e Compare July 20, 2022 20:05
@qedawkins
Copy link
Collaborator Author

Thanks for the comments! Let me know if it looks like there are still issues.

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!

@qedawkins qedawkins merged commit 647e75e into llvm:main Jul 20, 2022
@qedawkins qedawkins deleted the view-expand-collapse branch July 20, 2022 21:36
gpetters94 pushed a commit to gpetters94/mlir-npcomp that referenced this pull request Jul 27, 2022
- Supports cases where the view op expands and collapses dims
simulataneously. This does not handle the case where it is neither
expanding nor collapsing (e.g. [2, 3] -> [3, 2])
 - Additionally fixes a previous bug with adding 1-sized dims on both
sides of a tensor with aten.view
qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
* minor edit for the docs/mnist_example

Signed-off-by: Alexandre Eichenberger <[email protected]>

* additional output

Signed-off-by: Alexandre Eichenberger <[email protected]>

* responding to suggestions

Signed-off-by: Alexandre Eichenberger <[email protected]>

* added recommended -O3 option

Signed-off-by: Alexandre Eichenberger <[email protected]>

Co-authored-by: Tung D. Le <[email protected]>
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.

3 participants