Conversation
There was a problem hiding this comment.
Pull request overview
Replaces callback-based rand and mix evaluation with native x86_64 and AArch64 JIT assembly.
Changes:
- Implements scalar and SIMD hashing directly in assembly.
- Adds interval validation and fallback handling.
- Improves interval-test diagnostics.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
fidget-jit/src/x86_64/point.rs |
Adds scalar native hashing. |
fidget-jit/src/x86_64/interval.rs |
Adds interval-aware native hashing. |
fidget-jit/src/x86_64/grad_slice.rs |
Adds gradient native hashing. |
fidget-jit/src/x86_64/float_slice.rs |
Adds AVX2 vectorized hashing. |
fidget-jit/src/aarch64/point.rs |
Adds scalar NEON hashing. |
fidget-jit/src/aarch64/interval.rs |
Adds interval-aware NEON hashing. |
fidget-jit/src/aarch64/grad_slice.rs |
Adds gradient native hashing. |
fidget-jit/src/aarch64/float_slice.rs |
Adds vectorized NEON hashing. |
fidget-core/src/eval/test/interval.rs |
Adds assertion diagnostics. |
Suppressed comments (4)
fidget-jit/src/x86_64/interval.rs:572
- This numerical equality check also accepts rhs bounds
-0.0and+0.0, althoughInterval::mixrejects bounds with differentto_bits()values. Such a non-singleton rhs is incorrectly hashed; use an integer comparison of the two lanes before the NaN check.
// check rhs.lower == rhs.upper (which also checks that they aren't NaN)
; vpshufd xmm1, Rx(reg(rhs_reg)), 0b11111101u8 as i8 // xmm1 = rhs.upper
; vcomiss xmm1, Rx(reg(rhs_reg))
; jp >N
; jne >N
fidget-jit/src/x86_64/point.rs:437
- This
miximplementation emits BMI2-onlyshrxinstructions even though the x86_64 JIT contract checks only for AVX2 (fidget-jit/build.rs:10-16). On an AVX2 CPU or VM without BMI2, evaluatingmixwill fault. Replace the variable shifts with instructions covered by the supported baseline, or explicitly detect and require BMI2.
; shrx r10d, r9d, r8d
fidget-jit/src/x86_64/grad_slice.rs:470
- This gradient
mixpath also introduces BMI2-onlyshrxunder an AVX2-only host requirement. AVX2 does not guarantee BMI2, so the generated function can fault on a supported CPU/VM. Avoid BMI2 in both rounds or add consistent BMI2 gating for the x86_64 JIT.
; shrx r10d, r9d, r8d
fidget-jit/src/x86_64/interval.rs:634
- The interval
randsingleton path likewise uses BMI2-onlyshrxdespite the crate's AVX2-only feature check. This can produce an illegal instruction on a host that satisfies the advertised requirement. Replace this shift with baseline/AVX2 instructions or gate the entire x86_64 JIT on BMI2.
; shrx r9d, r9d, r8d
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+620
to
+624
| // check arg.lower == arg.upper (which also checks that they aren't NaN) | ||
| ; vpshufd xmm1, Rx(reg(arg_reg)), 0b11111101u8 as i8 // xmm1 = arg.upper | ||
| ; vcomiss xmm1, Rx(reg(arg_reg)) // comparison | ||
| ; jp >N // NaN sets the parity flag | ||
| ; jne >N // NaN handler also handles non-equal case |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As always, it's annoying to write but a significant speedup versus
main: