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

[UMD] Switching to new coord API #17003

Merged
merged 7 commits into from
Jan 27, 2025
Merged

[UMD] Switching to new coord API #17003

merged 7 commits into from
Jan 27, 2025

Conversation

broskoTT
Copy link
Contributor

@broskoTT broskoTT commented Jan 22, 2025

Ticket

Related to #17002

Problem description

One of the many changes leading towards tt-metal using new CoreCoords, both in soc_descriptor and in umd's interface.
Due to complexity of all the maps present, the changes will be relatively small.

What's changed

  • virtual_core_from_physical_core and get_virtual_coordinate_from_physical_coordinates rewritten to use translate_coord_to instead of convert_to_umd_coordinates and translate_to_noc_table_coords
  • removed worker_log_to_physical_routing and physical_routing_to_virtual_routing from metal_soc_descriptor
  • get_physical_tensix_core_from_logical and convert_to_umd_coordinates rewritten to use translate_coord_to
  • Rewritten some get_physical_core_from_logical_core to get_physical_tensix_core_from_logical
  • grayskull soc descriptor changed so that it is rowwise instead of columnwise. This matches the way it is written in UMD, and it matches other soc descriptors.
  • bump umd to pull in some new changes in UMD

Checklist

All runs on brosko/soc_new_api :

@broskoTT broskoTT marked this pull request as ready for review January 23, 2025 21:17
Copy link
Contributor

@abhullar-tt abhullar-tt left a comment

Choose a reason for hiding this comment

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

overall lgtm, @tt-asaigal you might want to take a look as well

tt_metal/common/metal_soc_descriptor.cpp Outdated Show resolved Hide resolved
tt_metal/common/metal_soc_descriptor.cpp Show resolved Hide resolved
Copy link
Contributor

@tt-asaigal tt-asaigal left a comment

Choose a reason for hiding this comment

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

Overall great changes! Just wondering - is the next step to have UMD accept generic coordinate systems so that we can get rid of convert_to_umd_coordinates?

tt_metal/llrt/tt_cluster.cpp Show resolved Hide resolved
tt_metal/common/metal_soc_descriptor.cpp Outdated Show resolved Hide resolved
tt_metal/common/metal_soc_descriptor.cpp Show resolved Hide resolved
@broskoTT
Copy link
Contributor Author

Overall great changes! Just wondering - is the next step to have UMD accept generic coordinate systems so that we can get rid of convert_to_umd_coordinates?

Yes, that would be one of the goals, it is questionable how many PRs I'll have until we come to that state, the existing code is messy.
The idea is to switch everything to tt::umd::CoreCoords and not have any translation outside of it. But also reduce number of translations if makes sense (I hope this will end up being obvious at some point)

Copy link
Contributor

@abhullar-tt abhullar-tt left a comment

Choose a reason for hiding this comment

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

Thanks for starting this work out with a small PR, makes it easier to see the uplift

tt_metal/common/metal_soc_descriptor.cpp Outdated Show resolved Hide resolved
tt_metal/common/metal_soc_descriptor.cpp Show resolved Hide resolved
@broskoTT broskoTT merged commit bc5f59e into main Jan 27, 2025
292 of 294 checks passed
@broskoTT broskoTT deleted the brosko/soc_new_api branch January 27, 2025 14:01
williamlyTT pushed a commit that referenced this pull request Jan 30, 2025
### Ticket
Related to #17002

### Problem description
One of the many changes leading towards tt-metal using new CoreCoords,
both in soc_descriptor and in umd's interface.
Due to complexity of all the maps present, the changes will be
relatively small.

### What's changed
- virtual_core_from_physical_core and
get_virtual_coordinate_from_physical_coordinates rewritten to use
translate_coord_to instead of convert_to_umd_coordinates and
translate_to_noc_table_coords
- removed worker_log_to_physical_routing and
physical_routing_to_virtual_routing from metal_soc_descriptor
- get_physical_tensix_core_from_logical and convert_to_umd_coordinates
rewritten to use translate_coord_to
- Rewritten some get_physical_core_from_logical_core to
get_physical_tensix_core_from_logical
- grayskull soc descriptor changed so that it is rowwise instead of
columnwise. This matches the way it is written in UMD, and it matches
other soc descriptors.
- bump umd to pull in some new changes in UMD

### Checklist
All runs on brosko/soc_new_api :
- [x] All post-commit tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986193862
- [x] Blackhole post-commit tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986195849
- [x] (Single-card) Model perf tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986198348
- [x] (Single-card) Device perf regressions :
https://github.com/tenstorrent/tt-metal/actions/runs/12986201144
- [x] (T3K) T3000 unit tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986203715
- [x] (T3K) T3000 demo tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986205723
- [x] (TG) TG unit tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986207280
- [x] (TG) TG demo tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986209173
- [x] (TGG) TGG unit tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986211545
- [x] (TGG) TGG demo tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986213812
yieldthought pushed a commit that referenced this pull request Jan 31, 2025
### Ticket
Related to #17002

### Problem description
One of the many changes leading towards tt-metal using new CoreCoords,
both in soc_descriptor and in umd's interface.
Due to complexity of all the maps present, the changes will be
relatively small.

### What's changed
- virtual_core_from_physical_core and
get_virtual_coordinate_from_physical_coordinates rewritten to use
translate_coord_to instead of convert_to_umd_coordinates and
translate_to_noc_table_coords
- removed worker_log_to_physical_routing and
physical_routing_to_virtual_routing from metal_soc_descriptor
- get_physical_tensix_core_from_logical and convert_to_umd_coordinates
rewritten to use translate_coord_to
- Rewritten some get_physical_core_from_logical_core to
get_physical_tensix_core_from_logical
- grayskull soc descriptor changed so that it is rowwise instead of
columnwise. This matches the way it is written in UMD, and it matches
other soc descriptors.
- bump umd to pull in some new changes in UMD

### Checklist
All runs on brosko/soc_new_api :
- [x] All post-commit tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986193862
- [x] Blackhole post-commit tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986195849
- [x] (Single-card) Model perf tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986198348
- [x] (Single-card) Device perf regressions :
https://github.com/tenstorrent/tt-metal/actions/runs/12986201144
- [x] (T3K) T3000 unit tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986203715
- [x] (T3K) T3000 demo tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986205723
- [x] (TG) TG unit tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986207280
- [x] (TG) TG demo tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986209173
- [x] (TGG) TGG unit tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986211545
- [x] (TGG) TGG demo tests :
https://github.com/tenstorrent/tt-metal/actions/runs/12986213812
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.

3 participants