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.

Sign up for GitHub

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

Jump to bottom

Initial Chip class #351

Merged
merged 16 commits into from
Dec 10, 2024
Merged

Initial Chip class #351

merged 16 commits into from
Dec 10, 2024

Conversation

broskoTT
Copy link
Contributor

@broskoTT broskoTT commented Nov 28, 2024 •
edited
Loading

Issue

First PR towards #248

Description

First in set of changes to introduce Chip abstraction class according to the plan https://docs.google.com/drawings/d/1-m1azdsBqMA0A6ATYRMfkhyeuOJuGCEI62N5a96LXj0/edit
This change also required minor rethinking of some parts of how Cluster is constructed (this took much time to figure out).

List of the changes

  • Added Chip class, and derived Local, Remote, and Mock classes. Local will be created for mmio chips, Remote for non-mmio. Mock chip is now only a placeholder, to show the Cluster constructor API changes, but it should merge with existing MockupDevice. Chip is an abstract class.
  • Chip classes for now only hold soc descriptors and nothing else. Further PRs will gradually move out logic from Cluster to Chip.
  • get_soc_descriptor and soc_descriptor_per_chip were moved around to conform to the new reality
  • This was reverted after offline discussion: (Cluster constructor which takes soc descriptor was changed so that it takes tt_SocDescriptor object rather than yaml file. This is because the object also holds harvesting information.)
  • Added another Cluster constructor which takes a set of chips. This is an experimental one, and users should know what they're doing and they can break the code easily.
  • Previously mentioned new constructor was used to offer Cluster::create_mock_cluster which is a helper function to build you a cluster with mock chip.
  • Wrote a test which showcases different constructor usages, which was a leftover from Simplify tt_SiliconDevice construction #277

Testing

Existing tests should prove no functional changes in this PR.

API Changes

This PR has no API changes.

broskoTT
broskoTT commented Nov 28, 2024
View reviewed changes
tests/api/test_cluster.cpp Outdated Show resolved Hide resolved
@broskoTT broskoTT marked this pull request as ready for review November 28, 2024 20:08
@broskoTT broskoTT added the changes api API changing PR, needs changes in client code label Nov 28, 2024
@broskoTT broskoTT force-pushed the brosko/chip_socdesc branch from 84eea73 to b90578f Compare November 29, 2024 11:12
@broskoTT broskoTT requested a review from pjanevskiTT November 29, 2024 11:37
pjanevskiTT
pjanevskiTT approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@pjanevskiTT pjanevskiTT left a comment

Choose a reason for hiding this comment

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

Nice changes, this is in sync with my mental model of how Chip should fit in our cluster class as well. So you can resolve the comment, a lot of it quite small changes. So feel free to check it in after resolving comments

device/api/umd/device/chip/chip.h Show resolved Hide resolved
device/api/umd/device/tt_simulation_device.h Outdated Show resolved Hide resolved
device/mockup/tt_mockup_device.hpp Outdated Show resolved Hide resolved
device/mockup/tt_mockup_device.hpp Outdated Show resolved Hide resolved
device/simulation/deprecated/tt_emulation_device.h Outdated Show resolved Hide resolved
device/simulation/deprecated/tt_versim_device.h Outdated Show resolved Hide resolved
tests/api/test_cluster.cpp Show resolved Hide resolved
tests/api/test_cluster.cpp Show resolved Hide resolved
device/cluster.cpp Show resolved Hide resolved
@broskoTT broskoTT requested a review from joelsmithTT December 4, 2024 08:52
@broskoTT broskoTT mentioned this pull request Dec 4, 2024
Merged
2 tasks
Base automatically changed from brosko/detect_arch to main December 4, 2024 16:34
@broskoTT broskoTT force-pushed the brosko/chip_socdesc branch from 6a61373 to d8c5575 Compare December 5, 2024 15:25
@broskoTT broskoTT force-pushed the brosko/chip_socdesc branch from fb73258 to 418e8b7 Compare December 9, 2024 14:01
@broskoTT broskoTT force-pushed the brosko/chip_socdesc branch from 789145a to dfa459c Compare December 10, 2024 09:11
@broskoTT broskoTT merged commit c6b1ada into main Dec 10, 2024
24 checks passed
@broskoTT broskoTT deleted the brosko/chip_socdesc branch December 10, 2024 15:32
This was referenced Dec 13, 2024
Merged
Merged
broskoTT added a commit that referenced this pull request Dec 17, 2024
@broskoTT
[Chip] Move TTDevice to local chip (#396)
4ed53ca 
### Issue
Related to #351 

### Description
Another PR in a push to define Chip class properly.
This PR moves tt_device under Chip, specifically under LocalChips.

### List of the changes
- Add TTDevice to LocalChip
- Renamed all_chips, target_chips and remote_chips to have more
consistent naming.
- MockChip disabled at the moment, it will start working by itself at
some point, when enough stuff is moved to Chip class.

### Testing
Existing tests

### API Changes
There are no API changes in this PR.
broskoTT added a commit that referenced this pull request Dec 17, 2024
@broskoTT
Remove setup_core_to_tlb_map (#403)
bfbb7e9 
### Issue
Related to #44 but also on a path of #351 

### Description
This function in API is a surplus. All the info is already available to
the Cluster class through configure_tlb calls.
This is initial PR which changes the API. The following PR won't change
the API but will restructure functions internally to introduce
TLBManager class.

### List of the changes
- Removed setup_core_to_tlb_map. Fill up the same internal structure
through configure_tlb api
- tlbs_init_per_chip is also unnecessary
- Extracted the logic for checking whether static tlb is setup

### Testing
Covered through existing tests.

### API Changes
This PR has API changes:
- [x] tt_metal approved PR pointing to this branch:
tenstorrent/tt-metal#16048
- [X] tt_lens doesn't use this API call.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pjanevskiTT pjanevskiTT pjanevskiTT approved these changes

@joelsmithTT joelsmithTT Awaiting requested review from joelsmithTT

Assignees
No one assigned
Labels
changes api API changing PR, needs changes in client code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@broskoTT @pjanevskiTT