refactor(dispatch): Generate and validate the dispatch surface from a single declaration

[ahuber21]

The set of distance kernels compiled ahead of time is declared once, in
cmake/dispatch-surface.cmake, and the generated header, the per--march translation
units and the ctest checks all derive from it. Configure-time validation rejects a
malformed declaration, and VNNI becomes its own ISA level so that each level's -march
budget matches what it promises.

Gotchas:

• This PR now contains feat(dispatch): Validate the dispatch-surface declaration at configure time #373 and fix(dispatch): Make VNNI its own ISA level and lower AVX512 to skylake-avx512 #375. They were stacked on it and the review cleanup
  touched the same files, so redoing that work here rather than folding them in would have
  meant resolving the same conflicts twice. Both are closed as merged here. The diff is
  large for that reason, not because the change grew.
• The checkers moved. cmake/dispatch-checks/ is now
  tests/multi-arch/dispatch-checks/. They are test drivers; only generation stays under
  cmake/.
• These checks are add_test, not Catch2. link_probe.cpp exists to fail to link
  when a declared kernel is uninstantiated, and entry_probe.cpp gets driven under gdb —
  neither works from inside the single tests binary. Porting the four cmake -P checkers
  to Catch2 is a follow-up.
• include/svs/core/distance/dispatch_surface.h is generated but committed, so a bare
  -I include compile works without cmake. CI fails if it goes stale, and asserts that a
  build with an overridden surface leaves it alone.
• Configure now rejects two ISA levels sharing one -march. Previously accepted, and it
  shipped a fault: the weaker level got compiled with instructions its runtime predicate does
  not guarantee, and the instruction checker missed it because it looks the budget up by
  -march. Not a review point; found while probing whether these checks can still fail.
• Three points from the review were declined — Python for the checkers, dropping the
  declaration checker as redundant, and parallel lists for the ISA table. Reasons are in
  the review thread.

[ahuber21]

@copilot new or modified files should have 2026 in their copyright header. Fix

[Copilot]

@copilot new or modified files should have 2026 in their copyright header. Fix

Updated the headers in 408513c.

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[Copilot]

Pull request overview

Centralizes x86 distance-kernel extents and ISA levels into one generated dispatch declaration.

Changes:

• Generates kernel declarations, instantiations, and supported dimensions from one CMake surface.
• Adds linkage, instruction, declaration, and runtime dispatch checks.
• Installs the generated header and documents maintenance workflows.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
CMakeLists.txt	Installs the generated surface header.
cmake/AGENTS.md	Documents dispatch ownership.
cmake/dispatch-surface.cmake	Declares extents and ISA levels.
cmake/generate-dispatch-surface.cmake	Validates and generates the surface.
cmake/multi-arch.cmake	Builds ISA objects from the declaration.
cmake/templates/dispatch_surface.h.in	Defines the generated-header template.
cmake/dispatch-checks/README.md	Documents dispatch checks.
cmake/dispatch-checks/check-dispatch-declaration.cmake	Validates declared kernel counts.
cmake/dispatch-checks/check-dispatch-execution.cmake	Verifies runtime routing.
cmake/dispatch-checks/check-dispatch-instructions.cmake	Inspects ISA instruction budgets.
cmake/dispatch-checks/check-dispatch-linkage.cmake	Verifies kernel linkage.
include/svs/core/distance/cosine.h	Generates cosine extern templates.
include/svs/core/distance/dispatch_surface.h	Commits the default generated surface.
include/svs/core/distance/distance_core.h	Generates supported dimensions.
include/svs/core/distance/euclidean.h	Generates L2 extern templates.
include/svs/core/distance/inner_product.h	Generates IP extern templates.
include/svs/multi-arch/x86/avx2.cpp	Generates AVX2 instantiations.
include/svs/multi-arch/x86/avx512.cpp	Generates AVX512 instantiations.
include/svs/multi-arch/x86/preprocessor.h	Defines reusable instantiation macros.
tests/CMakeLists.txt	Enables multi-architecture tests.
tests/multi-arch/CMakeLists.txt	Registers dispatch probes and checks.
tests/multi-arch/x86/entry_probe.cpp	Exercises public dispatch entry points.
tests/multi-arch/x86/host_levels.h	Models host ISA predicates.
tests/multi-arch/x86/link_probe.cpp	References every declared kernel.

---

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

[ahuber21]

Will be fixed in 375 -- and all PRs will be merged together as a stack.

[rfsaliev]

The change is pretty big with high review/maintenance cost.
It seems like the huge AI generated change to be reviewed by AI.
Scripts should be simplified and well structured.

[rfsaliev]

As I understand, this file is autogenerated from dispatch_surface.h.in.
Why do we need to track it in repository?
Suggesting to remove the file from repository but generate it in binary directory during config/build.

[ahuber21]

I added a default version for reference. If someone researched the code on GH only, or on a fresh checkout without prior compilation there are no missing files.

[rfsaliev]

