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 E2E support for AtenEmbeddingBagPaddingIdxOp #1066

Merged
merged 1 commit into from
Aug 1, 2022
Merged

Add E2E support for AtenEmbeddingBagPaddingIdxOp #1066

merged 1 commit into from
Aug 1, 2022

Conversation

vidsinghal
Copy link
Collaborator

No description provided.

@vidsinghal vidsinghal marked this pull request as draft July 16, 2022 19:52
ramiro050
ramiro050 requested changes Jul 18, 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/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
@vidsinghal vidsinghal mentioned this pull request Jul 21, 2022
Open
@vidsinghal vidsinghal mentioned this pull request Jul 22, 2022
Closed
@vidsinghal vidsinghal changed the title WIP: start implementing AtenEmbeddingBagPaddingIdxOp WIP: E2E support for AtenEmbeddingBagPaddingIdxOp Jul 22, 2022
ramiro050
ramiro050 requested changes Jul 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.

Thanks for working on this! Below I have a few comments. They are mostly stylistic to help with readability. Also, don't forget to run git clang-format on your commit to make sure the indentation style matches the rest of the codebase

lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Dialect/Torch/Transforms/RefineTypes.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
python/torch_mlir/dialects/torch/importer/jit_ir/build_tools/shape_lib_gen.py Outdated Show resolved Hide resolved
python/torch_mlir_e2e_test/test_suite/basic.py Outdated Show resolved Hide resolved
@vidsinghal
Copy link
Collaborator Author

vidsinghal commented Jul 25, 2022 •
edited
Loading

Thanks for working on this! Below I have a few comments. They are mostly stylistic to help with readability. Also, don't forget to run git clang-format on your commit to make sure the indentation style matches the rest of the codebase

Hi thanks, I have worked on and resolved all the current comments. I have replied on some comments with more updates.

@vidsinghal vidsinghal marked this pull request as ready for review July 26, 2022 21:53
ramiro050
ramiro050 requested changes Jul 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.

A few more comments

lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Dialect/Torch/Transforms/RefineTypes.cpp Outdated Show resolved Hide resolved
python/torch_mlir/dialects/torch/importer/jit_ir/build_tools/shape_lib_gen.py Show resolved Hide resolved
@vidsinghal vidsinghal force-pushed the embedding_bag_padding_idx branch 2 times, most recently from bf6f5c8 to 4274c88 Compare July 28, 2022 16:53
@vidsinghal
Copy link
Collaborator Author

Completed prior requested changes.

ramiro050
ramiro050 requested changes Jul 28, 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

lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
ramiro050
ramiro050 approved these changes Jul 29, 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! Just three small change requests

lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/IndirectDataMovement.cpp Outdated Show resolved Hide resolved
lib/Dialect/Torch/Transforms/DecomposeComplexOps.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToMhlo/GatherOp.cpp Outdated Show resolved Hide resolved
@vidsinghal
Copy link
Collaborator Author

LGTM! Just three small change requests

Completed.

@vidsinghal vidsinghal changed the title WIP: E2E support for AtenEmbeddingBagPaddingIdxOp Add E2E support for AtenEmbeddingBagPaddingIdxOp Aug 1, 2022
@vidsinghal vidsinghal merged commit ed13ebf into llvm:main Aug 1, 2022
@vidsinghal vidsinghal mentioned this pull request Aug 8, 2022
Open
@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
Document Mac OSX workaround for check-onnx-backend. (llvm#1066)
b14acff 
Signed-off-by: Ettore Tiotto <etiotto@ca.ibm.com>
@silvasean silvasean mentioned this pull request Nov 18, 2022
Open
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 Awaiting requested review from silvasean

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

Successfully merging this pull request may close these issues.
2 participants
@vidsinghal @ramiro050