Skip to content

Use output shape when lowering Unique to krnl #2820

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

Merged
merged 15 commits into from
May 13, 2024

Conversation

chentong319
Copy link
Collaborator

This issue is discovered by #2799 . It may not be considered a bug in lowering. The issue is triggered by two imperfect components: onnx backend test and buffer deallocation.

  1. onnx backend test
    The output shape of Unique OP depends on the content of the input tensor. But the onnx backend test creates the onnx model based on the chosen input. Therefore, the output shape is static. I opened an issue in onnx.

  2. Buffer deallocation
    Due to the mismatch of the generated and specified type of the outputs UniqueOp, the existing lowering of Unique created some unrealized_conversion_cast ops. The deallocation pass in the new llvm can not handle the conversion for returned tensors correctly.

Anyway, I try to fix the problem from onnx-mlir with this PR. I use the type of the outputs for allocation if the type is static.

@negiyas
Copy link
Contributor

negiyas commented May 10, 2024

@chentong319 Your fixes look good to me! Thank you so much for fixing lowering Unique to krnl.

@chentong319
Copy link
Collaborator Author

@chentong319 Your fixes look good to me! Thank you so much for fixing lowering Unique to krnl.

Could you approve the PR?

@hamptonm1
Copy link
Collaborator

@chentong319 Your fixes look good to me! Thank you so much for fixing lowering Unique to krnl.

Could you approve the PR?

@chentong319 Since @negiyas said this looks good, I will make sure to merge this in for you :)

Copy link
Collaborator

@hamptonm1 hamptonm1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@hamptonm1 hamptonm1 merged commit 6133173 into onnx:main May 13, 2024
@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #14814 [push] Use output shape when lo... started at 14:48

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #13839 [push] Use output shape when lo... started at 15:01

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #14809 [push] Use output shape when lo... started at 13:48

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #14809 [push] Use output shape when lo... passed after 1 hr 14 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #14814 [push] Use output shape when lo... passed after 1 hr 46 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #13839 [push] Use output shape when lo... passed after 2 hr 0 min

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.

4 participants