Repository navigation
Vulkan: pick direct convolution whenever its output check passes, not by a timing race - #38
danielhanchen wants to merge 3 commits into
Conversation
|
@codex review |
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. |
![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: 4d6acac772
ℹ️ 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".
| BackendProbeOptions options; | ||
| options.measurement_runs = 1; | ||
| options.time_budget_ms = 1e9; | ||
| const auto result = sd_run_backend_probe(backend, 0, cases, options); |
There was a problem hiding this comment.
When a Vulkan device has less than 128 MiB free at context initialization—for example, because another workload is using VRAM—this call still inherits BackendProbeOptions::min_free_device_bytes = 128 MiB. sd_run_backend_probe then returns insufficient probe memory before checking outputs (backend_probe.cpp:116-123), and this function selects off; the same device with more free memory selects on. Thus machine load can still change the convolution path and fixed-seed pixels despite this change's determinism goal, so the correctness-only probe should remove or substantially lower that blanket threshold.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in a2c3b97: the Vulkan conv probe now sets min_free_device_bytes = 0. The probe graphs need a few MB, so the only remaining memory dependence is a device too full to allocate them at all, which still fails the probe and selects off. The changed files compile cleanly; I could not rerun the Vulkan leg on this host this round.
| convolution graph. On Vulkan, direct 2D convolution is then enabled when its | ||
| output on two small shapes matches im2col (a failed probe defaults to `off`), | ||
| and direct 3D convolution stays `off`. The decision does not depend on timing, | ||
| so the same seed renders the same pixels on a busy and on an idle machine. |
There was a problem hiding this comment.
This new rule contradicts docs/performance.md:35-36, which still tells users that automatic Vulkan selection uses small benchmarks. Timing now has no effect on the 2D decision and 3D is always disabled, so users following the tuning guide receive an obsolete description of auto; update that relevant guide together with these two documentation changes.
AGENTS.md reference: AGENTS.md:L74-L76
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a2c3b97: docs/performance.md now says Vulkan auto checks direct 2D output against im2col, does not use timing, and keeps direct 3D off.
The probe graphs need a few MB; the default 128 MiB floor still turned direct convolution off whenever another workload held the VRAM, so load could change the path and the pixels. A device too full to allocate the probe still fails it (off). Also update docs/performance.md, which still described the Vulkan rule as a benchmark.
|
@codex review |
Direct 2D convolution decodes faster in a fraction of the memory, but in UNet sampling (SD1.5, 512 px) it ran 11% slower than im2col on AMD's Windows driver (Radeon 8060S) and tied on RADV. Auto now turns it on for the VAE when the output check passes; the diffusion model and ControlNet keep im2col unless asked (--conv-direct auto,diffusion=on).
Stacked on the upstream sync branch (#35,
sync-upstream-a77d362, now at upstreammaster-995-fd21ecf, commit344a3b66). One change, insrc/core/probe/conv_direct.cpp, plus the two docs that describe it (docs/backend.md,docs/backend_probe.md). No ggml patch.Problem
On Vulkan,
--conv-direct auto(the default, and what Studio sends on Vulkan) decides direct convolution with a timing race: im2col and direct run on two tiny shapes (32x32x64 and 16x16x128) and direct must win by 5%. Each probe run is 0.1 to 0.2 ms of GPU work, so on a busy machine both paths mostly measure submit overhead and the answer flips from run to run.Published
master-982-0518b34-ud331c66Linux Vulkan bundle, Studio's Vulkan argv (--diffusion-fa), seed 42, B200 (NV_coopmat2) on a loaded host:a085fa2ba085fa2bd81e8a5f(differs)offwas chosen in 2 of 6 runs with Studio's argv and 1 of 6 with no flags; FLUX.2 klein 4B flipped the same way (4.805 vs 4.808 ms). So a fixed seed does not reproduce, and the run that lands onoffis much slower and larger:AMD Strix Halo iGPU (Radeon 8060S, gfx1151, RADV,
KHR_coopmat, UMA), 1024x1024, 3 runs per arm, run-to-run spread 2%:off): generate / VAE decode / peak RSSon): generate / VAE decode / peak RSSWith coopmat disabled (
GGML_VK_DISABLE_COOPMAT=1, the scalar shaders an iGPU without cooperative matrix support gets): VAE decode 2.72 vs 14.69 s at 1024 and 0.85 vs 1.38 s at 512. Z-Image 512 with coopmat: VAE 0.66 vs 0.97 s.B200: VAE compute buffer at 512x512 is 1984 MB with im2col and 704 MB with direct; VAE decode 405 vs 333 ms of GPU time.
The gfx1151 numbers come from forcing each mode (
--conv-direct onvs--conv-direct off) on the published bundle, not from a build of this branch. This branch selects what theonarm runs for 2D convolution.Direct 3D convolution has the same race on NVIDIA (0.388 vs 0.400 ms, a tie), and on RADV its probe output check fails, so
autoalready turns it off there.Fix
On Vulkan,
autono longer uses the timing:conv2d direct auto: diffusion -> off (Vulkan: VAE only)). In UNet sampling direct convolution was slower on AMD's Windows driver and a tie on RADV (below), and DiT models have almost no 2D convolutions outside the VAE.--conv-direct auto,diffusion=onturns it on for the diffusion model.Other backends are unchanged. An explicit
--conv-direct on|offor a per-module assignment still wins.Tests
-DSD_VULKAN=ON -DGGML_NATIVE=OFF), all ggml patches applied on ggmlc1f80e5.on(8553f24f,0cfb4141). In 2 of those 6 runs the probe timed direct slower (5.448 vs 7.199 ms, 2.962 vs 4.856 ms), where the old rule would have chosenoff.3fb6fe67); its VAE is 3D, so the 2D choice does not reach it.coopmat2andcoopmat1(GGML_VK_DISABLE_COOPMAT2=1) paths. For scale,coopmat1vscoopmat2alone is LPIPS 0.0008 / 0.0197.onin 9 of 9 runs, so there this branch renders what the pin renders when nothing else is running, and keeps doing so when something is.Results on gfx1151 (
a2c3b975, the revision before the VAE-only rule, built with the prebuilt Vulkan leg flags; base #35 in the same job, 3 interleaved runs)conv2d direct auto: Vulkan0 -> on (output check passed ...)andconv3d direct auto: Vulkan0 -> off (Vulkan)on both drivers. The base's conv3d probe fails its output check on both, which is why 3D stays off.off;onin another run), and the flip changed the Z-Image pixels for one seed. The branch rendered the same pixels as the base forced to--conv-direct on, on both drivers, in every run.test-backend-opsCONV_2D 1966/1966 and IM2COL 102/102 on Vulkan0, base and branch, both drivers.This revision (
3d7baa21, VAE only) vsa2c3b975on the same boxes, SD1.5 F16 512 px, 20 steps, medians of 3 interleaved runs:Both drivers log
conv2d direct auto: diffusion -> off (Vulkan: VAE only). The output equalsa2c3b975forced to--conv-direct off,vae=on, and--conv-direct auto,diffusion=onequalsa2c3b975, at 4 and 20 steps on both drivers.Not covered