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

[stdlib] Fix inconsistency with UnsafePointer[SIMD[bool]] #3800

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

helehex
Copy link
Contributor

@helehex helehex commented Nov 22, 2024

Fixes #2813 and #3229
This was originally fixed with DTypePointer, but regressed after it got removed.
I'm still curious about whether original behavior is intended or not, and what we want to do about the inconsistency (if anything).

SIMD[bool] does seem to be packed in memory, and I'm guessing that just carries over to pop.store/pop.load:

var a = SIMD[DType.bool, 4](True)
var ptr = UnsafePointer.address_of(a)
print(ptr.bitcast[UInt8]().load())
# prints 15

Because of that, this fix may have unwanted consequences.

The solution in this PR is the same as before: it forces simd bools to be represented as bytes when doing stores/loads.
Doing it this way keeps simd bools consistent with the other dtypes when doing stores/loads of varying widths.

Maybe there's a way we could pack bools in a width-independent way, but i cant see that working with pointer offset.
we might want to have a separate method, or a parameter which keeps the original behavior.

Another thing i noticed, bitwidthof[SIMD[DType.bool, 4]]() returns 32, so I'm really not sure what's intended here.

@helehex helehex requested a review from a team as a code owner November 22, 2024 23:01
Copy link
Collaborator

@JoeLoser JoeLoser left a comment

Choose a reason for hiding this comment

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

@abduld curious to get your thoughts here.

@owenhilyard
Copy link
Contributor

I'm not a fan of silently bit-packing bools, mostly due to the same reasons as why std::vector<bool> is considered a mistake. Could we get another DType for packed_bool or similar if it's built into the language and requires powers of 2 >= 8 (for byte alignment) widths in SIMD?

The other place my mind goes is to move towards Int[signed: Bool, width: Index] since LLVM already has support which Zig and C23's _BitInt have tested. This means we have bool as an llvm i1 for BitBool and keep Bool as byte sized, and people who want packing behavior can use BitBool and the compiler can do its best to bit-pack BitBool items in a struct. We can add implicit constructors to convert between them. This also helps bitwidthof, so that generic code which does checks does the right thing.

@skongum02 skongum02 deleted the branch modular:main January 29, 2025 18:58
@skongum02 skongum02 closed this Jan 29, 2025
@skongum02 skongum02 reopened this Jan 29, 2025
@skongum02 skongum02 changed the base branch from nightly to main January 29, 2025 20:44
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.

5 participants