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.

Sign up for GitHub

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

Jump to bottom

Add conversions for stablehlo ceil, sine and cosine Ops #939

Merged
merged 7 commits into from
Oct 30, 2024
Merged

Add conversions for stablehlo ceil, sine and cosine Ops #939

merged 7 commits into from
Oct 30, 2024

Conversation

uazizTT
Copy link
Contributor

@uazizTT uazizTT commented Oct 17, 2024

Added tests and refactored unary ops.

@uazizTT uazizTT changed the title Add conversions for stablehlo ceil, sin and cosine Ops Add conversions for stablehlo ceil, sine and cosine Ops Oct 17, 2024
@jnie-TT
Copy link
Contributor

jnie-TT commented Oct 18, 2024 •
edited by uazizTT
Loading

@uazizTT this look great! If these new ops are expected to work on silicon, can you add tests for them under test/ttmlir/Silicon/TTNN

Added Silicon tests.

@uazizTT uazizTT force-pushed the uaziz/stablehlo-convert-ops branch 2 times, most recently from 813f13f to 3e5e976 Compare October 22, 2024 04:51
@uazizTT uazizTT requested a review from tapspatel October 22, 2024 16:25
@uazizTT uazizTT force-pushed the uaziz/stablehlo-convert-ops branch 4 times, most recently from e700b84 to 9525a8d Compare October 29, 2024 18:15
@uazizTT uazizTT force-pushed the uaziz/stablehlo-convert-ops branch from 9525a8d to 4caae05 Compare October 29, 2024 19:04
tapspatel
tapspatel requested changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@tapspatel tapspatel left a comment

Choose a reason for hiding this comment

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

Can you add a ceil, concat and cosine test under Silicon/TTNN/perf_unit?

@uazizTT uazizTT force-pushed the uaziz/stablehlo-convert-ops branch from 4caae05 to 4fe5125 Compare October 30, 2024 00:12
tapspatel
tapspatel approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@tapspatel tapspatel left a comment

Choose a reason for hiding this comment

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

great! Looks good, thanks!

@uazizTT uazizTT force-pushed the uaziz/stablehlo-convert-ops branch 2 times, most recently from ed4c0b3 to 6434de2 Compare October 30, 2024 04:00
@uazizTT uazizTT force-pushed the uaziz/stablehlo-convert-ops branch from 6434de2 to dea22dd Compare October 30, 2024 12:59
@uazizTT uazizTT merged commit caa7b90 into main Oct 30, 2024
13 checks passed
@mrakitaTT
Copy link
Contributor

@uazizTT I don't think it's okay to merge PRs with unresolved comments without any discourse. I didn't want to block you so I didn't put "Requests changes" status, but I see no other way to prevent these situations in the future. I know I might be too pedantic on PRs and it is totally fine to disagree with my comments and explain why you won't fix them, or say "I will follow up with another PR", but I don't think it's okay to just ignore comments.

@uazizTT
Copy link
Contributor Author

uazizTT commented Oct 30, 2024 •
edited
Loading

@uazizTT I don't think it's okay to merge PRs with unresolved comments without any discourse. I didn't want to block you so I didn't put "Requests changes" status, but I see no other way to prevent these situations in the future. I know I might be too pedantic on PRs and it is totally fine to disagree with my comments and explain why you won't fix them, or say "I will follow up with another PR", but I don't think it's okay to just ignore comments.

Hey @mrakitaTT , I thought I covered all the comments and got a thumbs up from everyone before merging, but reading your last comment again, it seems you were asking to break down test/ttmlir/Silicon/TTNN/simple_eltwise.mlir file as well. I didn't disagree with your comment as I have already split the tests, sorry it wasn't clear to me.

Anyways that is outside the scope of this PR, so let me create a new issue for it to decompose both test/ttmlir/Silicon/TTNN/simple_eltwise.mlir and test/ttmlir/Silicon/TTMetal/simple_eltwise.mlir files to covered in a subsequent PR.

@uazizTT
Copy link
Contributor Author

uazizTT commented Oct 30, 2024 •
edited
Loading

Created an issue to split simple_eltwise.mlir files, linking it here for reference.
#1118

@mrakitaTT
Copy link
Contributor

@uazizTT No problem, let's just please try in the future to have proper discourse in comments and to resolve all comments you think you've addressed. As I said it is totally fine to disagree with the comment or "won't fix" it, the important thing is to have a conversation about it. In this case there was no conversation and the comment was left unresolved, so I couldn't know what you've assumed. When you mark comment as resolved I can at least know that you think you've addressed it so I can ping you about it.

The comment in question here wasn't about splitting the test file, you've marked that one as resolved and I didn't object. The comment in question here was about improving FILECHECK statements in conversion tests.

@uazizTT uazizTT deleted the uaziz/stablehlo-convert-ops branch October 30, 2024 18:55
@uazizTT
Copy link
Contributor Author

uazizTT commented Oct 30, 2024

The comment in question here wasn't about splitting the test file, you've marked that one as resolved and I didn't object. The comment in question here was about improving FILECHECK statements in conversion tests.

OK will cover them in subsequent PR.

@uazizTT uazizTT mentioned this pull request Oct 31, 2024
Merged
@uazizTT
Copy link
Contributor Author

uazizTT commented Oct 31, 2024

The comment in question here wasn't about splitting the test file, you've marked that one as resolved and I didn't object. The comment in question here was about improving FILECHECK statements in conversion tests.

OK will cover them in subsequent PR.

Updated the tests in this PR #1122.

@mrakitaTT
Copy link
Contributor

Updated the tests in this PR #1122.

Great, thank you Usman!

@staylorTT staylorTT added the Priority Model Op Bringup Label for priority model ops label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrakitaTT mrakitaTT mrakitaTT left review comments

@nsmithtt nsmithtt nsmithtt approved these changes

@mmanzoorTT mmanzoorTT mmanzoorTT approved these changes

@jnie-TT jnie-TT jnie-TT approved these changes

@svuckovicTT svuckovicTT svuckovicTT approved these changes

@tapspatel tapspatel tapspatel approved these changes

@AleksKnezevic AleksKnezevic AleksKnezevic approved these changes

@kmabeeTT kmabeeTT Awaiting requested review from kmabeeTT kmabeeTT is a code owner

@pilkicTT pilkicTT Awaiting requested review from pilkicTT pilkicTT is a code owner

@rpavlovicTT rpavlovicTT Awaiting requested review from rpavlovicTT

@sdjordjevicTT sdjordjevicTT Awaiting requested review from sdjordjevicTT sdjordjevicTT is a code owner

@mtopalovicTT mtopalovicTT Awaiting requested review from mtopalovicTT mtopalovicTT is a code owner

@jserbedzijaTT jserbedzijaTT Awaiting requested review from jserbedzijaTT jserbedzijaTT is a code owner

@nobradovictt nobradovictt Awaiting requested review from nobradovictt

Assignees
No one assigned
Labels
Priority Model Op Bringup Label for priority model ops
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@uazizTT @jnie-TT @mrakitaTT @AleksKnezevic @nsmithtt @tapspatel @svuckovicTT @mmanzoorTT @staylorTT