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

enable dps ops for matmul #15285

Merged
merged 28 commits into from
Dec 19, 2024
Merged

enable dps ops for matmul #15285

merged 28 commits into from
Dec 19, 2024

Conversation

asandhupatlaTT
Copy link
Contributor

@asandhupatlaTT asandhupatlaTT commented Nov 21, 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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

@bbradelTT bbradelTT requested a review from eyonland December 5, 2024 18:39
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/2)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (2/2)

@asandhupatlaTT asandhupatlaTT marked this pull request as draft December 6, 2024 03:06
@asandhupatlaTT asandhupatlaTT marked this pull request as ready for review December 6, 2024 18:54
Copy link
Contributor

@bbradelTT bbradelTT left a comment

Choose a reason for hiding this comment

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

Besides pipelines, please also wait at least for Eyon's and Brian's approvals before merging.

Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Signed-off-by: Amruth Sandhupatla <[email protected]>
Copy link
Contributor

@SeanNijjar SeanNijjar left a comment

Choose a reason for hiding this comment

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

Please also run t3k model perf (the change looks completely fine but just to be safe)

@bbradelTT
Copy link
Contributor

Please also run t3k model perf (the change looks completely fine but just to be safe)

That's broken on main. You'd need to just check that you're not introducing any new failures.

Copy link
Contributor

@jvegaTT jvegaTT left a comment

Choose a reason for hiding this comment

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

It looks good

@asandhupatlaTT asandhupatlaTT merged commit a41d357 into main Dec 19, 2024
9 checks passed
@asandhupatlaTT asandhupatlaTT deleted the matmul_dps_ops branch December 19, 2024 16:51
@asandhupatlaTT
Copy link
Contributor Author

Please also run t3k model perf (the change looks completely fine but just to be safe)

https://github.com/tenstorrent/tt-metal/actions/runs/12417202012 @SeanNijjar

azecevicTT added a commit to tenstorrent/tt-mlir that referenced this pull request Feb 23, 2025
TTNN supports `tranpose_a` and `transpose_b` paramaters as default
valued boolean (false) parameter for matmul and linear op. Some
frontends also have this parameter. This PR introduces it. I have also
refactored verifiers in some places. I have removed
`test/ttmlir/Dialect/TTIR/matmul/matmul_tests_positive.mlir` because
there is the same `matmul_tests_positive.mlir` in `TTNN` directory and
`TTIR` checks are done entirely as a part of that test.

Canonicalization pattern is added that transforms `matmul(tranpose(lhs),
rhs, transpose_lhs, transpose_rhs)` to `matmul(lhs, rhs, !transpose_lhs,
transpose_rhs)`. Same for the `rhs` and linear op.

I have also updated runtime to reflect a change in `tt-metal`
tenstorrent/tt-metal#15285.

I have opened a separate issue regarding a "Adding an op" doc, because
matmul is used as an example there.
#1749

Closes #1305
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.

6 participants