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 lowering of torch.aten.all.dim #2873

Merged
merged 4 commits into from
Feb 7, 2024

Conversation

mmakevic
Copy link
Contributor

@mmakevic mmakevic commented Feb 6, 2024

Lowering of torch.aten.all.dim to linalg.

Per PyTorch documentation:

This function matches the behaviour of NumPy in returning output of dtype bool for all supported dtypes except uint8. For uint8 the dtype of output is uint8 itself.

Since there is no support for ui8 in torch-mlir currently (#1384 (comment)) implementation returns failure for that case.

Initial lowering torch.aten.all.dim

Handle ui8 case
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.

Looks good but I think the tests can be more targeted (smaller tensor, set values by hand). Can you test the empty tensor case returns 'True' ?

@mmakevic
Copy link
Contributor Author

mmakevic commented Feb 6, 2024

@newling done :)

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.

Thanks for updating the tests, just the isa<.> comment remaining

lib/Conversion/TorchToLinalg/Reduction.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/Reduction.cpp Outdated Show resolved Hide resolved
@mmakevic mmakevic requested a review from newling February 6, 2024 17:19
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.

Just the little comment about the extra brackets and formatting, but approving now as it looks good to me otherwise.

lib/Conversion/TorchToLinalg/Reduction.cpp Outdated Show resolved Hide resolved
@mmakevic
Copy link
Contributor Author

mmakevic commented Feb 7, 2024

@newling can you merge this PR? I do not have write access :)

@newling
Copy link
Collaborator

newling commented Feb 7, 2024

My view of the PR:

image

@mmakevic Let me know if this looks correct ("authored by" email look ok?) and I'll merge it if no one else comments in the next day.

@mmakevic
Copy link
Contributor Author

mmakevic commented Feb 7, 2024

@newling Yeah, everything looks okay. Thanks!

@newling newling merged commit 32dbf99 into llvm:main Feb 7, 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