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

Cluster::configure_tlb is wrong #303

Closed
joelsmithTT opened this issue Nov 15, 2024 · 1 comment
Closed

Cluster::configure_tlb is wrong #303

joelsmithTT opened this issue Nov 15, 2024 · 1 comment
Assignees
@joelsmithTT
Labels
changes api API changing PR, needs changes in client code good first issue Good for newcomers

Comments

@joelsmithTT
Copy link
Contributor

void Cluster::configure_tlb(chip_id_t logical_device_id, tt_xy_pair core, std::int32_t tlb_index, std::int32_t address, uint64_t ordering)
The int32_t address is incorrect. It should be uint64_t.
@joelsmithTT joelsmithTT added changes api API changing PR, needs changes in client code good first issue Good for newcomers P2 Minor issues labels Nov 15, 2024
@joelsmithTT joelsmithTT mentioned this issue Nov 24, 2024
Merged
@broskoTT broskoTT removed the P2 Minor issues label Nov 26, 2024
joelsmithTT added a commit that referenced this issue Nov 26, 2024
@joelsmithTT
Fix address types (#328)
766cc54 
### Issue
#303

### Description
Using signed integer for address is wrong and can cause bugs due to sign
extension since UMD represents addresses as uint64_t internally. 32 bits
is unnecessarily restrictive (there are address spaces in the chips that
reach beyond 0x7fffffff that I might want to configure a static window
for).

### List of the changes
* Use uint64_t instead of int32_t in `configure_tlb` method
* Convert sizes/constants in some header files from int32_t to uint32_t
* Remove unnecessary `std::` prefixing

### Testing
CI

### API Changes
This PR has API changes, but it shouldn't break anything.
@joelsmithTT joelsmithTT self-assigned this Nov 26, 2024
@joelsmithTT
Copy link
Contributor Author

Fixed in #328

@joelsmithTT joelsmithTT mentioned this issue Dec 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@joelsmithTT joelsmithTT

Labels
changes api API changing PR, needs changes in client code good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@joelsmithTT @broskoTT