zig icon indicating copy to clipboard operation
zig copied to clipboard

std.simd.suggestVectorLengthForCpu: fix crashing edge cases

Open Fri3dNstuff opened this issue 6 months ago • 3 comments

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)

Fri3dNstuff avatar May 17 '25 18:05 Fri3dNstuff

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

alexrp avatar May 28 '25 10:05 alexrp

@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.

Fri3dNstuff avatar May 29 '25 05:05 Fri3dNstuff

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.

alexrp avatar May 29 '25 09:05 alexrp

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.

Fri3dNstuff avatar Jun 02 '25 19:06 Fri3dNstuff