Seems like files in this directory intended for tests.
IMHO it makes sense to move them to /test

[rfsaliev]

Seems like the cmake code here is pretty complicated.
Writing it in form of straightforward script leads high maintenance costs.
Please, modify the code to make it more structured, e.g. split to functions.

[ahuber21]

Thanks — this is a useful review and most of it is going in. One piece of context that changes several items: #372 is the middle of a stack, and #373/#375 already resolve some of what you raised. To make that reviewable rather than confusing, I've folded #373 and #375 into this PR, so what you see now is the whole dispatch-surface change. The diff is correspondingly larger; #373 and #375 are closed as merged here.

Taken

2 — shared checker helpers. Done. svs_require, svs_require_files, svs_nm_symbols, svs_report_first now live in one lib.cmake. The two divergent svs_symbols() definitions and the third inlined copy in the execution check are all one function now.

7 — checker location. Done. cmake/dispatch-checks/ moved to tests/multi-arch/dispatch-checks/. Only generation stays under cmake/.

4 — one ISA-level parser. Done. On the count: there are six string(REPLACE "|" ";" ...) sites, but only five parse the ISA-level/TU-spec encoding — those five now go through svs_parse_isa_level() / svs_parse_tu_spec(), which fail on a wrong field count instead of silently producing empty variables. The sixth, in check-dispatch-instructions.cmake, parses a different three-field table (<-march>|<required classes>|<forbidden classes>) with one reader and no duplicate, so it stays local; routing it through the ISA-level parser would be wrong.

5 — manifest instead of header re-parsing. Partly done, and I'd rather be precise about which part. The generator now emits dispatch_surface.manifest.cmake with the extent list and the ISA-level enumerators, and the declaration checker reads that instead of include()-ing the declaration and re-deriving both. The AVX_AVAILABILITY read from distance_core.h stays, for the reason under item 3: it is what derives the mangled digit instead of assuming it.

The SVS_TYPE_PAIRS_ macro parser — line-continuation folding and one-level alias following — is still there, and it is the most fragile thing left. It can't move to the manifest, because the generator doesn't know the count either: the pairs are a C++ preprocessor macro, so cmake would just be parsing the same header from a different file. The right fix is to have the probe print the count under --report, letting the preprocessor do the counting, and read a number instead of a regex. That's a follow-up I'd rather do properly than rush into this PR.

Not taken, with reasons

1 — port the checkers to Python. Declining, on your own escape clause ("if a Python dependency is unacceptable"). SVS's C++ build and test suite have no Python dependency today: no find_package(Python3) anywhere, bindings/python/ is isolated behind its own pyproject.toml and is never add_subdirectory'd, no .py file is invoked by any CTest test, and none of the C++ CI workflows set up Python. Making the C++ test suite need a Python interpreter is a bigger change than the duplication it removes, and 2/4/5 together take out most of what motivated it.

I do agree with the diagnosis, though — emulating dicts via set(prefix_HAVE_${key} ...) was the worst pattern in here, and item 5 is what removes the need for it.

3 — the hardcoded mangling. I tried this and measured it, and it doesn't work. The plan was to match nm -C output nstead, but demangling renders an enum template argument as (svs::distance::AVX_AVAILABILITY)1 — a number, not the numerator name. So the positional digit survives demangling, and 18446744073709551615 survives it too, because svs::Dynamicis aconstexpr size_t` and demangling prints its value. Of the three things you named, only ength-prefixing goes.

That isn't worth the trade: it swaps a dependency on the Itanium mangling, which is a published ABI, for a dependency on the demangler's own rendering, which is a libstdc++ implementation detail and less stable. Worse, the first cut of it deleted the code that reads the enum out of distance_core.h and hardcoded "NONE is 0, so declared levels start at 1" instead — which turns a reordered enum from a hard error into silently wrong matching. Keeping the header parse: it is the only thing that derives the digit rather than assuming it.

So the mangling stays, and I'd rather say that than claim the item is closed. What did improve is that it is now concentrated in one file with the enum read from the source of truth, instead of also being spread across a surface re-parse.

3, second half — "consider whether both checkers are needed." This one I think is wrong. The linkage check compares the probe's references against the archive's definitions, but both are generated from the same header — so a generator that drops an extent drops it from both, and they still agree. The declaration check derives its expectation from three hand-written sources (dispatch-surface.cmake, preprocessor.h, distance_core.h) and is the only thing that catches that class of bug. tests/multi-arch/dispatch-checks/README.md says as much. Reducing it to a count would drop the coverage the check exists for.

4, second half — parallel lists instead of the |-joined encoding. Declining. SVS_ISA_LEVEL_NAMES/_ARCHS/_INFIXES replaces "this row has three fields", which a parser can validate and fail on, with "these three lists are equal length and correspond positionally", which nothing checks and which breaks silently when someone appends to one. The parser keeps the error where the mistake is.

