Repository navigation
CPU: tinyBLAS GEMM for F16 / BF16 weights by default on x86 (VAE 1.9x, BF16 text encoder 2.4x) - #47
danielhanchen wants to merge 6 commits into
Conversation
…_SGEMM_TYPES=q8_0 restores Q8_0 only)
|
@codex review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…VX2 hosts On AMD the AVX2 F16 kernels ran slower than vec_dot from the release toolchains (SD1.5 F16 sampling, GitHub runners: Zen 3 0.78x and Zen 4 0.88x with GCC 11 on ubuntu-22.04, Zen 3 0.71x with MSVC), so F16 / BF16 stay opt-in there and in every MSVC build. Q8_0 stays on everywhere. GGML_CPU_SGEMM_TYPES still overrides the set.
The Windows x64 bundle's AVX512 variants measured 1.88x on a Xeon 8573C (SD1.5 F16 sampling), so the MSVC exclusion now covers only the AVX2 kernels, where AMD Zen 3 / Zen 5 measured 0.71x / 0.86x.
Adds Intel AVX2 under MSVC (1.24-1.25x), AMD Zen 4 AVX2 under MSVC (1.03-1.11x), AMD Zen 5 AVX2 under GCC / clang (1.07-1.08x) and aarch64 builds without FP16 vector arithmetic, whose tinyBLAS kernel accumulates in F32 (armv8.0 1.35x on a Neoverse N2; tinyBLAS is now built on aarch64 too). Stays off on Zen 3 AVX2 (0.71-0.78x), Zen 4 AVX2 with GCC 11 (0.87-0.88x), Zen 5 AVX2 with MSVC (0.86x) and aarch64 FP16-vector builds (FP16 accumulation). Q8_0 stays on x86 only.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71b556384e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (NOT DEFINED GGML_LLAMAFILE AND CMAKE_SYSTEM_PROCESSOR MATCHES "^(x86_64|AMD64|amd64|x64|aarch64|arm64|ARM64)$") | ||
| set(GGML_LLAMAFILE_DEFAULT ON) |
There was a problem hiding this comment.
For ordinary source builds on ARM64, this enables tinyBLAS without the precision safeguard added by patch 0017. The patch is applied only by .github/workflows/unsloth-sd-prebuilt.yml; the documented local build instead passes the submodule directly to add_subdirectory in cmake/ggml.cmake. Consequently, FP16-vector targets such as Apple Silicon can route F16 matmuls through the FP16-accumulating kernel that this change explicitly excludes from its defaults, changing CPU inference precision rather than merely accumulation order. Apply the filter to ordinary builds too, or gate the new ARM default on its availability.
Useful? React with 👍 / 👎.
Stacked on #42 (
cpu-sgemm-q8-0). Patch-only change: it widens the default type set ofggml-patches/0017(per CPU, see below), no source change.Problem
With #42, x86 CPU builds compile llamafile tinyBLAS but route only Q8_0 weights through it. F16 and BF16 matmuls still run one
vec_dotper output element:Fix
GGML_CPU_SGEMM_TYPESnow defaults toq8_0,f16,bf16where tinyBLAS measured faster thanvec_dotfor F16 / BF16, and toq8_0elsewhere:vec_dottinyBLAS is now also built on aarch64 (
CMakeLists.txt); upstream already compiles it out of the SVE / i8mm variants.Vendor and family are read with CPUID at
ggml_cpu_init. Q8_0 stays on on every x86 build (bit-identical, 1.07-1.11x on every CPU above).GGML_CPU_SGEMM_TYPES=q8_0restores the #42 behaviour, andGGML_CPU_SGEMM_TYPES=q8_0,f16,bf16turns F16 / BF16 on where the default leaves them off. F32 stays opt-in (it doubles the rms error vs an F64 GEMM). The patch file keeps its name so the #42 diff stays readable.Evidence
Colab CPU VMs (Intel Broadwell, 4 cores / 8 threads), #42 vs this branch, same VM, interleaved, median of 3:
DiT sampling is unchanged (Q4_K_M / Q8_0 DiTs have almost no F16 / BF16 matmuls).
GitHub-hosted runners, the fork's prebuilt flags (all CPU variants), SD1.5 F16 256 px, 4 steps, same runner per row, interleaved, median of 2-3. These are the rows behind the default above (one job per row):
vec_dotThe same EPYC 7763 built with GCC 13 (ubuntu-24.04) ran tinyBLAS F16 at 1.79x, so the AMD AVX2 loss comes from the release toolchains' code generation, not the CPU. Validation runs of the default: Zen 3 (GCC 11 and MSVC) render the
GGML_CPU_SGEMM_TYPES=q8_0pixels at its speed, and Intel (GCC, MSVC, clang) and AVX512 runners render the tinyBLAS pixels.Accuracy: the pixels change once (both paths read the same F16 / BF16 inputs and accumulate in F32, in a different order). Against a reference that runs the text encoder and VAE in F32 weights (4 prompts x 2 seeds, 256 px, Z-Image 8 steps, klein 4 steps):
The branch is closer on 5 of 16 renders, so this is a tie, not an improvement. GEMM rms error vs an F64 GEMM is the same for both paths.
Not covered