Repository navigation
fix(rocm): clear the nine gfx1151 test failures in verify-rocm - #2613
Merged
Merged
Conversation
The hiprtc preamble of the ROCm compiled-kernel builder defined no complex64_t, although the shared codegen names complex64 values that way and emits complex64_t constants and casts. Every compiled graph that touched complex64 failed to build ("unknown type name 'complex64_t'"), so compiled real(z) and imag(z) threw where eager evaluation worked. Real, Imag and Conjugate were also identity or zero stubs written for real inputs.
The preamble now defines complex64_t with the hipFloatComplex layout, explicit conversions to and from real types, arithmetic and comparison operators, and complex64 overloads for Add, Subtract, Multiply, Divide, Square, Abs, Real, Imag and Conjugate. Float-promoting op templates without a complex overload assert on complex64, so an unsupported complex graph fails to build loudly instead of computing on the real part. Recorded as LOCAL_FIXES.md item 59.
Six tests loaded an MLX-native NVFP4 expert plane and failed on ROCm because the load is refused there by design: the capability table marks NVFP4 as ConvertTo(Affine) on ROCm, and only ModelOpt checkpoints are converted at load (#1806). A new expect_native_nvfp4_load test helper returns the layer where NVFP4 is native and, elsewhere, asserts that the refusal names the plane and the mode, prints the skip, and stops the test before the parts that need the loaded layer. The backend-independent assertions (sidecar stacking, the split's per-expert scale on the weight map) still run on every backend. fused_expert_kernel_keeps_sparsemixer_scores compared the fused kernel against the bf16 graph path. The fixture cancels hard in the down projection, so the graph path's bf16 rounding of gate, up and the activation alone puts it about 2% from the all-f32 result, on the test's 2% bound and over it on ROCm (2.2%), while the fused kernel is within 0.2%. The reference now runs the graph path in f32; a renormalizing kernel would still be 1.5x away.
The complex64_t added to the hiprtc preamble converted to every real type through one templated explicit conversion that keeps the real part. For bool that differs from MLX CPU and CUDA, where a complex value is true when either part is nonzero, so a fused astype(z, bool) or logical_not(z) returned false for 0+1j. NaNEqual had no complex64 overload and its float template compared real parts only, so a compiled array_equal(..., equal_nan=True) ignored the imaginary part. Before #2608 both graphs failed to build; with the new type they returned wrong results silently. complex64_t gains a non-template explicit operator bool, which overload resolution prefers over the template, and NaNEqual gains a complex64 overload with the MLX CUDA and Metal rule. LOCAL_FIXES.md item 59 records both. Validated by compiling the extracted preamble device-only for gfx1151 with hipcc (real-typed overload results unchanged, unsupported ops still hit MLX_ROCM_REAL_ONLY) and a host build of the type and NaNEqual checking the bool and NaN cases. No GPU run. Refs #2608
inureyes
force-pushed
the
fix/issue-2608-rocm-test-failures
branch
from
October 26, 2026 06:15
4aeee28 to
5e803e5
Compare
MLX's abs() builds an Abs node whose dtype stays complex64 and casts it to float32 afterwards, so the codegen declares the Abs result as complex64_t. The complex overload returned float, and the explicit complex64_t(float) constructor made every fused abs of a complex64 fail to JIT. It now returns complex64_t(|z|, 0), as MLX CUDA does, and LOCAL_FIXES item 59 says so. A new compile probe graph (abs(z) * 2 + a) and compiled_abs_of_complex64_matches_eager cover it: on gfx1151 the test fails with the previous overlay and passes with this one, and the 43 mlxcel-core compiled tests pass. Refs #2608
Member
Author
|
Pre-merge follow-up from the orchestrator review:
|
inureyes
marked this pull request as ready for review
October 26, 2026 06:25
This was referenced Oct 26, 2026
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
Fixes the nine
make verify-rocmfailures on gfx1151 from #2608. Reproduced on main at 773aa55. On macOS (Nightly verify run 39703647781 at 414ae12) all nine pass, so every failure is ROCm-only. Hosted CI on main runs no test jobs right now, so there is no CUDA data point.Root causes and resolution
Group B: compiled
real/imagof complex64 (mlxcel-core), fixed.compiled_dtype_cast_tests::compiled_real_of_complex64_matches_eagercompiled_dtype_cast_tests::compiled_imag_of_complex64_matches_eagerCause: the hiprtc preamble (
g_jit_includesinoverlays/rocm/mlx/backend/rocm/compiled.cpp) defined nocomplex64_t, although the shared codegen names complex64 values that way and emitscomplex64_t(re, im)constants andstatic_castforAsType. Every fused kernel touching complex64 failed to JIT ("unknown type name 'complex64_t'").Real,ImagandConjugatewere also identity or zero stubs for real inputs.Fix: the preamble defines
complex64_t(hipFloatComplex layout, explicit real conversions, arithmetic and comparison operators) with complex64 overloads forAdd,Subtract,Multiply,Divide,Square,Abs,Real,ImagandConjugate.complex64_ttoboolis true when either part is nonzero, andNaNEqualhas a complex64 overload, both matching MLX CPU and CUDA (added during review: the generic real-part conversion would otherwise have made a fusedastype(z, bool)andarray_equal(..., equal_nan=True)silently wrong). Float-promoting op templates with no complex overload nowstatic_asserton complex64 (MLX_ROCM_REAL_ONLY), so an unsupported complex graph fails to build with a message instead of silently computing on the real part. Recorded asLOCAL_FIXES.mditem 59. It is a backend file, soCORE_RESIDUAL.diffis unchanged.Group A, NVFP4 loads (
mlxcel): confirmed backend gap, gated.models::switch_layers::global_scale_tests::native_global_scale_matches_dense_dequantized_referencemodels::switch_layers::global_scale_tests::per_expert_sidecars_stack_into_the_output_multipliermodels::switch_layers::global_scale_tests::a_plane_whose_family_left_the_modelopt_sidecars_unstacked_is_refusedloading::vlm::gemma_unified::tests::unified_nvfp4_fused_experts_keep_their_native_global_scalemodels::mistral4::modelopt_stack_tests::modelopt_expert_sidecars_stack_into_the_output_multipliermodels::phimoe::sanitize_tests::modelopt_expert_sidecars_stack_into_the_output_multiplierCause: each test loads an MLX-native NVFP4
SwitchLinear/SwitchGLUplane. ROCm has no NVFP4 kernel:GpuBackendKind::quant_mode_supportmarks itConvertTo(Affine)andvalidate_quantization_mode_runnablerefuses such a plane at load by design (#1806). Only ModelOpt checkpoints are converted. The ROCmGatherQMMalso has no input for the per-expert global scale, so this is a missing kernel, not a regression.Fix: a new test helper,
switch_layers::expect_native_nvfp4_load, returns the layer where NVFP4 is native. On other backends it asserts that the refusal names the plane andnvfp4, prints a skip that names the gap and #1806, and returnsNone, so the test stops before the parts that need the loaded layer. The parts that do not depend on the backend still run everywhere: sidecar stacking intoinv_global_scale, the "left unstacked" refusal, and, in the gemma-unified test, the split's per-expert native scale, now also checked directly on the weight map. Other tests already branch onquant_mode_support(Nvfp4)the same way.Group A, PhiMoE fused expert kernel: test reference fixed.
models::phimoe_tests::fused_expert_kernel_keeps_sparsemixer_scoresCause: the test compared the fused kernel against the bf16 graph path with a 2% bound. With
MLXCEL_FUSED_MOE_PARITY_CHECK=1on gfx1151: fused vs all-f32 truth 0.17%,gather_qmmgraph chain vs truth 2.2%, fused vs graph 2.2%. The fixture cancels hard in the down projection (|activation| about 176 against |output| about 12). Rounding the activation to bf16 alone moves the output 0.7%, and the bf16 gate/up/activation chain moves it several percent. A singlequantized_matmulmatches f32 to bf16 rounding (0.16%), so neither the ROCm kernel nor the fused kernel is wrong. The bf16 reference was simply on the bound.Fix: the reference runs the graph path in f32 (
xcast to f32,moe_weighted_sumto f32). A kernel that renormalized the sparsemixer scores (sum 1.5025) would still be 1.5x away and fail.Verification (gfx1151, all GPU runs under
scripts/rocm_gpu_guard.sh --hold)mlxcel-corelibcompiled_dtype_cast_tests: 7/7 pass (2 failed before). The broadercompiledfilter: 42/42 pass inmlxcel-coreand 8 pass (1 ignored) inmlxcel, re-run after the review commit.mlxcellibmodels::switch_layers57 pass (1 ignored, needs a checkpoint),models::phimoe14/14,loading::vlm::gemma_unified9/9,models::mistral42/2cargo clippy --profile test-fast --features rocm -p mlxcel -p mlxcel-core --lib --tests -- -D warnings: clean.cargo fmt --all -- --check: cleanverify-rocm-overlay:rocm_overlay.py recordsOK (59 entries),rocm_overlay_test.shOK,allocator_ownership_test.shandrocm_runtime_dir_test.shpass,rocm_overlay.py driftOKmake verify-rocm-heldgate runs centrally before mergeCloses #2608