6 — split generate-dispatch-surface.cmake into functions. Largely already done, in #373 (now part of this PR): validation moved out into validate-dispatch-surface.cmake and the generator shrank by ~100 lines. Your point isn't fully answered — that new file is still straight-line — but it has 14 accept/reject fixtures pinning its error messages, and restructuring it for readability alone risks that coverage. On the duplicate foreach: the generator only has one pass over SVS_ISA_LEVELS; the duplication was across files, between it and the validator, which is what item 4 removes. I'd rather leave the rest and let you re-raise it.

7 — obj_name keyed by arch. Already fixed in #375 (now part of this PR): it reads set(obj_name ${infix}_obj). Worth flagging that #375 is exactly the case you predicted — it adds a third level, AVX512_VNNI|cascadelake|vnni, alongside AVX512|skylake-avx512|avx512, so the collision was one shared -march away from being real.

7 — committed generated header. Also already addressed in #373 (now part of this PR). .github/workflows/dispatch-surface.yml has a job that configures and then fails if include/svs/core/distance/dispatch_surface.h comes out with a diff — the staleness gate you asked for — plus a second job asserting that a build with an overridden surface does not rewrite the committed copy. Keeping the committed file itself, for the reason in the inline thread: a fresh checkout or a bare -I include compile has no missing headers.

7 — comment volume in dispatch-surface.cmake. Keeping this one as is, deliberately. That file is the single lever for configuring the surface and is meant to be read more often than edited; the prose-to-code ratio is the point rather than an accident. The other files' comments are within normal range — for the record, the measured figures are 92% comments for dispatch-surface.cmake and 22–28% for the checkers, and multi-arch.cmake is 37%, not over half.

Found while doing this

Not one of your points, but worth flagging since it's in the same file. The validator rejected a duplicate ISA level and a duplicate TU infix but not a duplicate -march, and that one ships a fault: declaring AVX512|cascadelake|avx512 compiles the AVX512 object with VNNI while the AVX512 runtime predicate still admits hosts without it. The instruction checker can't catch it — it looks the budget up by -march, so it checks that object against the cascadelake row, which requires VNNI and forbids nothing, and passes. Configure was silent. There's now a duplicate--march check alongside the other two and a fixture pinning it.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated 4 comments.

Suppressed comments (3)

tests/multi-arch/dispatch-checks/check-dispatch-declaration.cmake:1

• The error message references ${SVS_SURFACE_FILE}, but this script is driven by SVS_MANIFEST and never defines SVS_SURFACE_FILE, so the message will be missing the file context. Use ${SVS_MANIFEST} (or reword to “declared in the manifest”) so failures point to the correct input.
  tests/multi-arch/CMakeLists.txt:1
• This else() triggers when either svs_nm or svs_gdb is missing, but the status message only mentions gdb. Make the message reflect the actual condition (e.g., mention missing nm and/or gdb) so skipped-test diagnostics are accurate.
  tests/multi-arch/CMakeLists.txt:1
• This else() triggers when either svs_nm or svs_gdb is missing, but the status message only mentions gdb. Make the message reflect the actual condition (e.g., mention missing nm and/or gdb) so skipped-test diagnostics are accurate.

[ahuber21]

By the way, here is the verbose output what the new tests are doing.

# kernel linking
dispatch surface probe: called 918 kernels that computed a total distance sum of 76537.054688

# kernel entry, single ISA (the runner's ISA)
dispatch surface entry probe: called 432 kernels that computed a total distance sum of 36017.449219

# every declared kernel is defined in the library, and none of them in the consumer
dispatch linkage: 918 kernels declared, instantiated and reachable; none instantiated by the consumer

# the built surface counted from the declaration instead of from a probe
dispatch declaration: 918 kernels required by the manifest (3 levels x 9 extents x 3 distances x type pairs per level {AVX2: 16, AVX512: 16, AVX512_VNNI: 2}); the archive defines exactly those and the entry points reach all of them

# instruction budget per level: no wider vectors than the level's predicate guarantees
dispatch instructions: level AVX2 at -march=haswell has 21062 ymm, and no zmm, mask, vnni
dispatch instructions: level AVX512 at -march=skylake-avx512 has 19338 zmm, and no vnni
dispatch instructions: level AVX512_VNNI at -march=cascadelake has 3122 zmm, 456 vnni, and nothing forbidden

# runtime dispatch, observed under gdb: which level a call really enters
dispatch execution: a call to L2 at extent 64 entered AVX_AVAILABILITY level 3, the highest this host satisfies

I agree the cmake is still a bit bulky but it's quite self-contained. It delivers a status quo that is extremely well tested and if it ever breaks (for example because of the mangling assumptions you criticized), it's just a test failure, not a defect. We can update and fix the test. But I actually doubt the tests will start failing in the foreseeable future. claude explained quite convincing why the Itanium name scheme should be stable.

So what's left are the changes to existing headers and implementations. Those are very small. It's mostly just removing the now redundant instantiations and some updated dispatching logic because of the VNNI change.

So, honestly, I wouldn't be too concerned about "large AI generated PR".