blasgraph: route SIMD through ndarray::simd, gate hand-rolled fallback#666
blasgraph: route SIMD through ndarray::simd, gate hand-rolled fallback#666AdaWorldAPI wants to merge 1 commit into
Conversation
Two SIMD-hygiene fixes in ndarray_bridge:
1. Consume ndarray::simd::{hamming_distance_raw,popcount_raw} (the W1a
polyfill facade) instead of reaching into ndarray::hpc::bitwise::*
directly. 'All SIMD from ndarray::simd' — hpc is the impl home, simd
is the contract surface consumers use.
2. The 8 hand-rolled unsafe intrinsics (hamming/popcount scalar+avx2+
avx512bw+avx512vpopcntdq) were compiled unconditionally but only
CALLED under #[cfg(not(feature = ndarray-hpc))], so with ndarray-hpc
on (production) they were dead code — hand-rolled unsafe SIMD sitting
in a build where ndarray's SIMD is the live path. Gate each definition
behind not(ndarray-hpc) so they vanish when ndarray is present and the
only SIMD is ndarray's; they still serve the genuine no-ndarray
(minimal/wasm) fallback.
Verified: default build (ndarray-hpc) finishes clean with no
ndarray_bridge dead-code warnings; the no-default-features fallback path
still compiles the intrinsics.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RbXniqYUYgwUsRrEnRRiid
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_de8278a8-7425-46fc-b774-47d84410b7fe) |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ndarray-lance-graph SIMD dispatch bridge now calls ChangesSIMD Dispatch Bridging Update
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55269e195b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// Scalar Hamming distance: XOR each u64 pair and sum `count_ones()`. | ||
| #[cfg(not(feature = "ndarray-hpc"))] |
There was a problem hiding this comment.
With the default feature set, ndarray-hpc is enabled in crates/lance-graph/Cargo.toml, so this cfg removes hamming_scalar from the test build; the same newly added cfg on popcount_scalar has the same effect. The tests in this module still call those helpers directly in test_scalar_hamming_small and test_scalar_popcount_small, so cargo test --manifest-path crates/lance-graph/Cargo.toml will fail name resolution before running the CI-gated default tests. Please either keep these helpers compiled under cfg(test) or gate the scalar-only tests the same way.
Useful? React with 👍 / 👎.
2 participants
Closed per operator directive — do not touch lance-graph. No changes to lance-graph in this session. (The SIMD-hygiene idea can be revisited separately if wanted.)
Summary by CodeRabbit