Repository navigation
CPU: AVX512 kernels on Intel, faster bit-identical direct convolution by default, physical-core thread default - #36
danielhanchen wants to merge 7 commits into
Conversation
…t on Linux The sysfs loop read /sys/devices/system/cpu<N>/topology/thread_siblings (missing the /cpu path component), never opened a file, and always fell back to half of std::thread::hardware_concurrency(). That halves every core count on a host without SMT and ignores taskset / container cpusets (a 8-CPU cpuset on a 192-CPU host ran 96 threads). Count the distinct thread-sibling sets among the CPUs in the process affinity mask instead, falling back to the old estimate over the allowed CPUs when sysfs has no topology.
…ical) The CPU CONV_2D / CONV_3D kernels (--conv-direct) gathered each patch element by element across all input planes, and wrote each GEMM output element into a different output plane. Process up to 64 consecutive patches of one output row at a time instead: each source row segment is read once per run and each output plane gets a contiguous row segment. Same values and the same fp32 -> fp16 conversions in the same slots, so results are bit-identical to the per-patch loops.
… AVX-VNNI 0008 dropped the AVX512 variants on every CPU without AVX-VNNI because Zen 4 ran them 15 to 50% slower than AVX2. That also took them from Intel Skylake-SP, Cascade Lake and Ice Lake, which run them much faster: on a 6-core Cascade Lake VM the cascadelake variant (AVX512-VNNI) cut Z-Image Turbo sampling from 50.2 s to 29.3 s and FLUX.2 klein from 63.8 s to 39.3 s against haswell. Only AMD without AVX-VNNI (Zen 4) keeps the AVX2 variant now.
…fer) --conv-direct auto now picks the direct CONV_2D / CONV_3D kernels on the CPU backend. ggml-cpu computes them with the same fp16 patches and the same GEMM as im2col + mul_mat, so outputs are bit-identical, but through a fixed 16 MB patch buffer instead of the whole im2col matrix (2.8 GB for one 128-channel conv of a 1024x1024 VAE decode, 5.6 GB for a 256-channel one). With the row-run fill from the previous commit they are also faster than im2col. Direct convolution is only taken for F16 / F32 weights, which is all the direct kernels accept; any other weight type keeps im2col instead of aborting in ggml-cpu (also for an explicit --conv-direct on).
Codex Review SummaryThis 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 |
![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: f38a4dad43
ℹ️ 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 (sd_backend_is_cpu(backend)) { | ||
| // ggml-cpu's direct kernels produce the same values as im2col + mul_mat (bit-identical) and work through a | ||
| // fixed 16 MB patch buffer instead of materialising the whole im2col matrix, which in a VAE decoder at | ||
| // 1024x1024 is 2.8 GB for one 128-channel conv and 5.6 GB for a 256-channel one. | ||
| LOG_INFO("conv%dd direct auto: %s -> on (CPU)", dims, name); | ||
| return true; |
There was a problem hiding this comment.
When the LTX 2.3 encoder runs on CPU with the default auto mode, this unconditional result sets direct=true, so ggml_ext_conv_3d enters its direct branch before checking force_prec_f32. That model deliberately sets force_conv_prec_f32 for down_blocks.7.conv because FP16 accumulation overflows (src/model/vae/ltx_vae.hpp:822-828), but the direct path bypasses the ggml_prec_set_acc(..., GGML_PREC_F32) branch at src/core/ggml_extend.cpp:511-519, potentially producing invalid encoded latents. CPU auto-selection must retain the im2col/F32 path for convolutions requesting forced-F32 accumulation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not an issue on CPU. ggml-cpu never reads the GGML_PREC_F32 flag (its F16 mul_mat always accumulates in F32), so the force_prec_f32 branch and the direct kernel do the same math there. Checked with a standalone ggml test on the patched CPU backend: the force_prec_f32 graph from ggml_ext_conv_3d vs ggml_conv_3d_direct, F16 weights, outputs up to 1.9e5 (about 3x the F16 max), 19200 outputs, 0 differing bits and no inf or nan, on the sapphirerapids, haswell and x64 variants. The flag matters on GPU backends, which this CPU rule does not touch.
| if (sd_backend_is_cpu(backend)) { | ||
| // ggml-cpu's direct kernels produce the same values as im2col + mul_mat (bit-identical) and work through a | ||
| // fixed 16 MB patch buffer instead of materialising the whole im2col matrix, which in a VAE decoder at | ||
| // 1024x1024 is 2.8 GB for one 128-channel conv and 5.6 GB for a 256-channel one. | ||
| LOG_INFO("conv%dd direct auto: %s -> on (CPU)", dims, name); | ||
| return true; |
There was a problem hiding this comment.
For ordinary source/CMake builds, this enables CPU direct convolution against the unmodified ggml submodule, because the new row-run optimization exists only as scripts/unsloth/ggml-patches/0014-... and the repository applies those patches solely in .github/workflows/unsloth-sd-prebuilt.yml:226-229. Consequently those builds use the old direct kernel that the commit's own measurements show is 32–53% slower than im2col for large convolutions, while still selecting it unconditionally. Either integrate/apply the optimization for normal builds or keep CPU auto disabled when it is absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in e181fad. Patch 0014 now also defines GGML_CPU_CONV_DIRECT_ROW_RUNS in ggml-cpu.h, and the CPU auto rule picks direct only when that macro is present, so a plain source build without the fork's ggml patches keeps auto at off. Preprocessing conv_direct.cpp against the pristine and the patched ggml headers gives "off (CPU, ggml without the row-run direct kernels)" and "on (CPU)". The full patch set still applies cleanly to the pinned ggml. A patched CPU build renders Z-Image 256 (4 steps, seed 42) pixel-identical to the previous head.
| cpu_set_t mask; | ||
| CPU_ZERO(&mask); | ||
| if (sched_getaffinity(0, sizeof(mask), &mask) == 0) { |
There was a problem hiding this comment.
On Linux hosts whose kernel affinity mask exceeds the fixed CPU_SETSIZE (1024 CPUs), passing sizeof(cpu_set_t) makes sched_getaffinity fail with EINVAL; this then falls through to hardware_concurrency(), which reports the host's online CPUs rather than the process's taskset/container allowance. A process restricted to a small cpuset on such a host can therefore again start hundreds of threads, defeating the affinity-aware behavior introduced here. Use a dynamically sized CPU set (or retry with a larger mask) before falling back.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e181fad: the mask is now allocated with CPU_ALLOC and doubled on EINVAL before falling back. On a 2x48-core SMT host: taskset 0-3 gives 4, 0-15 gives 16, 0,96 (one core's two siblings) gives 1, all 192 CPUs give 96. The over-1024-CPU branch itself could not be exercised on this host.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b964f151b0
ℹ️ 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".
| dims, name, result.candidate_faster ? "on" : "off", result.reference_ms, result.candidate_ms); | ||
| return result.candidate_faster; | ||
| } | ||
| if (sd_backend_is_cpu(backend)) { |
There was a problem hiding this comment.
When users run a CPU backend with the default --conv-direct auto, this branch now enables direct convolution unconditionally, but docs/backend.md:235-245 still states that auto uses Vulkan benchmarking or implicit-GEMM checks and that other backends default to off; docs/performance.md:35-36 repeats that description. Update these user-facing selection rules so CLI and library users can predict the new CPU default and know when to override it with off.
AGENTS.md reference: AGENTS.md:L74-L76
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e181fad: docs/backend.md and docs/performance.md now describe the CPU rule (on for 2D and 3D when ggml carries the row-run kernels from patch 0014, off otherwise).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b964f151b0
ℹ️ 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".
| LOG_INFO("conv%dd direct auto: %s -> on (CPU)", dims, name); | ||
| return true; |
There was a problem hiding this comment.
When CPU auto handles a model with F32 2D convolution weights (for example an original F32 checkpoint or --type f32), this unconditional selection sends the operation through ggml_conv_2d_direct. The previous im2col path explicitly stores activations as F16 for every non-BF16 weight (ggml_extend.cpp:468-469), whereas the new direct-kernel patch stores them as F32 when the kernel is F32 (0014-ggml-cpu-conv-direct-row-runs.patch:70-73). The dot products therefore are not bit-identical and default CPU generation can produce different results; keep F32 2D operations on im2col or reproduce its conversion in the direct path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly. The examples given do not reach this: Conv2d and Conv3d blocks always create F16 weights (init_params in ggml_block.hpp), whatever the checkpoint dtype or --type, so model convolutions stay bit-identical. F32 conv weights only show up for runtime-applied LoRA tensors, and there the direct kernel keeps F32 activations where im2col rounds them to F16, so it is more precise rather than worse: in a standalone test vs an fp64 reference, relative rms error 3.1e-4 for im2col vs 8.8e-8 for direct. Depthwise convs behave the same way (2.1e-4 vs 5.6e-8). Rather than force the F16 rounding back in, e181fad corrects the comment and docs so bit-identical is only claimed for F16 weights.
… mask - ggml-patches/0014 also defines GGML_CPU_CONV_DIRECT_ROW_RUNS in ggml-cpu.h. CPU auto selects direct convolution only when it is defined, so a source build without the fork's ggml patches keeps the old off default instead of the slower per-element direct loops. - The comment and docs now say bit-identical holds for F16 conv weights (every Conv2d / Conv3d block); F32 LoRA tensors and depthwise convs keep F32 activations in the direct kernels and come out closer to exact. - sd_get_num_physical_cores sizes the affinity mask with CPU_ALLOC and grows it on EINVAL, for kernels built for more than 1024 CPUs. - docs/backend.md and docs/performance.md describe the CPU auto rule.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
Stacked on the upstream sync PR (
sync-upstream-a77d362); the diff below that branch is four changes, all on the CPU path.Problem
Studio sends no-GPU and low-end hosts to the CPU build of this fork. Profiling it on clean, unshared VMs (Intel Xeon @ 2.2 GHz, Cascade Lake, 6 cores / 12 threads, AVX512 + AVX512-VNNI, no AVX-VNNI; 6 threads, Studio's own CPU argv) showed three things worth fixing:
ggml-patches/0008drops the AVX512 CPU variants on every CPU without AVX-VNNI, because Zen 4 ran them 15 to 50% slower than AVX2. That rule also catches Skylake-SP, Cascade Lake and Ice Lake (Xeon and many cloud VMs), where the AVX512-VNNIcascadelakevariant is far faster: on the VM above, Z-Image Turbo sampling takes 29.3 s with it and 50.2 s withhaswell.--conv-direct autokeeps im2col + mul_mat, which for one 128-channel 3x3 conv of a 1024x1024 decode is a 2.8 GB buffer (5.6 GB for a 256-channel one). Z-Image at 1024x1024 peaks at 15.5 GB RSS, most of it that buffer. The CPU direct kernels compute exactly the same values through a 16 MB patch buffer, but were slower (Z-Image 1024 VAE 122.6 s vs 112.8 s), so nothing used them.sd_get_num_physical_cores()read/sys/devices/system/cpu<N>/topology/...(no/cpu), never opened a file, and fell back tohardware_concurrency() / 2. That halves every core count on a host without SMT and ignores taskset / container cpusets (a 4-CPU cpuset on a 192-CPU host ran 96 threads).Where the time goes before this PR (6 threads,
haswellvariant, ggml op timing on thread 0, 256x256 / 4 steps unless noted):Changes
ggml-patches/0008: AVX512 variants stay available on Intel. Only AMD CPUs without AVX-VNNI (Zen 4) are steered to the AVX2 variant now, exactly the case 0008 was measured on. Intel AVX512 CPUs without AVX-VNNI loadskylakex/cascadelake/icelakeagain. CPUs with AVX-VNNI (Zen 5, Alder Lake and newer, Sapphire Rapids and newer) are unaffected.ggml-patches/0014: faster CPU direct convolution, bit-identical.CONV_2DandCONV_3Dfilled each patch element by element across all input planes and wrote each GEMM output into a different output plane per element. They now handle up to 64 consecutive patches of one output row at a time: each source row segment is read once per run and each output plane receives a contiguous row segment. Same values, same fp32 to fp16 conversions in the same slots, same GEMM.--conv-direct autopicks direct convolution on the CPU backend (VAE, diffusion model and ControlNet). Outputs are bit-identical to im2col, the 2.8 to 5.6 GB im2col buffers disappear, and with change 2 it is also faster. Direct convolution is only built for F16 / F32 weights, which is all the direct kernels accept; any other weight type keeps im2col instead of aborting in ggml-cpu (this also covers an explicit--conv-direct on).thread_siblingssets oversched_getaffinity, with the old estimate as the fallback when sysfs has no topology. macOS and Windows are unchanged.Results
Clean Colab A100 VMs (the CPU of the VM; no GPU used), Intel Xeon @ 2.2 GHz Cascade Lake, 6 cores / 12 threads, 83 GB RAM. Base = the published
master-982-0518b34-ud331c66Linux x64 bundle; head = this branch built on the VM like the prebuilt CPU leg (GGML_BACKEND_DL, all CPU variants).-t 6, seed 42, prompt "a photo of a red fox sitting in fresh snow, golden hour, detailed fur", Studio's assets per family. Arms interleaved, one freshsd-cliper run, median [min, max] in seconds; peak RSS fromwait4. Every repeat of an arm rendered the same pixels (A/A spread of the images is 0) and repeat-to-repeat timing spread is under 1%.--type q8_0Phases for Z-Image 1024: sampling 218.6 to 132.6 s, VAE decode 113.0 to 81.7 s.
Each change on its own
AVX512 variants on Intel (change 1). Same VM, base bundle vs fork master built without 0008 (loads
libggml-cpu-cascadelake.so):haswellsamplingcascadelakesamplinghaswellgeneratecascadelakegenerateThe
skylakexvariant (AVX512 without VNNI) forced on the same CPU: Z-Image sampling 45.1 s, so the gain is the VNNI int8 dot products.Direct convolution (changes 2 and 3).
convbench(one conv, F16 weights, 6 threads, best of 3, median of 3 repeats; all outputs bit-identical between im2col and direct, old and new):(The buffer columns include the conv's own input and output tensors.)
End to end on the
haswellvariant (so change 1 plays no part), same VM, base vs this branch with direct convolution:The old kernel's RSS saving was already there (FLUX.2 klein 1024: 12.54 to 8.77 GB), but at +7% VAE time; the new kernel is what makes it a speedup. Also pixel-identical with direct convolution: Krea 2 (conv3d VAE), SDXL at 768, Z-Image and FLUX.2 klein at 256 and 512.
Threads (change 4). With
-tpassed (as Studio does) nothing changes. Without-t, on the VM pinned to one thread per core (taskset -c 0-5, a 6-core host without SMT), base picks 6 only because the VM itself has 12 logical CPUs; on a real 6-core host without SMT it picks 3, and 3 threads vs 6 is 112.44 [112.28, 112.76] vs 58.63 [58.61, 58.81] s for Z-Image at 256 (1.92x). This branch reportsusing 6 threadsundertaskset -c 0-5,2under a 4-CPU / 2-core mask and6unrestricted; on the 192-CPU dev host, 8 / 1 / 4 / 16 threads for masks of 8 cores, one SMT pair, 4 SMT pairs and 16 cores (base: 96 for all of them).Accuracy
Changes 2, 3 and 4 do not change any output: pixel-identical renders on every family above, and
convbenchcompares the direct and im2col outputs bit for bit (0 differing values, including stride 2, 1x1, 5x5, conv3d and odd sizes).Change 1 swaps the int8 dot-product kernels on Intel AVX512 CPUs, so the float accumulation order differs. LPIPS of
cascadelakevshaswell: Z-Image 0.015 (256), 0.008 (512) and 0.006 (1024), FLUX.2 klein 0.014 (256) and 0.039 (512), SDXL Turbo 0.008, Krea 2 0.133 (256, 4 steps, seed 42). For scale, the samehaswellcode built by two GCC versions differs by LPIPS 0.013 on Z-Image. Against a higher-precision reference (the Q8_0 DiT of the same model, same seed,haswell), 3 seeds at 256x256, 4 steps, LPIPS to the reference:haswell)haswellcascadelake)The AVX512 kernels land as close to the reference as the AVX2 ones. Base vs
cascadelakediffers by LPIPS 0.011 to 0.015 on Z-Image and 0.002 to 0.007 on Krea 2 seeds 43 / 44, the same size as base vs thehaswellcode built by another GCC (0.012 to 0.016 and 0.005). Krea 2 seed 42 is the one large difference (0.133): there thecascadelakerender is the one closer to the Q8_0 reference (0.099 vs 0.184).Not covered
ggml_ext_conv_2d/ggml_ext_conv_3dwrappers with F16 weights, whichconvbenchcovers, but no end-to-end render was compared.