Replace CHOOSE_IMPLEMENTATION/CHOOSE_SVE_IMPLEMENTATION macros with constexpr dispatch tables - #1004
Draft
dor-forer wants to merge 2 commits into
Draft
Replace CHOOSE_IMPLEMENTATION/CHOOSE_SVE_IMPLEMENTATION macros with constexpr dispatch tables#1004dor-forer wants to merge 2 commits into
dor-forer wants to merge 2 commits into
Conversation
…onstexpr dispatch tables The switch-generating macro pyramid in implementation_chooser.h is replaced by dispatch_by_residual/dispatch_by_sve_residual (residual_dispatch.h): a small C++20 helper that builds a static constexpr array of the same per-residual kernel specializations via a templated-lambda selector, indexed once by dim % chunk instead of switched on. The chunk size is stated exactly once (as the helper's own template argument) so it can't drift from a separately written modulus the way two independent literals could. Choose_*/GetDistFunc names and signatures are unchanged, so no other file needed to change - test_spaces.cpp and bm_spaces.h were verified to compile against this unchanged. Every leaf SIMD kernel is untouched. Measured on this AVX2 build: per-wrapper machine code drops from 528 to 19 bytes (the 32-way jump-table landing blocks disappear in favor of one indexed load); net code+data across a converted pair is ~48% smaller. A synthetic dispatch microbenchmark (random dim per call, to avoid branch-predictor warmup) shows ~5.8x faster selection - real, but this call happens once per index build, never per query, so it has no effect on distance-kernel throughput.
Explain what each template argument at a dispatch_by_residual/ dispatch_by_sve_residual call site actually represents (Dist, Chunk, Selector), with a worked example, instead of describing the helper only in terms of the now-deleted CHOOSE_IMPLEMENTATION macro it replaced. Also fixes a pre-existing clang-format drift in the lambda bodies.
Collaborator
Author
|
✅ `make check-format` passes clean on this branch. |
5 tasks
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.
Summary
C1..C64/CASES4..64/CHOOSE_IMPLEMENTATION/SVE_CASE/CHOOSE_SVE_IMPLEMENTATIONmacro pyramid inimplementation_chooser.hwith two small C++20 helpers in the newspaces/functions/residual_dispatch.h:dispatch_by_residual<Dist, Chunk>(dim, selector)anddispatch_by_sve_residual<Dist>(dim, chunk, selector). Both build astatic constexprlookup table once, at compile time, and index it withdim % Chunk— same O(1) shape as the switch it replaces, but the chunk size is stated exactly once (the helper's own template argument), so it can't drift from a separately-written modulus at the call site the way two independent literals could.Choose_*function bodies across all 21spaces/functions/*.cppfiles (x86: SSE/SSE3/SSE4/AVX/AVX2 family/F16C/AVX512 family; ARM: NEON family, SVE/SVE2/SVE_BF16) converted.implementation_chooser.h/implementation_chooser_cleanup.hdeleted — zero remaining references anywhere.Choose_*/GetDistFunc*names and signatures are unchanged, sotests/unit/test_spaces.cppandtests/benchmark/spaces_benchmarks/bm_spaces.hneeded zero changes. Every leaf SIMD kernel is untouched.SPACES-REFACTOR-PLAN.md/SPACES-REFACTOR-LOG.mdin this repo). R2 (replacing the#ifdef-nested runtime feature-check cascades inIP_space.cpp/L2_space.cppwith a declarative dispatch table) is stacked on top of this in a separate PR.Verification
-Werror -Wall -std=gnu++20 -O2, e.g.-mavx512f -mavx512bw -mavx512vl -mavx512vnniforAVX512F_BW_VL_VNNI.cpp) — clean, no warnings. The 7 ARM files (NEON/SVE families) converted with the same pattern but could not be compiled on this dev box (no aarch64/SVE cross-toolchain available) — needs a real ARM build + test pass before merge.make unit_test CTEST_ARGS="-R Spaces"— 1442/1442 passed, 0 failed, on this machine's real detected x86 tiers.BF16_IP/BF16_L2conversion): per-wrapper code drops from 528 to 19 bytes — the 32-way jump-table landing blocks disappear in favor of one indexed load into a 256-bytestatic constexprtable. Net code+data across a converted pair: ~48% smaller. Section-by-section breakdown confirms the whole-.osize increase some builds show is 100% DWARF debug info for the longer template/lambda names (stripped in production); real shipped.text+.rodata+.data.rel.roshrank.dimper call to defeat branch-predictor warmup) shows ~5.8x faster dispatch selection (15.6ns → 2.7ns/call). Real and reproducible, but this call happens exactly once per index build (components_factory.h:23), never per query — confirmed viabm_spaces.h's own structure, which callsChoose_*once in benchmark setup, outside the timed loop. So: no effect on distance-kernel query throughput (expected and confirmed — kernels are byte-identical), real effect on the one-time dispatch cost.Test plan
make unit_test CTEST_ARGS="-R Spaces"— 1442/1442 passedfunctions/*.cppwith exact release flagsmake check-format/ CI format check