Make find_best_match selection an explicit build-system opt-in - #362
Open
BrianPugh wants to merge 3 commits into
Open
Make find_best_match selection an explicit build-system opt-in#362BrianPugh wants to merge 3 commits into
BrianPugh wants to merge 3 commits into
Conversation
The core C sources auto-detected 64-bit hosts and silently included compressor_find_match_desktop.c. That put an architecture policy decision inside the vendored trio, where consumers that are not setup.py (espidf, wasm, the MicroPython native module, users who vendor the three .c files) had no say in it and no way to see what they were getting. Invert it: the core now always defaults to the portable scan, and each build system opts into the configuration measured for its platform, the way TAMP_ESP32 already worked. setup.py sets TAMP_USE_DESKTOP_MATCH=1 on 64-bit interpreters, mirroring the previous wheel behavior. - common.h gains a "Platform performance tuning" section documenting the contract, with TAMP_USE_EMBEDDED_MATCH / TAMP_USE_DESKTOP_MATCH defaulting to 0. - Selections are mutually exclusive, enforced with an #error that counts TAMP_ESP32 too: the espidf component supplies find_best_match via extern and the dispatch checks it first, so pairing it with an explicit TAMP_USE_*_MATCH would otherwise silently drop the requested finder. - The setup.py gate tests sys.maxsize as well as platform.machine(), which reports the OS architecture: under a 32-bit interpreter on a 64-bit host (win32 cibuildwheel legs, linux32-personality containers) machine() still says AMD64, and the desktop matcher needs MSVC's 64-bit-only _BitScanForward64. The desktop finder #errors on 32-bit MSVC targets as a backstop. - c-test now compiles the same finder the wheels use, so the desktop path stays covered once it is no longer auto-selected; c-test-embedded keeps covering the portable path. - New c-compile-matrix target (a c-test prerequisite) compiles the vendored trio under each documented flag combination, and asserts the two conflicting match selections fail to compile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR moves find_best_match implementation selection out of the vendored core C sources and into explicit build-system opt-in flags, so consumers that vendor common.c/compressor.c/decompressor.c control which matcher they get (portable by default), while preserving existing wheel behavior via setup.py.
Changes:
- Adds and documents explicit matcher-selection flags in
common.h, including mutual-exclusion enforcement (countingTAMP_ESP32as a selection). - Updates
compressor.cto include the desktop matcher only whenTAMP_USE_DESKTOP_MATCH=1, making the portable scan the default everywhere. - Updates build tooling to keep current behavior/coverage:
setup.pyopts 64-bit builds into the desktop matcher;Makefilepinsc-testto desktop and adds ac-compile-matrixcompilation guard target.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tamp/_c_src/tamp/compressor.c | Switches desktop-matcher selection from architecture auto-detect to explicit TAMP_USE_DESKTOP_MATCH. |
| tamp/_c_src/tamp/compressor_find_match_desktop.c | Adds a compile-time failure for unsupported MSVC targets when opted into the desktop matcher. |
| tamp/_c_src/tamp/common.h | Documents platform tuning contract and enforces mutually-exclusive matcher selections. |
| setup.py | Opts 64-bit CPython builds into the desktop matcher to preserve wheel behavior. |
| Makefile | Pins c-test to desktop matcher and adds c-compile-matrix to compile-check documented flag combinations. |
| CLAUDE.md | Updates documentation to reflect the new “build system opts in” matcher selection policy. |
Suppressed comments (1)
Makefile:584
- The must-NOT-compile checks in c-compile-matrix also use
$(CC) rather than $ (CTEST_CC), which can makemake c-testdepend on a different compiler than the one used to build/run the tests.
if $(CC) -O2 -Wall -Itamp/_c_src $$cfg -c tamp/_c_src/tamp/common.c \
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @set -e; for cfg in $(C_COMPILE_MATRIX_CONFIGS); do \ | ||
| echo "c-compile-matrix: $$cfg"; \ | ||
| for src in tamp/_c_src/tamp/common.c tamp/_c_src/tamp/compressor.c tamp/_c_src/tamp/decompressor.c; do \ | ||
| $(CC) -O2 -Wall -Itamp/_c_src $$cfg -c $$src -o build/c_compile_matrix.o.tmp; \ |
Comment on lines
+16
to
+20
| /* Fail loudly on targets that can't provide the 64-bit intrinsics (e.g. MSVC | ||
| * x86: _BitScanForward64 exists only on x64/ARM64 targets, and would otherwise | ||
| * surface as an unresolved-symbol link error). Selection is a build-system | ||
| * decision, so a wrong opt-in must be a clear compile error. */ | ||
| #if defined(_MSC_VER) && !defined(_M_X64) && !defined(_M_ARM64) |
Comment on lines
52
to
+56
| # Force embedded find_best_match implementation on desktop (for testing) | ||
| if os.environ.get("TAMP_USE_EMBEDDED_MATCH", "0") == "1": | ||
| print("Using embedded find_best_match implementation") | ||
| define_macros.append(("TAMP_USE_EMBEDDED_MATCH", "1")) | ||
| elif sys.maxsize > 2**32 and platform.machine().lower() in ("x86_64", "amd64", "arm64", "aarch64"): |
Comment on lines
+238
to
+240
| - `TAMP_USE_EMBEDDED_MATCH=1` - the portable `find_best_match` (selections are | ||
| mutually exclusive, including `TAMP_ESP32`; conflicts are a compile error) | ||
| - `TAMP_USE_DESKTOP_MATCH` - 64-bit SWAR for 64-bit hosts |
Count each selection as `(FLAG != 0)` instead of summing the raw macro values. This keeps the check correct for a flag defined to something other than 0/1 (e.g. `-DTAMP_USE_DESKTOP_MATCH=2` no longer trips a false conflict), and parenthesizing each term makes the sum immune to `+` binding tighter than `&&` should a future selection flag be defined as an unparenthesized expression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
compressor.c's dispatch is `#if TAMP_ESP32 / #elif TAMP_USE_DESKTOP_MATCH / #else portable` and never reads TAMP_USE_EMBEDDED_MATCH, so listing it alongside "desktop" implied a selection it does not make. 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.
First of four branches that #351 was split into for review. This one is the build-system policy change; the other three are the matcher implementations that build on it. #351 itself stays open for now — it still points at the unsplit branch, and the four branches' trees together are byte-identical to it.
What changes
The core C sources auto-detected 64-bit hosts and silently
#include'dcompressor_find_match_desktop.c:#elif (defined(__x86_64__) || defined(__aarch64__) || defined(_M_X64) || defined(_M_ARM64)) && !TAMP_USE_EMBEDDED_MATCHThat puts an architecture policy decision inside the vendored trio, where consumers that aren't
setup.py— espidf, wasm, the MicroPython native module, anyone who vendors the three.cfiles — had no say in it and no easy way to see what they were getting.This inverts it: the core always defaults to the portable scan, and each build system opts into the configuration measured for its platform, the way
TAMP_ESP32already worked.setup.pysetsTAMP_USE_DESKTOP_MATCH=1on 64-bit interpreters, preserving existing wheel behavior.Details
common.hgains a "Platform performance tuning" section documenting the contract.TAMP_USE_EMBEDDED_MATCHandTAMP_USE_DESKTOP_MATCHboth default to 0.#errorthat countsTAMP_ESP32as a selection: the espidf component suppliesfind_best_matchviaexternand the dispatch checks it first, so pairing it with an explicitTAMP_USE_*_MATCHwould otherwise silently drop the requested finder.setup.pygate testssys.maxsizeas well asplatform.machine().machine()reports the OS architecture, so under a 32-bit interpreter on a 64-bit host (win32 cibuildwheel legs, linux32-personality containers) it still saysAMD64— and the desktop matcher needs MSVC's 64-bit-only_BitScanForward64. The desktop finder#errors on 32-bit MSVC targets as a backstop. Both of Make find_best_match selection explicit opt-in; add prefilter and SWAR32 finders #351's win32 CI fixes are folded in here.c-testnow compiles the same finder the wheels use, so the desktop path keeps behavioral coverage once it's no longer auto-selected.c-test-embeddedcontinues to cover the portable path.c-compile-matrixtarget (ac-testprerequisite) compiles the vendored trio under each documented flag combination, and asserts that the two conflicting match selections fail to compile.Scope
No compression behavior changes and no measured performance change on any platform: this only moves which implementation gets selected from the source to the build system, and the selection each existing build system makes is the same one it made before.
The one behavior difference worth naming: a consumer that compiles
compressor.con x86_64/aarch64 without setting any flag now gets the portable matcher where it previously got the desktop one. That is the intended point of the change, but it is a real change for anyone vendoring the sources directly, so it's called out inCLAUDE.md.Verification
make c-testandmake c-test-embedded: 35/35 each under ASan/UBSan.make c-compile-matrix: all documented flag combinations compile; both conflicting selections correctly rejected.