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

Broadcast aten.maximum.default and aten.minimum.default inputs #586

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

swimdi
Copy link
Collaborator

@swimdi swimdi commented Dec 11, 2024

Ticket

#592

Problem description

aten.maximum have some broadcasting issue of tenstorrent/tt-metal#12852 , I do the workaround by using aten.expand to broadcast its inputs beforehand, and aten.expand may lowered to ttnn later

What's changed

  • Broadcast maximum/minimum inputs
  • Remove aten_maximum_default_blocklist (remain one because ttnn.from_torch not support scalar)

@swimdi swimdi self-assigned this Dec 11, 2024
@swimdi swimdi enabled auto-merge December 11, 2024 08:52
@swimdi swimdi changed the title Broadcast maximum inputs, remove aten_maximum_default_blocklist Broadcast maximum inputs Dec 11, 2024
@swimdi swimdi linked an issue Dec 11, 2024 that may be closed by this pull request
@swimdi swimdi changed the title Broadcast maximum inputs Broadcast aten.maximum.default inputs Dec 11, 2024
@swimdi swimdi force-pushed the swimdi/stage6-maximum branch from dec0d74 to a6822d3 Compare December 11, 2024 10:05
@swimdi swimdi changed the title Broadcast aten.maximum.default inputs Broadcast aten.maximum.default and aten.minimum.default inputs Dec 11, 2024
Comment on lines +1175 to +1178
if len(args) > 1:
other_tensor = args[1]
else:
other_tensor = kwargs["other"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if len(args) > 1:
other_tensor = args[1]
else:
other_tensor = kwargs["other"]
other_tensor = None # Explicitly initialize to a default value.
if len(args) > 1:
other_tensor = args[1]
else:
other_tensor = kwargs["other"]

Comment on lines +456 to +461
if new_shape is not None or new_dtype is not None:
shape = new_shape if new_shape is not None else new_node.meta["val"].size()
dtype = new_dtype if new_dtype is not None else new_node.meta["val"].dtype
fake_mode = FakeTensorMode()
fake_tensor = fake_mode.from_tensor(torch.zeros(shape, dtype=dtype))
new_node.meta["val"] = fake_tensor
Copy link
Member

Choose a reason for hiding this comment

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

can you clarify the need for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

call_function is to create a new_node and is assigned meta from current_node which is being traversed, but new_node's shape & dtype may not same with cur_node (for example, new_node.target is aten.expand from current_node and then shape change), so there give the option for user to specify the correct shape & dtype

Comment on lines +623 to +624
if input_tensor_shape == torch.Size([]):
input_tensor_shape = torch.Size([1])
Copy link
Member

Choose a reason for hiding this comment

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

can you clarify the need for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

below code cannot handle [], and the result of expand [] and [1] is the same, so I see [] as [1]

Copy link
Member

@ayerofieiev-tt ayerofieiev-tt left a comment

Choose a reason for hiding this comment

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

I hesitate to move forward with this.
For this one, I feel it is better to wait for the proper bcasting fix in TT-NN.
Change looks fairly intrusive to me

@swimdi
Copy link
Collaborator Author

swimdi commented Dec 13, 2024

I hesitate to move forward with this.
For this one, I feel it is better to wait for the proper bcasting fix in TT-NN.
Change looks fairly intrusive to me

ok, then I cancel this PR and just wait tt-metal support tenstorrent/tt-metal#12852

@swimdi swimdi marked this pull request as draft December 13, 2024 03:23
auto-merge was automatically disabled December 13, 2024 03:23

Pull request was converted to draft

@ayerofieiev-tt
Copy link
Member

tenstorrent/tt-metal#16068

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.maximum.default
2 participants