Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 53 additions & 8 deletions third_party/whisper.cpp/AETHERSDR-PATCHES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
jensenpat marked this conversation as resolved.
Comment thread
jensenpat marked this conversation as resolved.

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
Expand Down Expand Up @@ -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
Comment thread
jensenpat marked this conversation as resolved.
Comment thread
jensenpat marked this conversation as resolved.
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
Expand All @@ -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/<file>`). 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/<file>` 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.
34 changes: 21 additions & 13 deletions third_party/whisper.cpp/AETHER_VENDORING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -52,30 +52,38 @@ 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

To add a different GPU backend (CUDA, Metal, …), **re-copy that backend's
directory** from upstream at the pinned commit and turn its `GGML_<X>` 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.
2 changes: 1 addition & 1 deletion third_party/whisper.cpp/ggml/src/ggml-cpu/ggml-cpu.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment thread
jensenpat marked this conversation as resolved.
THREAD_POWER_THROTTLING_STATE t;
ZeroMemory(&t, sizeof(t));
t.Version = THREAD_POWER_THROTTLING_CURRENT_VERSION;
Expand Down
Loading