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

Soc descriptor harvesting #202

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Conversation

pjanevskiTT
Copy link
Contributor

@pjanevskiTT pjanevskiTT commented Oct 22, 2024

Fix #102, part of a general effort around soc descriptor #100

Goal of the PR is to move harvesting logic to soc descriptor, with clear interface for coordinate translation

Finished TODOs

  • Derive logical, virtual and physical coordinates from tt_xy_pair
  • Make soc descriptor tests arch independent as much as possible
  • Cleanup soc desc code for harvesting on y or x based on the arch
  • Revert commented tests
  • Move open UMD doc about harvesting to UMD repo
  • Add tests for grayskull
  • Full interface to convert between cordinate system

First class TODOs (will finish in this PR)

  • Blackhole to translated coordinates fix
  • cpp nits

UMD client changes

No changes needed for bumping

TODOs with follow up issues (won't fix in this PR)

device/tt_soc_descriptor.cpp Outdated Show resolved Hide resolved
tests/blackhole/test_soc_descriptor.cpp Outdated Show resolved Hide resolved
device/tt_soc_descriptor.h Outdated Show resolved Hide resolved
@abhullar-tt
Copy link
Contributor

is the idea for tt_xy_pair to have an additional field to denote coordinate type (logical, virtual, physical, etc?)

@pjanevskiTT
Copy link
Contributor Author

is the idea for tt_xy_pair to have an additional field to denote coordinate type (logical, virtual, physical, etc?)

I am probably going to derive each type of coordinates from tt_xy_pair, so instead of

using tt_logical_coords = tt::umd::tt_xy_pair;

we are going to have derive tt_logical_coords from tt_xy_pair, so you still have only x and y as fields, but you can use coordinates as types, you will know which coordinates you are using just based on the type of the variable. Also, we want to be sure to not use the API in a way it is not intended, we can provide API for each type of coordinates. Does that make more sense?

Copy link
Contributor

@joelsmithTT joelsmithTT left a comment

Choose a reason for hiding this comment

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

I do not have an accurate mental model for the coordinate systems.. I suspect I am not the only person with this problem. I think that clear definitions of what each of the coordinate systems represent (perhaps with some examples) could go a long way for helping developers build an accurate understanding of coordinates and harvesting.

device/tt_xy_pair.h Outdated Show resolved Hide resolved
@abhullar-tt
Copy link
Contributor

is the idea for tt_xy_pair to have an additional field to denote coordinate type (logical, virtual, physical, etc?)

I am probably going to derive each type of coordinates from tt_xy_pair, so instead of

using tt_logical_coords = tt::umd::tt_xy_pair;

we are going to have derive tt_logical_coords from tt_xy_pair, so you still have only x and y as fields, but you can use coordinates as types, you will know which coordinates you are using just based on the type of the variable. Also, we want to be sure to not use the API in a way it is not intended, we can provide API for each type of coordinates. Does that make more sense?

yes that makes sense, thanks!

@pjanevskiTT pjanevskiTT force-pushed the pjanevski/soc_desc_harvesting branch 5 times, most recently from 94355d4 to 7f5e04a Compare October 23, 2024 09:12
@pjanevskiTT pjanevskiTT marked this pull request as ready for review October 23, 2024 15:17
@pjanevskiTT pjanevskiTT mentioned this pull request Oct 23, 2024
@pjanevskiTT pjanevskiTT added the changes api API changing PR, needs changes in client code label Oct 24, 2024
@pjanevskiTT pjanevskiTT force-pushed the pjanevski/soc_desc_harvesting branch 2 times, most recently from a508dac to 950feee Compare October 24, 2024 13:09
@pjanevskiTT
Copy link
Contributor Author

pjanevskiTT commented Oct 25, 2024

I know this PR is becoming a bit hard to review with a lot of code, so I will separate TODOs left into first class that I will solve in this PR and then merge it, and into second class tasks that I will open issues for and solve it all as separate PRs. I tried thinking about making this PR smaller as well, but I think this is a good base (coordinate conversion API, tests, docs) for harvesting so we can build on top of it. Uplifting this is also going to be seamless since it doesn't change or removes the API, it just adds the functions, so I can make the API function for tt-metal in the following commits.

@broskoTT @joelsmithTT let me know if this is ok for you

docs/coordinate_systems.md Outdated Show resolved Hide resolved
tests/soc_descs/grayskull_10x12.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@joelsmithTT joelsmithTT left a comment

Choose a reason for hiding this comment

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

If you want to get this in and refine later, that is good with me.

But I maintain the view that it is too difficult to establish a correct understanding of the coordinate schemes from the information available in the repo. Furthermore, I am suspicious that the coordinate situation is overly complex and that there is opportunity for simplification.

- Coordinate manager class for each arch

- Write tests for the harvesting

- Add functions to convert between coordinate systems
@pjanevskiTT pjanevskiTT merged commit bfa337a into main Oct 29, 2024
18 checks passed
@pjanevskiTT pjanevskiTT deleted the pjanevski/soc_desc_harvesting branch October 29, 2024 14:51
mbezuljTT pushed a commit that referenced this pull request Nov 1, 2024
- Define base for new harvesting API (coordinate types, translation functions)

- Add tests for the harvesting API

- Implement Blackhole harvesting requirements
This was referenced Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes api API changing PR, needs changes in client code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Harvesting in SoCDescriptor
5 participants