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

[torch] Add OnnxToTorch lowering for Onnx.STFT op #3492

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

vinayakdsci
Copy link
Contributor

Adds OnnxToTorch lowering for Onnx.STFT op.

@vinayakdsci
Copy link
Contributor Author

cc @vivekkhandelwal1.

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.

Overall, there are too many control flow paths many of which can be combined with slight modifications, please do it wherever possible to make the flow simpler.

lib/Conversion/TorchOnnxToTorch/DefaultDomainQtoZ.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchOnnxToTorch/DefaultDomainQtoZ.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchOnnxToTorch/DefaultDomainQtoZ.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchOnnxToTorch/DefaultDomainQtoZ.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchOnnxToTorch/DefaultDomainQtoZ.cpp Outdated Show resolved Hide resolved
@vinayakdsci
Copy link
Contributor Author

Hi @vivekkhandelwal1, sorry for so many control flow paths. I have fixed all the readabiity issues, and in the AtenSTFTOp, now there is only one condition checking if onesided is true or false, which I could not avoid.

I apologize for the rebase, it was necessary to resolve the merge conflicts. Thanks for the review!

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.

Please address the minor comments, then the PR is ready to go.

lib/Conversion/TorchOnnxToTorch/DefaultDomainQtoZ.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchOnnxToTorch/DefaultDomainQtoZ.cpp Outdated Show resolved Hide resolved
@vinayakdsci
Copy link
Contributor Author

@vivekkhandelwal1 all comments have been addressed in the latest commit. Thanks for the review!

@vivekkhandelwal1 vivekkhandelwal1 merged commit 0234040 into llvm:main Jun 25, 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.

None yet

2 participants