Make find_best_match selection explicit opt-in; add prefilter and SWAR32 finders - #351
Make find_best_match selection explicit opt-in; add prefilter and SWAR32 finders#351BrianPugh wants to merge 4 commits into
Conversation
…R32 finders
The core sources no longer auto-select the 64-bit desktop matcher on
x86_64/aarch64; every build system opts into its platform's measured
configuration instead (setup.py adds TAMP_USE_DESKTOP_MATCH=1 on 64-bit
hosts). New in common.h: the TAMP_ARMV7EM profile knob and the
TAMP_USE_{EMBEDDED,SWAR32,DESKTOP,PREFILTER}_MATCH selection, mutually
exclusive with TAMP_ESP32 (conflicts are a compile error, guarded by
must-not-compile entries in the new c-compile-matrix target).
New finders: first-byte prefilter (1.36x compression on Cortex-M7, the
ARMV7EM default) and an experimental 32-bit SWAR variant (opt-in;
measured 0.93x on M7). The desktop finder's EXTEND_MATCH macros are
replaced by a shared static-inline tamp_match_extend; output is
byte-identical to the previous implementation.
Tests: c-test now pins the desktop matcher (matching what wheels ship),
c-test-prefilter behaviorally covers the ARMV7EM matcher on host (new CI
leg), c-test-embedded keeps covering the portable scan.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
platform.machine() reports the OS architecture (AMD64 even under 32-bit Python on 64-bit Windows), so the win32 cibuildwheel legs were opted into TAMP_USE_DESKTOP_MATCH and failed to link MSVC's 64-bit-only _BitScanForward64 intrinsic. Gate on sys.maxsize > 2**32 as well (also covers linux32-personality containers), and add a clear #error in compressor_find_match_desktop.c for 32-bit MSVC targets so a wrong opt-in fails at compile rather than at link. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR makes find_best_match selection an explicit build-time opt-in (rather than auto-selected by architecture in core sources) and adds new match-finder variants plus CI coverage to keep the vendored C trio compiling and tested under the intended configurations.
Changes:
- Introduces explicit, mutually-exclusive match-finder selection flags in
common.h(including treatingTAMP_ESP32as a selection) and updates documentation accordingly. - Adds an inline first-byte prefilter matcher and a new experimental SWAR32 matcher, and refactors the desktop matcher to share common helper logic.
- Updates build/test tooling to pin the desktop matcher for
c-test, add ac-test-prefilterCI leg, and add a compile-matrix target.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tamp/_c_src/tamp/compressor.c |
Adds shared SWAR helper utilities and implements the inline prefilter matcher; switches selection logic to explicit flags. |
tamp/_c_src/tamp/compressor_find_match_swar32.c |
New experimental 32-bit SWAR match finder included by compressor.c when selected. |
tamp/_c_src/tamp/compressor_find_match_desktop.c |
Refactors desktop matcher to use shared helpers defined in compressor.c. |
tamp/_c_src/tamp/common.h |
Adds “Platform performance tuning” section and enforces mutually-exclusive match-finder selection flags. |
setup.py |
Opts Cython wheels into TAMP_USE_DESKTOP_MATCH=1 on 64-bit hosts to keep shipped behavior unchanged. |
Makefile |
Pins c-test to desktop matcher, adds c-test-prefilter, and adds c-compile-matrix compile coverage. |
CLAUDE.md |
Updates repository guidance to match the new explicit selection/standalone vendoring contract. |
.github/workflows/tests.yaml |
Adds CI leg to run make c-test-prefilter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- compressor_find_match_swar32.c: #error on toolchains without __builtin_ctz (MSVC) so a wrong opt-in fails clearly at compile - c-compile-matrix: add the -DTAMP_USE_SWAR32_MATCH=1 configuration - Makefile: embedded/prefilter test_runner.o rules now list test_stream.c/test_stream_filesystems.c (test_runner.c #includes them; the embedded rule predates this PR with the same gap) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The shared helper block compiles before the finder files, so a 32-bit MSVC opt-in previously died on an unknown-identifier error at _BitScanForward64 — before desktop's #error (and prefilter had no guard at all). Also rename HAS_ZERO_BYTE in a comment to the helper's actual name, tamp_has_zero_byte. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Closing in favor of splitting this into four smaller PRs — the four concerns here are independent, and bundling them meant the one part with consequences for existing consumers (the selection change) was reviewed alongside 350 lines of new matcher code. The split, in dependency order:
Nothing was dropped or changed in the split: The review feedback on this PR (the win32 wheel gate, the shared Each branch was verified independently: all three C test legs 35/35 under ASan/UBSan, plus a per-branch differential confirming compressed output is byte-identical across every finder over the first 100 KB of enwik8 at each window size 8..15. |
Part 1 of 5 splitting the
stm32h7-optimizationsperformance campaign into reviewable pieces (compressor match finders → decompressor fast paths → fuzz coverage → QEMU profiler → device harnesses). The original branch remains as the experiment-history archive; the stack's final tree is byte-identical to it.What this does
The core sources no longer auto-select the 64-bit desktop matcher via
#if __x86_64__ ...— the vendored trio now defaults to portable code everywhere, and each build system opts into its platform's measured configuration.setup.pyopts wheels intoTAMP_USE_DESKTOP_MATCH=1on 64-bit hosts, so shipped wheels are unchanged.common.h"Platform performance tuning" section: theTAMP_ARMV7EMprofile knob and theTAMP_USE_{EMBEDDED,SWAR32,DESKTOP,PREFILTER}_MATCHselection. Selections are mutually exclusive includingTAMP_ESP32(the espidf component providesfind_best_matchvia extern); conflicts are a compile error, guarded by must-NOT-compile entries in the newc-compile-matrixtarget.TAMP_ARMV7EMdefault; ~0.5x on out-of-order 64-bit hosts, hence not the desktop choice).static inline tamp_match_extend; compressed output is byte-identical to the previous implementation (verified old-vs-new on enwik8 + text + random-compressible inputs across window bits 8–15).c-testnow pins the desktop matcher (what wheels ship), newc-test-prefilterleg behaviorally covers the ARMV7EM matcher on host (wired into CI),c-test-embeddedkeeps covering the portable scan.Behavior note for vendoring users
Projects that vendor
common.c/compressor.c/decompressor.cand compiled on x86_64/aarch64 previously got the desktop matcher automatically; they now get the portable scan (correct output, slower compression) unless they defineTAMP_USE_DESKTOP_MATCH=1. This is the intended contract — the core never self-selects architecture-specific code — and is documented incommon.hand CLAUDE.md; worth a release-note line.Verification
c-compile-matrixpasses, including the two must-NOT-compile conflict configs.main's implementation (24/24 input×window combinations byte-identical).🤖 Generated with Claude Code