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

DPS version of matmul in TTNN #15038

Closed
azecevicTT opened this issue Nov 14, 2024 · 9 comments
Closed

DPS version of matmul in TTNN #15038

azecevicTT opened this issue Nov 14, 2024 · 9 comments
Assignees
Labels

Comments

@azecevicTT
Copy link

Is there a reason why matmul (and linear) aren't modeled as DPS ops in TTNN? We model them as DPS ops in TTNN MLIR dialect, is there a plan to make them DPS ops in the future or should we change them to non DPS to reflect TTNN state.

@sdjordjevicTT
Copy link
Contributor

Hi @bbradelTT, we discovered this during our onboarding in the TT-MLIR compiler. Can you give us some feedback on whether this inconsistency in the matmul op is a limitation or just needs to be implemented from your side?

@bbradelTT
Copy link
Contributor

It just needs to be implemented from our side. The underlying device op has output tensors passed in to it that are created via operation::get_workers_for_op_output. It should be straightforward to expose that via an optional parameter. Having said that, the output tensor would need to be the right format, otherwise we would need to fail in validation.

How high of a priority would this work be for you and when would you need it by?

@sdjordjevicTT
Copy link
Contributor

sdjordjevicTT commented Nov 14, 2024

Thanks for the quick reply @bbradelTT! It isn't critical for now, but it would be great to have support for DPS matmul sometime in the near future, hence I will mark this as P1 if that sounds ok with you?

@bbradelTT
Copy link
Contributor

@sdjordjevicTT sounds good. @asandhupatlaTT will work on it.

@sdjordjevicTT
Copy link
Contributor

Great, thanks! Let's keep this issue open to track the progress.

@bbradelTT
Copy link
Contributor

@asandhupatlaTT
Please try to see if it makes it easier if we convert to compute_output_specs for struct Matmul.
See ttnn/cpp/ttnn/operations/reduction/generic/device/reduce_op.hpp for an example.

@bbradelTT
Copy link
Contributor

@sdjordjevicTT @azecevicTT which TTNN ops are DPS ops already? Using those as guides will help speed up development.

@sdjordjevicTT
Copy link
Contributor

All the ops that define the optional output tensor in their APIs should be DPS.

For example, you can take a look at element-wise ops API definition:
https://github.com/tenstorrent/tt-metal/blob/8901511f737c15d181629ce4de93fae4a11da3b8/ttnn/cpp/ttnn/operations/eltwise/binary/binary.hpp#L29C1-L29C69

Or take a look at the embedding op and its optional output tensor:
https://github.com/tenstorrent/tt-metal/blob/8901511f737c15d181629ce4de93fae4a11da3b8/ttnn/cpp/ttnn/operations/embedding/embedding.hpp#L26C1-L26C70

asandhupatlaTT added a commit that referenced this issue Dec 19, 2024
### Ticket
Link to Github Issue: 
#15038

### Problem description
Provide context for the problem.
Dps ops are not being generated in MLIR for matmul op.

### What's changed
Describe the approach used to solve the problem.
Summarize the changes made and its impact.
Enabled std::optional for ourput tensor as seen for other dps supported
ops such as Embedding & element-wise ops

### Checklist
- [x] Post commit CI passes :
https://github.com/tenstorrent/tt-metal/actions/runs/12204499266
- [x] Blackhole Post commit (if applicable) :
https://github.com/tenstorrent/tt-metal/actions/runs/12204501225
- [x] Model regression CI testing passes (if applicable) :
https://github.com/tenstorrent/tt-metal/actions/runs/12204503539
- [x] Device performance regression CI testing passes (if applicable) :
https://github.com/tenstorrent/tt-metal/actions/runs/12204505818
- [x] New/Existing tests provide coverage for changes

---------

Signed-off-by: Amruth Sandhupatla <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@asandhupatlaTT
Copy link
Contributor

fixed at #15285

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants