Use CSEL to compute the NEON popcount inner-loop batch size - #548
Closed
lemire wants to merge 1 commit into
Closed
Conversation
The five NEON popcount routines each opened their outer block loop by clamping the outstanding block count to INNERMAX with a compare and a forward branch: MOVD INNERMAX, R4 CMP R4, R3 BHS <label> MOVD R3, R4 <label>: SUB R4, R3, R3 Replace it with a branch-free CSEL sequence, factored into a SPLITBLOCKS macro alongside the file's existing ZEROPART/DRAIN/FOLD4 macros: SUBS INNERMAX, R3, R6 MOVD INNERMAX, R4 CSEL CS, R4, R3, R4 CSEL CS, R6, ZR, R3 The SUBS computes the leftover block count and sets the flags that pick between the two cases in one go, so this is four instructions instead of five and drops a branch. It also removes five now-unused labels (slinner, andinner, orinner, xorinner, maskinner). This is a code-size and clarity change, not a speed one. The sequence runs once per INNERMAX batch, and since a roaring bitmap container is 1024 words = 128 blocks, that is once per call, against an inner loop that is saturating the NEON pipes for ~280 cycles. Benchmarks on an arm64 laptop show no change beyond run-to-run noise: PopcntSlice1024NEON 70.3 ns/op -> 69.8 ns/op PopcntAndSlice1024NEON 104.0 ns/op -> 103.9 ns/op R6 is used as the scratch register rather than R5, which holds the slice length across the vector loop in the two-input routines. Suggested by a reviewer on the AVX2 popcount PR.
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.
Follow-up to a review suggestion on the AVX2 popcount PR, applied to the NEON side.
What
Each of the five NEON popcount routines opened its outer block loop by clamping the outstanding 64-byte-block count to
INNERMAXwith a compare and a forward branch:This replaces that with a branch-free
CSELsequence, factored into aSPLITBLOCKSmacro to sit alongside the file's existingFOLD4/ZEROPART/DRAINmacros:The
SUBScomputes the leftover count and sets the flags that choose between the two cases, so it is four instructions instead of five, branch-free, and it removes five now-unused labels (slinner,andinner,orinner,xorinner,maskinner).R6is the scratch register rather thanR5, becauseR5holds the slice length across the vector loop in the four two-input routines.Performance: none, and that is expected
This is a code-size/clarity change. The sequence runs once per
INNERMAXbatch; since a roaring bitmap container is 1024 words = 128 blocks andINNERMAXis 1024 blocks, that is once per call, against an inner loop that saturates the NEON pipes for ~280 cycles. Measured on an arm64 laptop,-count 6:PopcntSlice1024NEONPopcntAndSlice1024NEONBoth differences are inside run-to-run noise. Please do not merge this expecting a speedup.
Testing
go test ./...passes on arm64, as doesgo vet.neonTestLengthstops out at 1025 words = 128 blocks, so the outer block loop never iterates more than once and theCSside of bothCSELs is never taken. I confirmed this by mutation: flippingCSEL CStoCSEL CCstill passesTestNEONPopcntDifferential.I verified this change locally with a temporary test at lengths that cross the batch boundary (8188/8191/8192/8193/8200, 16383–16385, 24576, 32775) plus an all-ones saturated case, differentially against the
popcnt*SliceGoreference implementations. That temp test passes on this branch and fails on the mutant. It is not included here, since the request was to keep this PR to theCSELchange alone — happy to add it as a follow-up if you would like the coverage upstream.