Fix GCC-12.x prefetch-loop collapse in greedy_search neighbor prefetch - #361
Open
yuejiaointel wants to merge 6 commits into
Open
Fix GCC-12.x prefetch-loop collapse in greedy_search neighbor prefetch#361yuejiaointel wants to merge 6 commits into
yuejiaointel wants to merge 6 commits into
Conversation
svs::lib::prefetch_l0(std::span) issues one software prefetch per cacheline to warm the next neighbor vector before the distance kernel reads it, during greedy_search graph traversal. GCC 12.x (all point releases 12.1-12.4, verified) collapses this counted loop to a SINGLE prefetch: only the first cacheline of each vector is warmed and the remaining cachelines are demand-loaded cold from DRAM. GCC 11 and GCC >=13 emit the full loop. This is a compiler codegen regression, not an SVS logic bug. Measured impact (VecSim standalone knn_query, cohere-768 fp16 IP, 1 thread, iso-recall 0.95, node = SPR Xeon 8480L, 5-rep median), gcc11 vs gcc12 vs this fix: QPS 744 / 613 / 735 (gcc12 -18%; fix recovers ~93% of the gap) L3 misses 24.7M / 856.6M / 24.3M (gcc12 ~35x; fix back to baseline) IPC 0.66 / 0.37 / 0.66 Prefetch instrs in the search worker (objdump): 7 / 3 / 5. gcc12 runs FEWER instructions yet is slower -> pure memory-latency stall from the dropped prefetch; restoring the prefetch loop removes it. Fix: force every iteration's prefetch to be emitted via volatile inline asm on x86; keep the portable _mm_prefetch path for non-x86 (#if defined(__SSE__)). Alternative for users: build with GCC != 12.x (11.x or >=13.x are unaffected).
Replaces the x86-only inline-asm workaround with `#pragma GCC unroll` above the prefetch loop. This keeps the portable _mm_prefetch path (a no-op on non-x86), so the fix works on ALL architectures, and is a harmless hint on compilers that don't recognize the pragma. Verified: gcc12 now emits the full prefetch loop (collapsed -> restored); gcc11/13/15 unaffected; clang still emits prefetches.
The #pragma GCC unroll variant fixes the isolated loop but does NOT survive the real inlining chain (accessor.prefetch -> SimpleData::prefetch -> lib::prefetch): the built module's search worker still shows the collapsed prefetch (3, same as stock gcc12). Only the volatile inline-asm version restores the prefetches in the actual binary and recovers the perf (measured). Comment updated to document this.
Replaces the x86 inline-asm workaround with a standard ISO C++ signal fence (std::atomic_signal_fence) inside the prefetch loop. Generates no code at runtime but stops GCC 12.x's RTL backend from folding away the per-cacheline prefetches. Why not #pragma GCC unroll: the loop survives GCC's GIMPLE passes intact in both gcc11 and gcc12 (verified via -fdump-tree); the collapse is an RTL-backend decision, so the GIMPLE-stage unroll pragma is a no-op on the hot greedy_search clone. Verified on the real VecSim/SVS module (cohere-768 fp16 IP, 1 thread, iso-recall 0.95418, 5-rep median): restores the gcc11 stride prefetch loop (worker prefetches 3->7) and QPS gcc12 721 -> 930 (gcc11 = 938), recovering ~97% of the gap. Portable across arches (no asm); Clang/ICX unaffected by the original bug.
|
Tick the box to add this pull request to the merge queue (same as
|
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
2 participants
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.
What this fixes
svs::lib::prefetch_l0(std::span)(include/svs/lib/prefetch.h) issues one software prefetch per cacheline to warm the next neighbor vector before the distance kernel reads it, duringgreedy_searchgraph traversal.GCC 12 drops all but the first prefetch from this loop in the built
greedy_searchhot path — so only the first cacheline of each vector is warmed and the rest load cold from DRAM. Result: a large cache-miss / memory-stall regression in SVS search. GCC 11 and GCC ≥13 are unaffected; Clang and Intel ICX are unaffected.The loop survives GCC's GIMPLE passes intact in both gcc11 and gcc12 — the collapse is an RTL-backend decision, so
#pragma GCC unroll(a GIMPLE-stage directive) does not fix it. A per-iterationstd::atomic_signal_fence— standard ISO C++, generates no code at runtime — stops the backend folding the prefetches away and restores the full loop. Portable (no inline asm; no effect on non-GCC or non-x86).The change
One line in the loop:
Impact (measured, cohere-768 fp16 IP, iso-recall 0.95, 5-rep median)
Codegen — prefetches in the fp16-IP greedy_search worker: gcc11 = 7 (stride loop present), gcc12 stock = 3 (collapsed), gcc12+fence = 6–7 (restored).
VecSim standalone, 1 thread (SVS QPS):
End-to-end Redis, parallel=100 (SVS RPS):
End-to-end, the fence brings SVS back to gcc11 parity (full recovery) and collapses the HNSW-over-SVS gap from 1.23× → 1.04×. HNSW is compiler-insensitive across builds (control), confirming the e2e gap was the SVS prefetch collapse, not an intrinsic HNSW advantage.
Verification
Confirmed by objdump (loop restored) and QPS/RPS at bit-identical recall on real VecSim + RediSearch builds. The regression was also causally isolated by re-adding the prefetch as the only change and observing full recovery. Bug reproduces across all GCC 12.x point releases (12.1–12.4); GCC 11, 13, 14, 15, 16, Clang 17, and Intel ICX 2023 are all unaffected — so an alternative to this patch is simply building with a non-GCC-12 compiler.