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

HarvestingMasks struct #456

Merged
merged 4 commits into from
Jan 28, 2025
Merged

HarvestingMasks struct #456

merged 4 commits into from
Jan 28, 2025

Conversation

pjanevskiTT
Copy link
Contributor

Issue

#435

Description

Add struct for software harvesting which should be used to simulate all 3 types of harvesting (Tensix, DRAM, ETH)

List of the changes

  • Add struct for software harvesting
  • Integrate it into Cluster

Testing

CI

API Changes

/

@pjanevskiTT pjanevskiTT self-assigned this Jan 14, 2025
device/api/umd/device/types/harvesting.h Outdated Show resolved Hide resolved
device/cluster.cpp Outdated Show resolved Hide resolved
device/cluster.cpp Outdated Show resolved Hide resolved
@abhullar-tt
Copy link
Contributor

will the software harvesting + silicon harvesting masks be combined or is the idea for users to query both and combine them?

@pjanevskiTT
Copy link
Contributor Author

Well we need to apply silicon harvesting masks always in order to not hang. Software harvesting is combined at the moment with silicon masks. I am happy to change that anyway you want, this is probably never going to be used by UMD, it is mostly for clients like tt-lens and tt-metal. If you have any idea how to implement this with different API let me know

@abhullar-tt
Copy link
Contributor

Well we need to apply silicon harvesting masks always in order to not hang. Software harvesting is combined at the moment with silicon masks. I am happy to change that anyway you want, this is probably never going to be used by UMD, it is mostly for clients like tt-lens and tt-metal. If you have any idea how to implement this with different API let me know

Internally if UMD stores software/silicon masks separately that is okay but would prefer if they are exposed together just as HarvestingMask

@pjanevskiTT
Copy link
Contributor Author

Sure, is logical representation of harvesting masks good for you? That means if bit 0 is set, 0 logical row of tensix is harvested, bank 0 and eth channel 0?

@abhullar-tt
Copy link
Contributor

Sure, is logical representation of harvesting masks good for you? That means if bit 0 is set, 0 logical row of tensix is harvested, bank 0 and eth channel 0?

yeah that is okay, its different than today (I think we get physical rows)

@broskoTT
Copy link
Contributor

Client should not have an option to not harvest truly harvested hw rows.
I think today we do combine whatever is passed as simulated harvesting mask with the one reported in cluster descriptor, and I think it should stay that way.

@pjanevskiTT pjanevskiTT force-pushed the pjanevski/sw_harvesting branch from e1d3f35 to c4a6d79 Compare January 27, 2025 21:14
@pjanevskiTT
Copy link
Contributor Author

I put all harvesting masks into a struct, so now those can be expose together as Almeet suggested. Patched the tests as well

@pjanevskiTT pjanevskiTT changed the title Implement software harvesting HarvestingMasks struct Jan 27, 2025
@pjanevskiTT pjanevskiTT force-pushed the pjanevski/sw_harvesting branch from 8f8da48 to e9457bd Compare January 27, 2025 21:38
Copy link
Contributor

@broskoTT broskoTT left a comment

Choose a reason for hiding this comment

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

looking good!

device/api/umd/device/blackhole_coordinate_manager.h Outdated Show resolved Hide resolved
tests/api/test_core_coord_translation_wh.cpp Show resolved Hide resolved
device/api/umd/device/cluster.h Show resolved Hide resolved
@pjanevskiTT pjanevskiTT merged commit 6a5a04e into main Jan 28, 2025
19 checks passed
@pjanevskiTT pjanevskiTT deleted the pjanevski/sw_harvesting branch January 28, 2025 13:19
@pjanevskiTT pjanevskiTT mentioned this pull request Jan 28, 2025
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