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

Implement conversion for stablehlo.select and add Where Op #852

Merged
merged 12 commits into from
Nov 12, 2024
Merged

Implement conversion for stablehlo.select and add Where Op #852

merged 12 commits into from
Nov 12, 2024

Conversation

uazizTT
Copy link
Contributor

@uazizTT uazizTT commented Oct 1, 2024

Fixes #639
Fixes #640

@nvukobratTT
Copy link
Contributor

nvukobratTT commented Oct 2, 2024 •
edited
Loading

FYI @dgolubovicTT seems like a good op we should map from FFE as well :))

@uazizTT uazizTT force-pushed the uaziz/stablehlo-select-op branch from 9c3677d to 49f962e Compare October 15, 2024 01:06
@uazizTT uazizTT force-pushed the uaziz/stablehlo-select-op branch from 49f962e to 0bf67eb Compare October 15, 2024 01:36
@uazizTT uazizTT force-pushed the uaziz/stablehlo-select-op branch from df6a7fd to 4b8d8d7 Compare October 22, 2024 17:43
@uazizTT uazizTT self-assigned this Oct 22, 2024
@uazizTT uazizTT added this to the [Third Party] HLO + XLA milestone Oct 22, 2024
mrakitaTT
mrakitaTT requested changes Oct 28, 2024
View reviewed changes
include/ttmlir/Dialect/TTIR/IR/TTIROps.td Outdated Show resolved Hide resolved
include/ttmlir/Dialect/TTIR/IR/TTIROps.td Outdated Show resolved Hide resolved
include/ttmlir/Dialect/TTNN/IR/TTNNOps.td Outdated Show resolved Hide resolved
runtime/lib/ttnn/operations/eltwise/where/where.cpp Outdated Show resolved Hide resolved
runtime/lib/ttnn/operations/eltwise/where/where.cpp Outdated Show resolved Hide resolved
runtime/lib/ttnn/operations/eltwise/where/where.h Outdated Show resolved Hide resolved
test/ttmlir/Conversion/StableHLOToTTIR/select_op.mlir Outdated Show resolved Hide resolved
@kmitrovicTT
Copy link
Contributor

@uazizTT does this solve #1009? Is so, please link it. Also, excel row for select is marked blue, no PR and assignee set to it.

@svuckovicTT
Copy link
Contributor

@kmitrovicTT I wasn't aware there was another place where we tracked ops... @sdjordjevicTT @nvukobratTT can you folks pick one to be the source of truth (either github issues or the spreadsheet) to avoid issues of multiple people implementing the same things?

@nvukobratTT
Copy link
Contributor

@kmitrovicTT I wasn't aware there was another place where we tracked ops... @sdjordjevicTT @nvukobratTT can you folks pick one to be the source of truth (either github issues or the spreadsheet) to avoid issues of multiple people implementing the same things?

From my side, GitHub should be the main source of truth.

It's available for everyone, and it's regularly updated. Other docs, sheets, etc. should be on a team/personal level, and shouldn't be interpreted for the latest state of op support.

Other thoughts @sdjordjevicTT @AleksKnezevic?

@mrakitaTT
Copy link
Contributor

@svuckovicTT @nvukobratTT There was a parallel discussion about this today and we've agreed with this: https://tenstorrent.slack.com/archives/C07A30HRX7C/p1730460299618899 So everyone working on frontends can have their own GH tasks about ops bringup, but you also have to open separate task under the MLIR Dialect / Passes component in case that you have to make changes in tt-mlir TTIR/TTNN dialect/passes for that specific op. That way we won't have clashes in op bringup in TTIR/TTNN.

@uazizTT uazizTT force-pushed the uaziz/stablehlo-select-op branch from 4b8d8d7 to 47d11bc Compare November 5, 2024 00:43
@sdjordjevicTT sdjordjevicTT mentioned this pull request Nov 5, 2024
Merged
@uazizTT uazizTT force-pushed the uaziz/stablehlo-select-op branch from 47d11bc to bb3f739 Compare November 5, 2024 16:03
@uazizTT uazizTT force-pushed the uaziz/stablehlo-select-op branch 4 times, most recently from 7114e54 to fb38c4f Compare November 6, 2024 23:36
svuckovicTT
svuckovicTT approved these changes Nov 8, 2024
View reviewed changes
lib/Conversion/TTIRToTTNN/TTIRToTTNN.cpp Outdated Show resolved Hide resolved
lib/Conversion/TTIRToTTNN/TTIRToTTNN.cpp Outdated Show resolved Hide resolved
@uazizTT uazizTT force-pushed the uaziz/stablehlo-select-op branch 2 times, most recently from da57af0 to d8fce32 Compare November 12, 2024 03:32
uazizTT and others added 11 commits November 12, 2024 14:03
@uazizTT uazizTT force-pushed the uaziz/stablehlo-select-op branch 3 times, most recently from 635bc70 to 5fa793f Compare November 12, 2024 16:37
@uazizTT uazizTT requested a review from jnie-TT November 12, 2024 17:38
jnie-TT
jnie-TT approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@jnie-TT jnie-TT left a comment

Choose a reason for hiding this comment

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

This is awesome. I forgot that in the flatbuffer the inputs is actually an array of variable-number elements, so we can integrate this into eltwiseop directly instead of creating a ternary op type, even better than the original plan! Thanks for looking at this!

@uazizTT uazizTT force-pushed the uaziz/stablehlo-select-op branch 2 times, most recently from 6d81371 to 2c92934 Compare November 12, 2024 19:59
tapspatel
tapspatel approved these changes Nov 12, 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.

looks good! only change is can you add a test under Silicon/TTNN/perf_unit for where op? otherwise looks good

@uazizTT uazizTT force-pushed the uaziz/stablehlo-select-op branch from 5fa793f to 839c875 Compare November 12, 2024 20:59
@uazizTT uazizTT merged commit 430b036 into main Nov 12, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmanzoorTT mmanzoorTT mmanzoorTT left review comments

@mrakitaTT mrakitaTT mrakitaTT approved these changes

@nvukobratTT nvukobratTT nvukobratTT left review comments

@AleksKnezevic AleksKnezevic AleksKnezevic approved these changes

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

@svuckovicTT svuckovicTT svuckovicTT approved these changes

@tapspatel tapspatel tapspatel 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

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

@rpavlovicTT rpavlovicTT Awaiting requested review from rpavlovicTT

@nobradovictt nobradovictt Awaiting requested review from nobradovictt

@nsmithtt nsmithtt Awaiting requested review from nsmithtt nsmithtt 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

Assignees

@uazizTT uazizTT

Labels
None yet
Projects
None yet
Milestone
[Third Party] HLO + XLA
Development

Successfully merging this pull request may close these issues.

[MNIST] Implement conversion of stablehlo.select OP to TTIR dialect [Ops] Add TTIR where OP