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

Remove Cluster' version of harvesting #439

Open
broskoTT opened this issue Dec 26, 2024 · 0 comments
Open

Remove Cluster' version of harvesting #439

broskoTT opened this issue Dec 26, 2024 · 0 comments
Assignees

Comments

@broskoTT
Copy link
Contributor

While working on #248
Harvesting should be removed from Cluster class, now that it exists in tt_socdescriptor. This old functionality should be carefully removed since it has a lot of code/structures, but all of it should be already reimplemented through coord_manager and tt_soc_descriptor.

@broskoTT broskoTT self-assigned this Dec 26, 2024
broskoTT added a commit that referenced this issue Jan 3, 2025
)

### Issue
Related to #439 

### Description
First of the changes preparing removal of harvesting logic inside
Cluster.
perform_harvesting, and simulated_harvesting_masks arguments are now
added to affect tt_SocDescriptor.
Had to remove logic converting harvesting_mask from physical layout
coords to logical coords out of the CoordinateManager constructor.

### List of the changes
- perform_harvesting now affects harvesting in tt_SocDescriptor
- simulated_harvesting_masks are now used for harvesting in
tt_SocDescriptor
- get_tensix_harvesting_mask is a helper function which does both of
these
- shuffle_tensix_harvesting_mask is now a static public function, which
should be called prior to calling CoordinateManager
- CoordinateManager now accepts harvesting in logical coords.
- Due to deprecated get_harvesting_masks_for_soc_descriptors, there is
an additional shuffle_tensix_harvesting_mask_to_noc0_coords which does
the required translation.
- tt_SocDescriptor removed copy constructor, since it is already a
default one.
- Other functions changed accordingly to pass this arguments

### Testing
- Changed call to get_harvesting_masks_for_soc_descriptors, so it is
tested with the new flow, through harvesting provided by socdescriptor
class
- Added HarvestingShuffle tests in all arch tests

### API Changes
There are no API changes in this PR.
broskoTT added a commit that referenced this issue Jan 10, 2025
### Issue
Related to #439 

### Description
The last missing feature of Coordinate Manager is noc_translation
enabled feature. If this is disabled, translated coords will be same as
physical coords.

### List of the changes
- Add bool noc_translation_enabled to CoordinateManager constructor
- Added default versions of translate-physical mapping, which do
identity mapping.
- Added a test to test if this flag works
- The translation flag is now required, so added it explicitly to tests
where needed.

### Testing
Added another test, existing CI tests.

### API Changes
There are no API changes in this PR.
broskoTT added a commit that referenced this issue Jan 21, 2025
### Issue
Somewhat related to #439 

### Description
get_core_at can be a useful API provided by coordinate manager. There
are scenarios where we want to learn what is located at a specific core
location. This obviously can't be offered for LOGICAL coord system, but
for others it is possible.
Since there are also some places in the code which request specifically
some cores which might be router_only cores, I've also added
router_cores to CoordinateManager and SocDescriptor (see
Cluster::test_setup_interface or
Cluster::broadcast_pcie_tensix_risc_reset)

### List of the changes
- Add translate_coord_to api which doesn't know what coretype is there
- Add router cores everywhere
- Restructured constants a bit to follow tensix, dram, eth, arc, pci
ordering.
- Wrote a test to verify new behavior

### Testing
Added tests which test the new API.

### API Changes
There are no API changes in this PR.
broskoTT added a commit that referenced this issue Jan 23, 2025
### Issue
While working on #439

### Description
Translation was being calculated by using logical coordinates. But we
want logical coordinates to be in the same order as defined in the input
yaml. Since these are not consecutive (meaning consecutive logical
coords are not consecutive any other coords), this breaks translation
logic.
BH to be fixed.

### List of the changes
- Fixed the wh eth translation test not to have any logic
- Fixed eth translation to match the earlier one from cluster

### Testing
Fixed the test to have hardcoded values.

### API Changes
There are no API changes in this PR.
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

No branches or pull requests

1 participant