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

#0: Remove incorrect assertion on cb page sizes being a multiple of 4… #15752

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

tt-aho
Copy link
Contributor

@tt-aho tt-aho commented Dec 5, 2024

… (should have been 16) and update circular buffer init/usage to support any page size for cbs that aren't used by compute. Compute CBs still require 16B multiple for address/sizes

Ticket

Link to Github Issue

Problem description

We were incorrectly asserting for 4B alignment on main for CB page sizes, when we actually needed 16B. However, we only really need 16B alignment when the CBs are used in compute so we can relax this constraint in some cases.

What's changed

Remove the assertion for 4B alignment for CBs. We will pass and store the original values for address, size, page size when used in a DM kernel, and bit shift these values by 4 when initialized in a compute kernel.

Checklist

@tt-aho tt-aho force-pushed the aho/remove-cb-restriction branch 2 times, most recently from 604612d to 3bc2e6f Compare December 8, 2024 17:06
@tt-aho tt-aho marked this pull request as ready for review December 8, 2024 20:02
@tt-aho tt-aho requested a review from rtawfik01 December 8, 2024 20:02
@tt-aho
Copy link
Contributor Author

tt-aho commented Dec 8, 2024

@rtawfik01 @pgkeller I haven't added the watcher validation for the compute cb addr/sizes to this pr yet.

In the non-validated case we will bit-shift the addrs/ptrs during cb init to avoid bit shifting at runtime.
We can't validate during CB initialization since we don't know which cbs are used by compute or not yet, that means when we need to validate then we can't actually bit shift during cb initialization, and have to wait until we access these attributes/generate addresses within the compute apis/llks to validate the address is 16B aligned, and then bit shift.

This will touch quite a few compute/llk files. I can try and do that in this pr, or we can split it and add the validation in a separate pr. Adding the validation separately wouldn't make the behaviour any less safe than what is on main, since we were only checking that the page size was 4B aligned on main so it wasn't guaranteeing any correct behaviour anyways.

@tt-aho tt-aho force-pushed the aho/remove-cb-restriction branch from 3bc2e6f to 395b5a7 Compare December 8, 2024 20:21
… (should have been 16) and update circular buffer init/usage to support any page size for cbs that aren't used by compute. Compute CBs still require 16B multiple for address/sizes
@tt-aho tt-aho force-pushed the aho/remove-cb-restriction branch from 395b5a7 to 64dae83 Compare December 9, 2024 01:07
@tt-aho tt-aho merged commit edce563 into main Dec 9, 2024
155 checks passed
@tt-aho tt-aho deleted the aho/remove-cb-restriction branch December 9, 2024 19:38
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