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

Add MLIR lowering for cumsum op #1186

Merged
merged 1 commit into from
Feb 8, 2025
Merged

Conversation

ashokkumarkannan1
Copy link
Contributor

Ticket

Fix #891, Fix #503

Problem description

Add MLIR lowering for cumsum op

What's changed

  • Added mapping for cumsum in lower_to_mlir
  • Add TargetType handling for Int64 type in lower_to_mlir
  • Add E2E tests for cumsum op

@ashokkumarkannan1 ashokkumarkannan1 changed the title cumsum support with the attr type mapping Add MLIR lowering for cumsum op Feb 6, 2025
Copy link

github-actions bot commented Feb 6, 2025

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests512 ran452 passed60 skipped0 failed
TestResult
No test annotations available

1 similar comment
Copy link

github-actions bot commented Feb 6, 2025

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests512 ran452 passed60 skipped0 failed
TestResult
No test annotations available

Copy link

github-actions bot commented Feb 6, 2025

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests570 ran490 passed80 skipped0 failed
TestResult
No test annotations available

Copy link
Contributor

@mstojkovicTT mstojkovicTT 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 overall, just one nit

@@ -234,6 +236,7 @@ class MLIRGenerator
case TargetType::UInt32:
TT_ASSERT(std::get<int>(value) >= 0, "Value must be an >= 0 for conversion to uint32");
return builder_.getUI32IntegerAttr(static_cast<uint32_t>(std::get<int>(value)));
case TargetType::Int64: return builder_.getI64IntegerAttr(static_cast<int64_t>(std::get<int>(value)));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could you please add a line break here so it starts on a new line :))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initialy i wrote it in a newline after the clang formating it moved to the same line.

@ashokkumarkannan1 ashokkumarkannan1 force-pushed the akannan/op_support_cumsum branch 2 times, most recently from cfb8a21 to 3d7c81c Compare February 7, 2025 13:21
@ashokkumarkannan1 ashokkumarkannan1 force-pushed the akannan/op_support_cumsum branch from 3d7c81c to 3bdcb43 Compare February 7, 2025 13:32
Copy link

github-actions bot commented Feb 7, 2025

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests592 ran466 passed126 skipped0 failed
TestResult
No test annotations available

1 similar comment
Copy link

github-actions bot commented Feb 7, 2025

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests592 ran466 passed126 skipped0 failed
TestResult
No test annotations available

Copy link

github-actions bot commented Feb 7, 2025

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests650 ran509 passed141 skipped0 failed
TestResult
No test annotations available

1 similar comment
Copy link

github-actions bot commented Feb 7, 2025

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests650 ran509 passed141 skipped0 failed
TestResult
No test annotations available

@ashokkumarkannan1 ashokkumarkannan1 merged commit 7286f5b into main Feb 8, 2025
10 checks passed
@ashokkumarkannan1 ashokkumarkannan1 deleted the akannan/op_support_cumsum branch February 8, 2025 06:33
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.

[Op Testing] Cumsum Unsupported operation for lowering from TTForge to TTIR: cumsum
3 participants