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

Convert aten.cumsum to ttnn.moreh_cumsum #370

Merged
merged 4 commits into from
Nov 11, 2024
Merged

Convert aten.cumsum to ttnn.moreh_cumsum #370

merged 4 commits into from
Nov 11, 2024

Conversation

jerrysky3
Copy link
Contributor

@jerrysky3 jerrysky3 commented Nov 1, 2024

Ticket

#367, #372

Problem description

Add conversion from aten.cumsum to ttnn.moreh_cumsun

Currently moreh_cumsum only supports n and c dim of (n, c h, w) 4D tensor. This conversion covers the existing model cases by unsqueeze (h, w) to (h, w, 1, 1)

What's changed

@jerrysky3 jerrysky3 marked this pull request as ready for review November 1, 2024 12:59
@@ -306,9 +303,8 @@ def try_add_data_move_in(src_node, dst_idx, dst_node, device) -> torch.fx.node.N
else:
kwargs["dtype"] = TtnnBfloat16()

if (is_tt_compute(dst_node) and dst_node.target not in TTNN_LAYOUT_CHANGE_OPS) or (
dst_node.target in TTNN_LAYOUT_CHANGE_OPS and HasValidPageSize(src_node.meta["val"].size(), strict=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as the comment above regarding the check

if (
(dst_node.target in TTNN_LAYOUT_CHANGE_OPS and not can_be_tilized(dst_node))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

During testing, I found that these check might be incorrect for some cases. For example: ttnn.reshape can reshape (1, 32) into (1, 32, 1, 1). But we currently don't maintain the output shape of of converted ttnn ops, can_be_tilized will still check based on the stalled shape (1, 32) and returns incorrect decision.

Here is an example if we print the dst_node.meta["val"] of ttnn.reshape from (1, 32) to (1, 32, 1, 1)

arg0_1: {'val': FakeTensor(..., size=(1, 32), dtype=torch.bfloat16), 'tensor_meta': TensorMetadata(shape=torch.Size([1, 32]), dtype=torch.bfloat16, requires_grad=False, stride=(32, 1), memory_format=torch.contiguous_format, is_quantized=False, qparams={})}

We can see the dst_node.meta["val"].size() will return (1, 32) instead of the expected output shape (1, 32, 1, 1)

I filed #372 to track this issue and found that #322 seems to do lots of improvements for these checks. So I'd propose remove these checks for now (be more conservative) and let #322 make overall improvements

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, there has been some new changes in TTNN that makes some of these Layout workarounds obsolete. However, they're still not perfect and I find that some layout changes are still needed for some models. @jerrysky3 Have you checked if this PR passes all the models? If it does then we can merge this first, and I'll continue to work my PR #322 and merge afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jerrysky3 jerrysky3 added this pull request to the merge queue Nov 11, 2024
Merged via the queue into main with commit 55a6521 Nov 11, 2024
50 checks passed
@jerrysky3 jerrysky3 deleted the feature/cumsum branch November 11, 2024 07:11
@ayerofieiev-tt ayerofieiev-tt linked an issue Nov 21, 2024 that may be closed by this pull request
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.

aten.cumsum.default
3 participants