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 linalg lowering of diag_embed torch op #2885

Merged
merged 12 commits into from
Mar 22, 2024

Conversation

schnkmwt
Copy link
Contributor

@schnkmwt schnkmwt commented Feb 7, 2024

This PR adds lowering of diag_embed to linalg dilect.
Tracked in nod-ai/SHARK-ModelDev#288

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! Looks mostly good, I just have a few comments

include/torch-mlir/Conversion/Utils/Utils.h Outdated Show resolved Hide resolved
lib/Conversion/Utils/Utils.cpp Outdated Show resolved Hide resolved
lib/Conversion/Utils/Utils.cpp Outdated Show resolved Hide resolved
lib/Conversion/Utils/Utils.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@newling newling left a comment

Choose a reason for hiding this comment

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

I'm still trying to understand how the indexing works. Test coverage looks good, and the comments are useful. Nice one!

lib/Conversion/TorchToLinalg/DataMovement.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/DataMovement.cpp Show resolved Hide resolved
@ramiro050
Copy link
Collaborator

Not sure if you're manually making the lint fixes, but you can use git clang-format to automatically format the changes.

git clang-format main

formats your entire branch. If you don't have git clang-format, you should be able to do inside torch-mlir/

./externals/llvm-project/clang/tools/clang-format/git-clang-format main

@schnkmwt
Copy link
Contributor Author

Not sure if you're manually making the lint fixes, but you can use git clang-format to automatically format the changes.

git clang-format main

formats your entire branch. If you don't have git clang-format, you should be able to do inside torch-mlir/

./externals/llvm-project/clang/tools/clang-format/git-clang-format main

For some reason git clang-format main did not work for me. I guess it only works on most recent commit. But the git-clang-format under ./externals worked like a charm! Thanks for the suggestion.

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 think you need to XFAIL your tests on the ONNX backend

@schnkmwt
Copy link
Contributor Author

LGTM, I think you need to XFAIL your tests on the ONNX backend

Yes just pushed that. Got help from discord!

@schnkmwt schnkmwt requested a review from newling March 22, 2024 02:10
@newling newling merged commit 1fcbfa8 into llvm:main Mar 22, 2024
3 checks passed
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