Skip to content

std.simd.suggestVectorLengthForCpu: fix crashing edge cases #23915

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Fri3dNstuff
Copy link
Contributor

This PR fixes two edge cases in the function std.simd.suggestVectorLengthForCpu which previously caused a compile error, namely:

  • a type input of length greater than 8192 bytes (reaching unreachable)
  • a RISC-V CPU with features .v and .zvl65536b (attempting to assign 65536 to a u16)

@alexrp alexrp self-assigned this May 17, 2025
@alexrp
Copy link
Member

alexrp commented May 28, 2025

This approach seems a bit unfortunate for readability. Is there a reason we were using u16 in the first place?

@Fri3dNstuff
Copy link
Contributor Author

@alexrp My rationale to use the log of the size instead of the size itself is to show that the only values you find as the vector lengths are powers of two, and to make clear that the size of the input type is rounded before the calculation - I find it somewhat easier to read, but if you think otherwise, I'll change it back.

As for using u16, I don't see a reason for it was chosen over any other (runtime available) integer type, we can bump it up to a u32, or better yet, make std.math.ceilPowerOfTwo accept comptime_ints.

@alexrp
Copy link
Member

alexrp commented May 29, 2025

I personally prefer the previous approach, yeah.

or better yet, make std.math.ceilPowerOfTwo accept comptime_ints.

This seems like it'd be nice to do. There's been some effort to make other std.math functions accept comptime_int/comptime_float too.

@Fri3dNstuff
Copy link
Contributor Author

I've made a PR to make std.math.ceilPowerOfTwo (and friends) accept comptime_ints - when that'll be merged (assuming no issues there) I'll update this PR to use the changes.

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.

2 participants