Repository navigation
fix(vision): add GELU selector, use tanh where checkpoints declare it - #2552
Merged
Merged
Conversation
`mlxcel_core::gelu_approx` computes the exact erf GELU, while MLX's `nn.gelu_approx` and `nn.GELU(approx="precise")` are the tanh approximation. Ports that needed tanh each grew a private copy, and the comments around `gelu_approx` claimed erf matched `approx="precise"`.
Add `gelu_tanh` (input dtype) and `gelu_tanh_f32` (f32 evaluation, cast back) FFI entry points built on the C++ `gelu_tanh_approx` helper the fused GeGLU kernels already use, and a `GeluActivation { Erf, Tanh, TanhF32, Fast }` selector in `mlxcel_core::utils`. `gelu_approx` keeps its erf semantics; its C++, FFI and Rust doc comments now say so.
Fold the private tanh copies in BERT, Kokoro, Molmo2, SigLIP, Kimi-VL (MoonViT) and Qwen3-VL into the selector. Each fold keeps the precision its copy used (f32 for SigLIP, Molmo2, Qwen3-VL; input dtype for Kimi-VL and Kokoro), and new tests reproduce the old copies and assert bit equality for f32, bf16 and f16 inputs. BERT's copy promoted half inputs to f32 without casting back; it now uses `TanhF32`, which returns the input dtype and is bit-equal for f32 inputs.
No model's activation formula changes in this commit.
Refs #2423
Five vision sites computed erf GELU where both the checkpoint's Hugging Face activation and the mlx-vlm port use the tanh form. They now select it through `GeluActivation`, evaluated in f32 as the existing SigLIP `gelu_pytorch_tanh` path is: - MiniCPM-V 4.6 encoder MLP, read from `vision_config.hidden_act` (MiniCPM-o keeps erf until a checkpoint is measured), and the window merger, which HF hard-codes to `gelu_pytorch_tanh`. - Gemma 3 VLM and PaliGemma SigLIP: their configs omit `hidden_act`, whose `SiglipVisionConfig` default is `gelu_pytorch_tanh`; the two loaders now resolve the missing key, and the XLA Gemma 3 vision emitter does the same so the backends agree. Other SigLIP/CLIP loaders are unchanged (CLIP's default is `quick_gelu`). - Gemma 4 vision GeGLU, read from `hidden_activation`; DiffusionGemma and EmbeddingGemma2 reuse this tower, as their mlx-vlm ports do. InternViT and the MiniCPM-V 4.6 downsample merger keep erf deliberately: HF evaluates them as erf and the mlx-vlm port departs from the checkpoint there. Comments now say so, and the misleading ones on `compiled_gelu_topk`, Gemma3n's sparse path and DeepSeek-VL2 are corrected. The decided-position logit-trace gate cannot tell erf from tanh at any of these families (the reverted arm does not fail; the swap moves logits by at most the bf16 quantum), so the decision rests on the declared activation. bf16 tanh was re-tested on the 4096-patch SigLIP tower without NaN or Inf. Inventory in docs/gelu-call-sites.md, measurements in docs/benchmark_results/gelu-erf-vs-tanh-2026-10-23.md. Refs #2423
Closed
3 of 6 tasks
15 tasks
The Gemma 3 SigLIP switch from erf to TanhF32 adds a measurable prefill cost that the record did not state. On gemma-3-4b-it-4bit with one 896px image (273 prompt tokens), interleaved on one binary with only the vision activation changed: erf median 872 ms, TanhF32 1025 ms (paired +151 to +172 ms, about +17 to 20 percent), input-dtype tanh 916 ms (+42 ms). The record now keeps the f32 choice with its measured reason (PyTorch evaluates the tanh GELU in f32 for half inputs; input-dtype tanh gave Gemma 3 the worst trace perplexity) and points the fused-kernel remedy at the #2548 follow-up. Also correct the Gemma 4 activation test comment: gemma-4-12b-it-4bit is gemma4_unified, which is encoder-free and never parses a Gemma4VisionConfig. Refs #2423
Member
Author
Implementation Review SummaryIntent
Findings Addressed
Remaining Items
Checked, no finding
Verification
|
The erf-to-tanh switch in this PR made Gemma 3 image prefill 151-172 ms (17-20 %) slower because the eager f32 tanh GELU is about eleven unfused elementwise passes over 27 x [1, 4096, 4304] activations. Add compiled_gelu_tanh_f32 to the bridge: gelu_tanh_approx with the upcast and the cast back inside one shapeless compile region, exposed as GeluActivation::TanhF32Compiled. Only the families switched by #2423 use it: the MiniCPM-V 4.6 encoder MLP and window merger, the Gemma 4 vision GeGLU, and the Gemma 3 / PaliGemma SigLIP towers whose config omits hidden_act (a new VisionHiddenActivation::SiglipDefaultTanh, set only by default_missing_hidden_act_to_siglip). TanhF32 and every call site folded onto it stay on the eager helper, so the bit-exact fold tests are unchanged. Numerics: the compiled kernel matches the eager helper within a few f32 ulps on the sample sweep and bit for bit on a tower-shaped bf16 [64, 4304] input with outliers to +-170 (0 of 275456 lanes differ). The shapeless compile audit lists the new site and passes on GB10. Refs #2423
The #2538 overlay publishes each cache file through a temporary named <final name>.tmp<pid>_<tick>. get_ptx_path keeps a module name of up to 245 characters in one path component, so for names of roughly 229-245 characters the temporary passed the 255-byte NAME_MAX, the open failed with ENAMETOOLONG, the entry was never written, and every process compiled that kernel again with NVRTC. The fused f32 tanh GELU kernel (a 234-character module name) hit it: each Gemma 3 image prefill paid about 1.7 s of JIT (2505 ms against 869 ms once cached). The temporary is now named from the suffix and the extension only, in the same directory, so the rename stays atomic and the name length no longer depends on the module name. tests/cuda_custom_kernel_disk_cache.rs also runs the compiled tanh GELU in its parent and child processes; it fails on the previous overlay (the child prints the JIT notice) and passes with this change. Refs #2423
Records the compiled f32 tanh GELU (numerics, smoke tests, the PTX cache NAME_MAX finding) and a quiet-host prefill A/B per switched family with a null arm: Gemma 3 +2.8 % (eager was +17.0 % in the same block), Gemma 4 +2.0 %, MiniCPM-V 4.6 and PaliGemma within the null floor. Every family keeps tanh. The call-site inventory marks the switched sites as compiled and describes the SigLIP default split. Refs #2423
Resolves the activation_helpers_return_the_input_dtype case list: main added the four qwen4 hyper helpers and this branch the three tanh GELU helpers.
Member
Author
Security and Performance ReviewScope: the PTX cache temp naming (a78ffcf), config-driven GELU selection, the compiled f32 tanh GELU, and the recorded prefill cost. No CRITICAL, HIGH or MEDIUM findings, so there are no fix commits. Checked, no finding
LOW (recorded, not changed)
TestsGB10, 316fea1, |
8 tasks done
inureyes
added a commit
that referenced
this pull request
Oct 30, 2026
Hardens the CUDA JIT PTX disk cache in the `jit_module.cpp` overlay, the one path every binary and embedder shares. The fused MoE and SSM decode kernels load from this cache, so on a multi-user host whoever controlled the cache directory could plant modules that load into other users' processes. Covers the issue body plus the PR #2552 review comment. ## What changes - Trust check (POSIX): the canonical cache path is `lstat`-ed from the leaf up to `/`. Every component must be owned by the user or root and must not be group- or other-writable, unless it is root-owned and sticky (`/tmp`). The canonical path is used after the check. On refusal the cache is off, kernels still compile, and one stderr line is printed (not muted by `MLXCEL_QUIET_JIT`): `mlxcel: CUDA kernel disk cache disabled: <component> <reason>`. - The POSIX fallback becomes `$TMPDIR/mlx-<euid>/<version>/ptx`, created 0700. Entries live in an `sm<cc>-nvrtc<major>.<minor>` subdirectory. That subdirectory and the `mlx-<euid>` root must be real directories: a symlink planted at either is refused. Flat entries in an old root are no longer read. Windows behavior is unchanged. - Publish temporaries are opened `O_CREAT|O_EXCL|O_NOFOLLOW` and fall back to a cold cache on any failure. Temporaries older than an hour (`.tmp<hex>_<hex>.{ptx,txt,cu}`) are swept on first resolve, without following symlinks. A test-only hook, `MLXCEL_PTX_CACHE_TEST_TMP_SUFFIX`, makes the temporary name deterministic. It accepts only that same suffix form. - `ensure_persistent_ptx_cache` creates its directories 0700. Under umask 002, earlier releases created `~/.cache/mlxcel/cuda-ptx/<commit>` as 0775, which the new check would refuse on every existing install. The function now clears group and other write on `cuda-ptx` and `cuda-ptx/<commit>`, and on the root only when `MLXCEL_CACHE_DIR` is unset. It touches only real directories owned by the euid, and changes modes through a descriptor after a device and inode match, so a swapped-in symlink is never chmod-ed. ## Behavior change A group-writable explicit `MLX_PTX_CACHE_DIR` or `MLXCEL_CACHE_DIR`, or a group-writable `~/.cache`, is refused with the notice. The fix is `chmod go-w` on the directory the notice names. The overlay README is left untouched because its `jit_module.cpp` mention shares a line with #2696. ## Validation (GB10, gpu-lock) - `cuda_ptx_cache_trust` 10/10, `cuda_custom_kernel_disk_cache` 1/1, `ptx_cache_dirs` unit tests 4/4. - Failing first on main 8385f40: the 0770 dir, the 0777 parent, the per-user fallback, device keying, the sweep, and the updated disk-cache test all fail; the private-dir control passes. Failing first on the previous PR head: the symlinked device dir, the symlinked fallback root, and the swapped-path chmod. - Mutation evidence for the two tests main cannot fail, since the hook is new and the retry already existed. With the open reverted to a following `O_CREAT|O_TRUNC` and `throw;` added at the top of the retry catch, the symlink test fails and the garbage-`.ptx` phase aborts. - `mlxcel generate` on granite-4.0-h-tiny-4bit: the second run prints no JIT notice with a fresh cache and also with the real home default. On the home default the 0775 directories became 0755, and `sm121-nvrtc13.0` is 0700. - fmt, clippy `-D warnings` (mlxcel and mlxcel-core lib and tests, both test targets), and `make verify-env-var-docs` pass. Closes #2539
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mlxcel_core::gelu_approxis the exact erf GELU, while MLX'snn.gelu_approx/approx="precise"is the tanh form. This PR adds an explicit GELU selector, folds the private tanh copies into it, corrects the comments that claimed erf matchedapprox="precise", and switches the vision families whose checkpoint and mlx-vlm port both use tanh, evaluated in f32 through a compiled kernel, which keeps the prefill cost small: Gemma 3 +2.8% pooled (same sign in both blocks, so a small real cost), Gemma 4 +2.0%, MiniCPM-V 4.6 and PaliGemma within the null arm (the eager form cost Gemma 3 +17%).GeluActivation { Erf, Tanh, TanhF32, TanhF32Compiled, Fast }plusfrom_hf_nameinmlxcel_core::utils, backed bygelu_tanh/gelu_tanh_f32/compiled_gelu_tanh_f32on the C++gelu_tanh_approxhelper.gelu_approxkeeps erf.gelu_pytorch_tanh, Kimi-VL/MoonViT, Qwen3-VL) keep their eager formula and precision; tests prove the folds bit-equal for f32/bf16/f16 (BERT now casts a half result back, bit-equal for f32).TanhF32Compiled: MiniCPM-V 4.6 encoder MLP (fromhidden_act) and window merger; Gemma 3 VLM and PaliGemma SigLIP (missinghidden_actresolved to the SigLIP default asVisionHiddenActivation::SiglipDefaultTanh, mirrored in the XLA Gemma 3 vision emitter); Gemma 4 vision GeGLU (fromhidden_activation, also reaching DiffusionGemma and EmbeddingGemma2).hidden_act/hidden_activationthat is not a GELU name now fails the load for the MiniCPM-V 4.6 encoder and the Gemma 4 vision tower instead of silently running erf; every local config declaresgelu_pytorch_tanhor omits the key.jit_module.cppoverlay wrote each cache file through<module name>.txt.tmp<pid>_<tick>, which passes NAME_MAX for module names of about 229-245 characters, so those kernels were never cached and every process NVRTC-compiled them again. The compiled tanh kernel (234 characters) hit it: Gemma 3 prefill 2505 ms uncached against 869 ms cached. The temporary is now named from the suffix and the extension only;tests/cuda_custom_kernel_disk_cache.rsruns this kernel in a second process, fails on the old overlay and passes now.Evidence
Inventory:
docs/gelu-call-sites.md. Measurements:docs/benchmark_results/gelu-erf-vs-tanh-2026-10-23.md.Image prefill,
mlxcel generate --image tests/fixtures/test_image_shapes.png -p "Describe the image." -n 8 --temp 0 --profile, one binary, erf and null arms through symlinked checkpoint copies declaringgelu, arm order rotated,MLX_ENABLE_TF32=0, onegpu-lockhold per block, no CI/cargo/nvcc process during any run. GB10, kernel 7.0.0-1019-nvidia, driver 580.178.04,MLX_CUDA_ARCHITECTURES=121.TanhF32(previous state)The Gemma 3 and Gemma 4 residual has the same sign in every block while the null arm is centered on zero, so it is a small real cost; pooled it is under the 3 % acceptance line, though one Gemma 3 block alone (+3.7 %) was over it. On CUDA the compiled region still runs as more than one fused kernel; a fully fused kernel is the #2548 item. The MiniCPM-V erf arm keeps the hard-coded tanh window merger, so that row slightly understates the difference.
# prompt_tokensdisagree with mlx-vlm for InternVL, MiniCPM-V 4.6 and Gemma 3, invalidating those cross-runtime comparisons (#2547).Test plan
-p mlxcel-core --lib gelu(24 passed, including the compiled-vs-eager bounds) andactivation_helpers_return_the_input_dtype,--test-threads=1,MLX_ENABLE_TF32=0ffi_tests::shapeless_compile_audit_harnesswithMLXCEL_SHAPELESS_COMPILE_AUDIT=1(compiled_gelu_tanh_f32PASS, 3 signatures warmed)--test cuda_custom_kernel_disk_cachefails on the previous overlay and passes with the fix-p mlxcel --libfiltered to siglip (39), minicpmv4_6 (14), gemma4::activation (2), vision::config (2); earlier on this branch also internvl, molmo2, qwen3_vl, bert, kimi, kokoro, moonvit, and-p mlxcel-xla --lib vision_config emitter::visioncargo clippy --profile test-fast --features cuda -p mlxcel-core --lib --testsand-p mlxcel --lib --testswith-D warnings;cargo fmt --all --check--imagesmoke on minicpm-v-4.6-bf16, gemma-3-4b-it-4bit, paligemma2-3b-6bit, gemma-4-e2b-it-4bit, internvl3-1b (compiled and eager builds, identical text); server request and DiffusionGemma CLI on the earlier commitUnverified: Metal (compiled path and overlay untouched there, not run); EmbeddingGemma2 (no local checkpoint); DiffusionGemma through the server (rejects image input, pre-existing); whole-workspace test and clippy runs.
Remaining families are tracked in #2548. Criteria 3, 4 and 6 are met only for the measured families, so this PR does not close the issue.
Refs #2423