Skip to content

db/recsplit: AVX-512 findBijection - #24418

Closed
AskAlexSharov wants to merge 5 commits into
mainfrom
alex/simd_recsplit_37
Closed

AskAlexSharov wants to merge 5 commits into
mainfrom
alex/simd_recsplit_37

Conversation

@AskAlexSharov

Copy link
Copy Markdown
Collaborator

Experimental, alongside #24410's SIMD build setup.

findBijection's eight hand-unrolled salt candidates fit one 512-bit register, so splitmix64 and remap16 run once per key instead of eight times.

BenchmarkFindBijection   3924us -> 835us   4.7x
BenchmarkBuild           1.415s -> 1.035s  1.37x

n5, AMD EPYC 4344P, go1.27.1, 5 and 3 runs. TestFindBijectionMatchesGeneric checks the compiled implementation against the scalar one, so it runs in both build modes.

AVX2 cannot host this: splitmix64 and remap16 both need a 64x64 multiply, which is VPMULLQ (AVX512DQ). Emulating it from VPMULUDQ partial products costs more than the unrolled scalar form saves. findSplit is left alone — it histograms into a per-fanout count array, which is a scatter.

Falls back to scalar when AVX-512 is absent, and builds only under go1.27 with GOEXPERIMENT=simd on amd64.

The eight salt candidates the scalar loop unrolls by hand fit one 512-bit
register, so the splitmix64 finaliser and remap16 run once per key instead of
eight times. AVX2 cannot host it: both need a 64x64 multiply, which is VPMULLQ
(AVX512DQ), and emulating that from VPMULUDQ partial products costs more than
the unrolled scalar form.

findSplit is left alone — it histograms into a per-fanout count array, which is
a scatter.

Experimental: builds only under go1.27 with GOEXPERIMENT=simd on amd64, and
falls back to the scalar path when AVX-512 is absent.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The hardware-specific path depends on experimental Go SIMD support and requires final validation on AVX-512 hardware.

Review effort: Balanced
Findings: None

What changed in this PR

Adds an AVX-512 implementation of RecSplit’s bijection search while preserving scalar fallback behavior.

Changes:

  • Renames the scalar implementation for explicit fallback use.
  • Adds SIMD and generic build-tag dispatchers.
  • Tests SIMD/scalar result consistency across bucket sizes.
File Description
db/​recsplit/​recsplit.go Exposes the scalar implementation as findBijectionGeneric.
db/​recsplit/​recsplit_test.go Adds implementation-equivalence coverage.
db/​recsplit/​bijection_simd.go Implements AVX-512 vectorized salt searching.
db/​recsplit/​bijection_generic.go Provides the non-SIMD dispatcher.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@AskAlexSharov

Copy link
Copy Markdown
Collaborator Author

Review of this PR, plus two follow-ups.

bijection_simd.go:L24-28: shrink: 5-line comment restated the commit message. 4 lines.  -2
bijection_simd.go:L36:    correctness: modulus was `m`, generic uses `m & 31`. Equivalent
                          while MaxLeafSize=24 is enforced, but raising it would make the
                          two diverge silently. Mirrored.
net: -2 lines.

Both applied. Everything else I checked and left: bijection_generic.go (the rename is forced — the SIMD file cannot redeclare the symbol, and a function-pointer dispatch puts an indirect call in the hot loop), the hasAVX512 fallback (an amd64 binary built with the experiment can still land on a non-AVX512 machine), and the hoisted broadcasts (loop-invariant).

Optimisation

The first version collapsed the scalar's eight independent salt chains into one register, which is latency-bound: splitmix64 is serial. Two explicit chains restore the ILP.

scalar              3885us
1 vector             835us   4.65x
4 vectors (array)   1007us   slower — arrays of SIMD values spill to stack
2 vectors (explicit) 788us   4.93x

Four chains in a [4]Uint64x8 lost 20% to spills; two in plain variables is the sweet spot on this core.

BenchmarkFindBijection  3885us -> 788us   4.93x
BenchmarkBuild          1.407s -> 1.023s  1.38x

n5, AMD EPYC 4344P, go1.27.1, 3 runs each.

Portable simd package

Not possible today. The portable simd API exposes Mul for 8/16/32-bit lanes but not 64-bit, and no per-lane ShiftLeft for any type. This kernel needs both: splitmix64 and remap16 are 64x64 multiplies, and the accumulator step is 1 << r. The portable API only surfaces what every target can do, and AVX2 has no VPMULLQ — the same reason this is AVX-512 only. So simd/archsimd with a runtime gate is the only option until the portable package grows 64-bit multiply and variable shift.

@AskAlexSharov

Copy link
Copy Markdown
Collaborator Author

Correction to my note above: the portable simd package can express this kernel — I was wrong that it could not. I wrote it and it is correct. It is just much slower, and on amd64 it does not build.

Both gaps are synthesizable:

  • 64x64 multiply from 16-bit limbs via Uint32s.Mul. Mask both operands to 16 bits and the odd 32-bit sublanes the multiply also touches are 0*0, so each 64-bit lane holds the exact product. Ten partial products per multiply.
  • 1 << r by conditionally shifting by each power of two: v.ShiftAllLeft(1<<k).IfElse(r.And(1<<k).NotEqual(zero), v) for k in 0..4.

Parity against the scalar form passes on arm64 (m up to 12).

arm64, 2 lanes:   scalar 2031ns   portable 30315ns   15x slower

The portable package fixes one vector width per execution and reports Len() == 2 on arm64, so it searches two salts per pass while paying ~85 ops per key against the scalar's ~20 for the same two salts.

On amd64 it does not compile — the 512-bit specialization emits an invalid instruction. Minimal reproducer on go1.27.1 linux/amd64:

func BitAt(r, one simd.Uint64s) simd.Uint64s {
	zero := simd.BroadcastUint64s(0)
	return one.ShiftAllLeft(1).IfElse(r.And(simd.BroadcastUint64s(1)).NotEqual(zero), one)
}
./ice_test.go:9:4: ice.BitAt@simd512: invalid instruction: VPSLLQ Z1, K1, Z1

So simd/archsimd with the runtime AVX-512 gate stays the right shape for this PR, now for measured reasons rather than assumed ones.

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