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

[MLIR][ONNX] Add OnnxToTorch support for ReduceSumSquare Op #3188

Merged
merged 21 commits into from
May 2, 2024

Conversation

archana-ramalingam
Copy link
Contributor

This commit adds the OnnxToTorch support for ReduceSumSquare ops.

@archana-ramalingam archana-ramalingam changed the title [MLIR][ONNX] Add OnnxToTorch support for ReduceSumSquare Ops [MLIR][ONNX] Add OnnxToTorch support for ReduceSumSquare Op Apr 19, 2024
@vivekkhandelwal1
Copy link
Collaborator

@archana-ramalingam Please clang-format your code.

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.

Either you can remove this test if its support is not present or add the valid checks. A lit test without any failure or check is not useful. The rest LGTM.

test/Conversion/TorchOnnxToTorch/simple_ops_q_to_z.mlir Outdated Show resolved Hide resolved
test/Conversion/TorchOnnxToTorch/simple_ops_q_to_z.mlir Outdated Show resolved Hide resolved
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.

Apart from a small change, everything else looks fine.

test/Conversion/TorchOnnxToTorch/simple_ops_q_to_z.mlir Outdated Show resolved Hide resolved
@vivekkhandelwal1
Copy link
Collaborator

@archana-ramalingam please rebase your branch.

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.

@archana-ramalingam, It seems the rebasing is not done properly, I see a lot of code changes not done by you in this PR. Please correct this.

@archana-ramalingam
Copy link
Contributor Author

@archana-ramalingam, It seems the rebasing is not done properly, I see a lot of code changes not done by you in this PR. Please correct this.

The latest llvm:main wasn't merged into this branch. Now it's fixed.

@vivekkhandelwal1
Copy link
Collaborator

@archana-ramalingam, It seems the rebasing is not done properly, I see a lot of code changes not done by you in this PR. Please correct this.

The latest llvm:main wasn't merged into this branch. Now it's fixed.

The PR is still not rebased properly. I see ReduceLogSum changes overwritten by this patch.

@archana-ramalingam
Copy link
Contributor Author

archana-ramalingam commented Apr 30, 2024

@archana-ramalingam, It seems the rebasing is not done properly, I see a lot of code changes not done by you in this PR. Please correct this.

The latest llvm:main wasn't merged into this branch. Now it's fixed.

The PR is still not rebased properly. I see ReduceLogSum changes overwritten by this patch.

For this particular conflict, I moved reducelogsum to a different position as both reducesumsquare and reducelogsum were written in the same lines. As you can see I am not adding reducelogsum only moving it alphabetically.

@vivekkhandelwal1 vivekkhandelwal1 merged commit a46fe2c into llvm:main May 2, 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.

2 participants