refactor(dispatch): Generate the dispatch surface from a single declaration - #372
Draft
ahuber21 wants to merge 6 commits into
Draft
refactor(dispatch): Generate the dispatch surface from a single declaration#372ahuber21 wants to merge 6 commits into
ahuber21 wants to merge 6 commits into
Conversation
Contributor
Author
|
@copilot new or modified files should have |
Contributor
This was referenced Aug 24, 2026
The set of distance kernels compiled ahead of time -- extents x ISA levels
-- was written out by hand in every place that needed it: three extern
template blocks, two per-arch translation units, the `supported_dim_list`
array, and 48 near-identical `SPEC struct` lines in the instantiation
macros. Adding an extent meant editing all of them and hoping none was
missed. One had been: `euclidean.h` was missing d=160 for AVX2 (fixed in
the preceding commit), which silently made consumers instantiate that
kernel locally at their own -march.
Declare the surface once, in `cmake/dispatch-surface.cmake`:
set(SVS_SUPPORTED_DIMS 64 96 100 128 160 200 512 768)
set(SVS_ISA_LEVELS
"AVX2|haswell|avx2"
"AVX512|cascadelake|avx512"
)
`cmake/generate-dispatch-surface.cmake` validates it and writes
`include/svs/core/distance/dispatch_surface.h`, which exports
`SVS_FOR_EACH_SUPPORTED_DIM(M)`, `SVS_FOR_EACH_DISPATCH_TARGET(M)` and
`SVS_SUPPORTED_DIM_COUNT`. Everything that used to spell the list out now
loops over one of those. 108 hand-written instantiation lines become 0.
Type pairs stay in C++, in `multi-arch/x86/preprocessor.h`. A pair exists
because an implementation exists for it -- sometimes a hand-written one --
so the list belongs beside those implementations, not in the build system.
`svs::Dynamic` is appended automatically and cannot be listed: it is what
serves every dimensionality without a fixed-extent kernel, and the library
is incorrect without it.
The generated header is committed as well as generated. The build always
compiles against the build-tree copy, placed ahead of the source include
directory, and installs it over the committed one; the committed copy is
refreshed only when the declaration is the default, so overriding the
surface for a one-off build cannot rewrite the tree. Committing it keeps a
bare `-I include` compile working without CMake -- which the downstream
repository relies on, since it compiles `multi-arch/x86/{avx2,avx512}.cpp`
by path with its own CMake.
No behaviour change: the static library exports the same 864 symbols with
the same sizes, and the two arch objects are symbol-identical before and
after, both here and in the downstream build. `[distance]` passes
(134402115 assertions, 13 test cases).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every comment this branch adds now says what the code cannot say for itself and stops there. The block comments that restated the surrounding code, or spent five lines on a hazard that takes two, are gone; the hazards themselves stay, each naming its failure mode. Comment-only. The non-comment diff against the previous tip is empty.
A kernel that is missing its `extern template` declaration does not
produce an error. The consumer instantiates it locally instead, from the
generic primary template -- and in a baseline consumer translation unit
the vectorized partial specializations are not even visible, since they
are guarded on SVS_AVX2 / SVS_AVX512_F. So the consumer silently gets a
scalar loop where the library has a vectorized kernel, compiled at
whatever -march the consumer happens to use. That is the bug that shipped
for L2 at d=160 with AVX2.
Nothing could catch it, because nothing referenced the whole surface at
once. This adds a consumer that does: tests/multi-arch/x86/link_probe.cpp
names every kernel the surface declares -- every (extent, ISA level) pair,
every element-type pair, all three distances -- and nothing else. It is
compiled at -march=x86-64, like an arbitrary consumer of the headers, and
two tests are run against it:
dispatch_surface_probe calls every kernel whose ISA level this host
satisfies, so a kernel compiled beyond what
its level guarantees faults here
dispatch_surface_linkage reads the object's symbol table and requires
the kernels it references to be exactly the
kernels the library defines
The linkage check is host-independent and covers the whole surface
everywhere; the run covers only what the host can reach.
On the default surface the two sets match exactly at 864 kernels, and on
the reduced surface used by the non-default-surface CI job, at 288. All
three failure modes were confirmed to fire: dropping the L2 extern block
reports 288 kernels instantiated by the probe itself, and checking against
an archive missing the AVX-512 translation unit reports its 432 kernels as
declared but never instantiated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"9 extents (8 fixed + svs::Dynamic) x 2 ISA levels" says nothing about which extents, which levels, or what instruction budget each level compiles at, so reading the log gave no way to tell a correct surface from a plausible one. Also name the AVX_AVAILABILITY enumerators that are not in the surface, since that is the question the old count invited and could not answer: NONE is dispatched to but has no translation unit, so every consumer instantiates its kernels itself, at the consumer's own -march. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four checks, each closing a failure mode the link probe cannot see. dispatch_surface_declaration derives what the library must contain from the three hand-written sources -- the extent list and ISA levels, the type-pair lists, and the AVX_AVAILABILITY enumerator order -- and never reads the generated header. The linkage check compares the archive against a probe built from that header, so a generator that dropped an extent would drop it from both and still agree; this one has nowhere to hide. It also checks the entry-point consumer, whose kernels must all come from the archive: one it defines itself is an extern declaration that is missing. dispatch_instructions_<level>, one test per ISA level, disassembles the level's object file and holds it to a budget table keyed by -march. A level guarantees only what its runtime predicate tests, so an instruction outside that budget faults on a host the dispatcher routes there -- and no symbol-table check can see it. dispatch_surface_execution is the only check that observes a kernel run rather than exist: a specialization lost behind an `#if` still links and still counts. It breaks on every level's kernel for one extent and confirms the run enters the level this host satisfies. Weaker levels are covered by hosts that satisfy only those. dispatch_entry_probe reaches the kernels through the entry points rather than by naming the Impl classes, which is what makes the consumer half of the declaration check meaningful. nm, objdump and gdb are each optional: a missing tool skips its tests rather than failing the build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ahuber21
force-pushed
the
dispatch/02-generate-surface
branch
from
August 24, 2026 10:28
c2d57c2 to
fe66e35
Compare
Answer the review on the declaration's maintenance story: cmake/dispatch-surface.cmake now states what to edit for an extent, a level, a type pair or an instruction budget, and why AVX_AVAILABILITY::NONE has no row. Move the four checker scripts to cmake/dispatch-checks/ with a README, and make the preprocessor.h type-pair comment stand without the refactoring for context. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The set of distance kernels compiled ahead of time -- extents x ISA levels -- was
written out by hand everywhere it was needed: three
extern templateblocks,two per-architecture translation units, and
supported_dim_list. Adding anextent meant editing six lists in agreement, and the preceding commit is what
happens when one of them disagrees.
cmake/dispatch-surface.cmakeis now the only place the extent list and the ISAlevels are written down. Everything else is derived from it:
include/svs/core/distance/dispatch_surface.h, which drives everyextern template, every explicit instantiation, andsupported_dim_listinstruction budget it is compiled at
The generated header is committed as well as generated, so that consuming the
headers with a bare
-I includeand no CMake keeps working.Configure now prints the surface in full rather than a count -- every extent,
and for each level its enumerator, its instruction budget and its translation
unit:
AVX_AVAILABILITY::NONEis a real hole and is now named as one: the entry pointsfall back to it, no translation unit compiles it, so every consumer builds those
kernels itself at whatever
-marchit happens to use. A baseline translationunit is its own change, not this one.
Seven
ctesttests replace what was a paragraph ofnmcommands in a reviewcomment. All of them derive their expectations from the declaration, so they hold
for an overridden surface too:
dispatch_surface_probe,dispatch_entry_probe-- consumers built at-march=x86-64. One names every kernel in the surface directly, the otherreaches them through the entry points; a declared-but-uninstantiated kernel is
an undefined symbol in the first, and a kernel the entry points cannot reach is
one in the second.
dispatch_surface_linkage-- the archive defines every kernel the probe names,nothing more, and the probe instantiated none of them itself: 864 kernels.
dispatch_surface_declaration-- the same 864, but counted from the threehand-written sources (the extent list, the type-pair macros, the enumerator
order) without consulting the generated header, so a generator bug has nowhere
to hide: 2 levels x 9 extents x 3 distances x {AVX2: 16, AVX512: 16} pairs.
dispatch_instructions_avx2,dispatch_instructions_avx512-- one per level,disassembling that level's object file against the instruction budget its
runtime predicate promises. The AVX2 object must contain
ymmand nozmm,mask or VNNI encodings; a level that emits what its predicate does not
guarantee is an illegal-instruction fault on a host the dispatcher considers
supported, which no symbol-table check can see.
dispatch_surface_execution-- the only check that observes a run rather thana symbol table:
gdbbreaks on every level's kernel for one extent and reportswhich one a call through the entry points actually enters. A specialization
that disappears behind an
#ifstill links and still counts.No behaviour change: the same 864 kernels, at the same instruction budgets, with
the same mangled names.
compute()is stillcompute(); nothing about the callchain or the runtime dispatch moves.
Part 2 of 4 of the ISA dispatching v2 milestone.