From c48c69fbd2b22ae53e06b639872fd9de80395f77 Mon Sep 17 00:00:00 2001 From: Ryan Butler Date: Tue, 1 Sep 2026 11:49:00 -0500 Subject: [PATCH] fix(asr): guard THREAD_POWER_THROTTLING_STATE to feature-detect for MinGW Under MinGW-w64, winbase.h defines the ThreadPowerThrottling enum value (so SetThreadInformation(..., ThreadPowerThrottling, ...) resolves) but not the THREAD_POWER_THROTTLING_STATE struct or the THREAD_POWER_THROTTLING_EXECUTION_SPEED/THREAD_POWER_THROTTLING_CURRENT_VERSION macros ggml_thread_apply_priority() fills in, so the existing _WIN32_WINNT-only guard fails to compile under MinGW GCC 13.1.0 (Qt's mingw_64 kit). MSVC's Windows SDK already has these, so this is invisible on the CI/installer path. Narrows the guard to feature-detect the actually-missing macro rather than gate on the toolchain, so it self-resolves if mingw-w64 ever adds the struct and also covers MinGW-clang. Under MinGW this only forgoes a Windows-11 core-parking performance hint -- SetThreadPriority() immediately below is unconditional and still applies. Originally proposed as #4406 and closed there as an in-place vendored edit, before this tree's AETHERSDR-PATCHES.md exception convention existed. #4553 (2026-07-31) documented that convention here for a different fix in the same tree; this re-lands #4406's fix through it, recorded as the third local patch alongside the two Metal changes. Verified with a from-scratch MinGW build (Qt 6.11.2 mingw1310_64): ggml-cpu.c compiles clean and AetherSDR.exe links, with no separate apply/reverse patch step needed. --- third_party/whisper.cpp/AETHERSDR-PATCHES.md | 61 ++++++++++++++++--- third_party/whisper.cpp/AETHER_VENDORING.md | 34 +++++++---- .../whisper.cpp/ggml/src/ggml-cpu/ggml-cpu.c | 2 +- 3 files changed, 75 insertions(+), 22 deletions(-) diff --git a/third_party/whisper.cpp/AETHERSDR-PATCHES.md b/third_party/whisper.cpp/AETHERSDR-PATCHES.md index 0cdcd7dc0..b2765fae1 100644 --- a/third_party/whisper.cpp/AETHERSDR-PATCHES.md +++ b/third_party/whisper.cpp/AETHERSDR-PATCHES.md @@ -5,8 +5,9 @@ The source snapshot is pinned to ggml-org/whisper.cpp commit The tree is otherwise an exact — if trimmed — upstream snapshot; see [`AETHER_VENDORING.md`](AETHER_VENDORING.md) for what was removed. -AetherSDR carries two local changes, both in the ggml Metal backend and both -from the same fix (#4535, PR #4553): +AetherSDR carries three local changes: two in the ggml Metal backend, from the +same fix (#4535, PR #4553), and one in the ggml CPU backend, a MinGW build fix +(#4406) landed separately. 1. `ggml/src/ggml-metal/CMakeLists.txt`: adds `GGML_METAL_EMBED_LIBRARY_COMPILED`. Upstream's `GGML_METAL_EMBED_LIBRARY` embeds the merged kernel **source** and @@ -37,11 +38,46 @@ never invoked. It re-compiles on every cold-cache launch, and on Intel-GPU Macs it can live-lock indefinitely and freeze the GUI — that is #4535, measured at no completion in 75 minutes on a Radeon Pro 560X. +3. `ggml/src/ggml-cpu/ggml-cpu.c`: narrows the Windows-11 core-parking + throttle guard in `ggml_thread_apply_priority()` from + `#if _WIN32_WINNT >= 0x0602` to + `#if defined(THREAD_POWER_THROTTLING_CURRENT_VERSION) && _WIN32_WINNT >= 0x0602`. + + Under MinGW-w64, `winbase.h` defines the `ThreadPowerThrottling` enum value + (so the `SetThreadInformation(..., ThreadPowerThrottling, ...)` call below + still resolves) but not the `THREAD_POWER_THROTTLING_STATE` struct or the + `THREAD_POWER_THROTTLING_EXECUTION_SPEED` / `THREAD_POWER_THROTTLING_CURRENT_VERSION` + macros this block fills in — those are MSVC-Windows-SDK-only in this + toolchain's header set, so the bare `_WIN32_WINNT` guard fails to compile + under MinGW GCC 13.1.0 (Qt's `mingw_64` kit): + + ``` + ggml-cpu.c:2533:10: error: request for member 'StateMask' in something not a structure or union + ``` + + MSVC (CI/installer path) is unaffected either way, since the macro the guard + now also checks is already defined there. Feature-detecting the actually- + missing macro, rather than gating on the toolchain (`_MSC_VER`), means the + guard self-resolves the day mingw-w64 adds the struct, and also covers + MinGW-clang. Under MinGW this only forgoes a Windows-11 core-parking + performance hint — the unconditional `SetThreadPriority()` immediately below + still applies, so it's a narrower optimization loss, not a correctness + change. + + Originally proposed as PR #4406, which fixed the same compile error the + same way but was closed as an in-place vendored edit before this file's + exception convention existed for this tree. Re-landed once #4553 documented + that convention here. Checked upstream at the time of #4406: the bare guard + was (and remains, as of that check) present unfixed at HEAD of both + `ggml-org/ggml` and `ggml-org/whisper.cpp`, with no existing issue or PR — + so a `COMMIT` bump alone would not have picked up a fix. + ## Refreshing When refreshing whisper.cpp, first check whether upstream has adopted an -equivalent compiled-embed option; if it has, drop the corresponding local patch -in favour of it. Otherwise reapply both changes and confirm with +equivalent compiled-embed option for the two Metal changes; if it has, drop +the corresponding local patch in favour of it. Otherwise reapply both changes +and confirm with ```bash ctest --test-dir build -R asr_gpu_probe_test -V @@ -51,7 +87,16 @@ run with `AETHER_ASR_EXPECT_PRECOMPILED=1` on an Apple Silicon host — that asserts a Metal device initialized *and* that ggml logged the precompiled branch, which is what catches a silent reversion to the source embed. -The authoritative diff for either file is its git history -(`git log -p -- third_party/whisper.cpp/ggml/src/ggml-metal/`). No -checked-in `.patch` copy is kept: it would need hand-syncing on every edit, and -its context would not apply cleanly across an upstream bump anyway. +For the `ggml-cpu.c` guard, first check whether the exact line still reads +`#if _WIN32_WINNT >= 0x0602` at `ggml_thread_apply_priority()` — if upstream +has since fixed this itself (feature-detect or otherwise), drop the local +patch. Otherwise reapply the guard change and confirm with a MinGW +configure + build (`cmake --build` from a MinGW-w64 Ninja toolchain); a +regression here only shows up as a MinGW compile failure, not a test failure, +since MSVC and non-Windows builds never exercise this branch. + +The authoritative diff for any of these files is its git history +(`git log -p -- third_party/whisper.cpp/ggml/src/ggml-metal/` or +`.../ggml-cpu/ggml-cpu.c`). No checked-in `.patch` copy is kept for any of +them: it would need hand-syncing on every edit, and its context would not +apply cleanly across an upstream bump anyway. diff --git a/third_party/whisper.cpp/AETHER_VENDORING.md b/third_party/whisper.cpp/AETHER_VENDORING.md index 0852e734a..441f2f3c3 100644 --- a/third_party/whisper.cpp/AETHER_VENDORING.md +++ b/third_party/whisper.cpp/AETHER_VENDORING.md @@ -8,7 +8,7 @@ upstream `LICENSE` covers the bundled `ggml/` tree too — upstream ships one MI file for both. Keep this a pristine mirror: do **not** modify vendored sources in place unless -the change genuinely cannot live outside the tree. Two files currently do; +the change genuinely cannot live outside the tree. Three files currently do; every one is recorded in [`AETHERSDR-PATCHES.md`](AETHERSDR-PATCHES.md), and anything not listed there is a drift bug. @@ -52,24 +52,32 @@ fallback). `GGML_NATIVE=OFF` is forced for portable/Pi/CI binaries. ## Local patches (deviations from pristine upstream) -Two vendored files carry AetherSDR-local changes; the pristine-mirror rule above -holds for everything else. Both are described — with their rationale and the -refresh checklist — in [`AETHERSDR-PATCHES.md`](AETHERSDR-PATCHES.md), following -the same convention as `third_party/wdsp` and `third_party/smartsdr-dsp`: +Three vendored files carry AetherSDR-local changes; the pristine-mirror rule +above holds for everything else. Each is described — with its rationale and +the refresh checklist — in [`AETHERSDR-PATCHES.md`](AETHERSDR-PATCHES.md), +following the same convention as `third_party/wdsp` and +`third_party/smartsdr-dsp`: - `ggml/src/ggml-metal/CMakeLists.txt` — build-time kernel compilation (`GGML_METAL_EMBED_LIBRARY_COMPILED`). PR #4553, fixes #4535. - `ggml/src/ggml-metal/ggml-metal-device.m` — loads the embedded compiled metallib and clamps `props.has_tensor` / `props.has_bfloat` to the kernels it actually contains. PR #4553. - -Both are kept as thin as possible: the *policy* around them — required -toolchain, missing-toolchain behaviour, deployment target, shader language -version — lives in the top-level `CMakeLists.txt` and reaches the vendored tree -only as `GGML_METAL_*` variables, so a refresh has less to re-apply. +- `ggml/src/ggml-cpu/ggml-cpu.c` — narrows the Windows-11 core-parking + throttle guard to a feature-detect so it compiles under MinGW-w64, which + doesn't declare `THREAD_POWER_THROTTLING_STATE`. Fixes the same MinGW-only + compile break originally raised in #4406. + +The two Metal changes are kept as thin as possible: the *policy* around them — +required toolchain, missing-toolchain behaviour, deployment target, shader +language version — lives in the top-level `CMakeLists.txt` and reaches the +vendored tree only as `GGML_METAL_*` variables, so a refresh has less to +re-apply. The `ggml-cpu.c` change is a single preprocessor-guard edit with no +corresponding top-level policy to keep in sync. They must be re-applied after any refresh; the re-vendoring recipe below -otherwise silently reverts them and reintroduces #4535. +otherwise silently reverts them, reintroducing #4535 (Metal) and the MinGW +compile break (`ggml-cpu.c`). ## Re-vendoring / adding another GPU backend @@ -77,5 +85,5 @@ To add a different GPU backend (CUDA, Metal, …), **re-copy that backend's directory** from upstream at the pinned commit and turn its `GGML_` option ON (with the matching toolchain + CI runner). To refresh: clone upstream at `COMMIT`, re-run the same trim (keeping `ggml-cpu`, `ggml-blas`, `ggml-vulkan`), -and diff — then re-apply the two local patches (see **Local patches** above); a -clean diff plus exactly those two files is the expected end state. +and diff — then re-apply the three local patches (see **Local patches** above); +a clean diff plus exactly those three files is the expected end state. diff --git a/third_party/whisper.cpp/ggml/src/ggml-cpu/ggml-cpu.c b/third_party/whisper.cpp/ggml/src/ggml-cpu/ggml-cpu.c index a82842fcf..6a6b677c4 100644 --- a/third_party/whisper.cpp/ggml/src/ggml-cpu/ggml-cpu.c +++ b/third_party/whisper.cpp/ggml/src/ggml-cpu/ggml-cpu.c @@ -2525,7 +2525,7 @@ static bool ggml_thread_apply_priority(int32_t prio) { // Newer Windows 11 versions aggressively park (offline) CPU cores and often place // all our threads onto the first 4 cores which results in terrible performance with // n_threads > 4 - #if _WIN32_WINNT >= 0x0602 + #if defined(THREAD_POWER_THROTTLING_CURRENT_VERSION) && _WIN32_WINNT >= 0x0602 THREAD_POWER_THROTTLING_STATE t; ZeroMemory(&t, sizeof(t)); t.Version = THREAD_POWER_THROTTLING_CURRENT_VERSION;