Skip to content

Manually cache block attributes in PackBMatrix::pack_unpack_ #702

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

Closed
wants to merge 1 commit into from

Conversation

swolchok
Copy link
Contributor

Summary: I noticed from inspecting assembly that we were re-loading the 4 block attributes on each loop iteration. It initially surprised me that the compiler was not able to prove that the loop body doesn't mutate the block (I don't see any function calls and TBAA should mean that it can prove nothing aliases the block). On reflection, it is notable that T is signed char -- char* is the one exception to the strict aliasing rule (see https://en.cppreference.com/w/cpp/language/reinterpret_cast), so the compiler cannot prove that unpack_buf and pack_buf do not alias block and thus it has to re-load from block each time.

Reviewed By: jspark1105

Differential Revision: D30976702

Summary: I noticed from inspecting assembly that we were re-loading the 4 block attributes on each loop iteration. It initially surprised me that the compiler was not able to prove that the loop body doesn't mutate the block (I don't see any function calls and TBAA should mean that it can prove nothing aliases the block). On reflection, it is notable that `T` is `signed char` -- `char*` is the one exception to the strict aliasing rule (see https://en.cppreference.com/w/cpp/language/reinterpret_cast), so the compiler cannot prove that `unpack_buf` and `pack_buf` do not alias `block` and thus it has to re-load from `block` each time.

Reviewed By: jspark1105

Differential Revision: D30976702

fbshipit-source-id: 3bae8716065db08b1d39b245f9e0757a57425be4
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D30976702

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in dccc573.

q10 pushed a commit to q10/FBGEMM that referenced this pull request Apr 10, 2025
Summary:
X-link: pytorch#3624

Pull Request resolved: facebookresearch/FBGEMM#702

Range-based lookup for fixed B, N, and K

Reviewed By: jwfromm

Differential Revision: D68780527

fbshipit-source-id: 7b494cbfe8aba358722eb8b6b0d9893e009b7622
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants