GPU rules for vcat, cat, hcat, permutedims.#1225
Conversation
| SkipConnection(Dense(2 => 2), vcat), | ||
| randn(Float32, 2, 3), | ||
| "SkipConnection(Dense(2 => 2), vcat)", | ||
| ), # vcat tangent mixes CPU/GPU: Illegal conversion of CUDA.DeviceMemory to Ptr{Float32} |
There was a problem hiding this comment.
The rules for vcat/hcat/cat on CuArrays are left unimplemented on purpose — if the underlying issue has been fixed, let's investigate what changed.
There was a problem hiding this comment.
So I tested further by disabling the rules, the three functions being diffed fail as the following places:
vcat->Base._typed_vcat->CUDAKernels.##_#6->kernel_compile->cufunction(containsBase.@lock (try/finally)hcat->Base._typed_hcat->CUDAKernels.##_#6->kernel_compile->cufunctioncat->Base._copy_or_fill!->CUDAKernels.##_#6->kernel_compile->cufunction
The root cause is Base.@lock inside cufunction (used for thread-safe kernel caching), which Mooncake can't handle in reverse mode due to the try/finally block. I tried adding a @zero_derivative rule for cufunction (to skip past the @lock), and then prepare_cuda_state (skip cuCtxGetCurrent ccall inside). After both, CUDAKernels.##_#6 itself then hits cuOccupancyMaxPotentialBlockSize (raw ccall). (this continues)
The explicit rules at the vcat/hcat/cat level might be the right intervention point. But in case Im seeing something incorrectly, happy to be corrected to the right approach ahead.
There was a problem hiding this comment.
for clarity, I'm of the understanding that the underlying issue was handling the relevant CUDA ccalls along these paths. Is it preferred to mark all these calls as non differentiable/with their respective rules?
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
vcat, cat, hcat, permutedims and NNLib.batchnormvcat, cat, hcat, permutedims.
| _throw_mixed_cat_error($_fn) | ||
| end | ||
| @eval function rrule!!( | ||
| ::CoDual{typeof($_fn)}, ::CoDual{<:CuMaybeComplexArray}, ::CoDual{<:AbstractArray} |
There was a problem hiding this comment.
this only cover the two argument case
| function rrule!!( | ||
| ::CoDual{typeof(Core.kwcall)}, | ||
| kw::CoDual{<:NamedTuple}, | ||
| ::CoDual{typeof(cat)}, | ||
| args::CoDual{<:CuMaybeComplexArray}..., | ||
| ) | ||
| pkw = primal(kw) | ||
| dim = pkw.dims | ||
| primals = map(primal, args) | ||
| fdatas = map(tangent, args) | ||
| out = cat(primals...; pkw...) | ||
| dy_out = zero(out) | ||
| pb!!(::NoRData) = ( | ||
| _cu_concat_pb!(fdatas, dy_out, dim); | ||
| (NoRData(), NoRData(), NoRData(), map(_ -> NoRData(), args)...) | ||
| ) | ||
| return CoDual(out, dy_out), pb!! | ||
| end |
There was a problem hiding this comment.
cat's dim argument could take Tuple and Val, this assumes dims to be a single integer?
| end | ||
| end | ||
|
|
||
| @is_primitive(MinimalCtx, Tuple{typeof(vcat),Vararg{CuMaybeComplexArray}}) |
There was a problem hiding this comment.
old error message functions would match Numbers, consider add them back for better error message?
| # Vararg{CuMaybeComplexArray} rules above, so pure-GPU calls still dispatch to those rules. | ||
| # For mixed GPU/CPU calls _throw_if_any_cu detects the CuArray at any position and throws. | ||
| # For pure-CPU calls the rules compute correct pullbacks using the same _cu_concat_pb! helper. | ||
| @is_primitive(MinimalCtx, Tuple{typeof(vcat),Vararg{AbstractArray}}) |
There was a problem hiding this comment.
I am not sure AbstractArray is a good idea: could you verify view, adjoints etc. not breaking on CPU with the extension loaded?
There was a problem hiding this comment.
The PR is still WIP, will request a review when ready
There was a problem hiding this comment.
changed the PR to draft as well for this, I think this needs more thought. sorry for the delay
94960b3 to
b26d1a0
Compare
- Restore the mixed CPU/GPU device guard (deleted in b26d1a0), this time only intercepting genuinely mixed calls, so pure-CPU/pure-GPU are unaffected. - Support cat(...; dims=(1,2)) (block-diagonal), not just Integer/Val. - Support Adjoint/Transpose/SubArray wrapping a CuArray. - Support Integer/Bool-eltype CuArrays. - Add matching tests.
…A is loaded - Replace @generated _is_primitive with a plain, defensive method (handles Vararg/UnionAll call-site types safely); widen it to also cover GPU+scalar mixing at any arity, fixing the N-arg regression and dropping ~18 now-dead methods. - Exclude Float16 from CuMaybeWrappedArray's SubArray branch (arrayify doesn't support it); reword the error message; fix a zero-arg edge case. - Restore deleted flux.jl comments; add a regression test; bump version.
There was a problem hiding this comment.
Great work, we are really close.
Two smaller items left worth fixing:
1. CuMaybeWrappedArray declares ComplexF16 wrappers the rule can't handle
ext/MooncakeCUDAExt/MooncakeCUDAExt.jl:127-131
The Adjoint/Transpose arms use CuFloatOrComplex, which includes Complex{Float16}:
Adjoint{<:CuFloatOrComplex,<:CuMaybeComplexArray},
Transpose{<:CuFloatOrComplex,<:CuMaybeComplexArray},But these rules canonicalise through arrayify, whose Adjoint/Transpose methods
(src/rules/blas.jl) are bounded Union{IEEEFloat,BlasFloat}, and ComplexF16 is not in
that set. So Adjoint{ComplexF16}/Transpose{ComplexF16} — and bare CuArray{ComplexF16}
(CuMaybeComplexArray on L128) — are declared primitives but then fail inside the rule
(MethodError from arrayify, or a kernel exception), instead of falling through to the
interpreter like other unsupported cases.
Fix: bound the complex arms to ComplexF32/ComplexF64 so the primitive declaration matches
what the rule can actually canonicalise, mirroring the BlasFloat bound already used on the
SubArray arm (L131).
Also fix the comment at L119-126, which is inaccurate on two points:
- The example
vcat(view(cu_f16_vec, 1:4), y)— a 1-D contiguousviewof a CuArray
returns aCuArray, not aSubArray, so it is unaffected by theSubArraybound and
works. Use a strided/2-D view (e.g.view(cu_f16_mat, 1:2, :)) to illustrate the point. - "excludes Float16/Complex{Float16}, unlike its Adjoint/Transpose/Symmetric siblings" —
the siblings do handleFloat16but notComplex{Float16}(sameUnion{IEEEFloat, BlasFloat}bound). Tightening the complex arms above makes this consistent.
2. The f_view test does not exercise what it claims
test/ext/cuda/cuda.jl:1325-1327
# Float16 SubArrays are excluded from CuMaybeWrappedArray (Finding 3);
# confirm this now falls through cleanly instead of erroring.
f_view(x, y) = sum(vcat(view(x, 1:2), y))view(x16, 1:2) on a 1-D contiguous CuArray returns a CuArray, not a SubArray, so this
hits the normal pure-GPU vcat rule and duplicates the "Bare Float16" test just above it
(L1314). It never touches the SubArray-exclusion path the comment describes.
Fix: to test the exclusion, use a strided view that stays a SubArray
(e.g. view(cu_f16_mat, 1:2, :)) and assert it errors (@test_throws); otherwise remove
the test as redundant.
|
Don't release yet until we look into the CI failures |
CI Summary — GitHub Actions
Documentation Preview
Mooncake.jl documentation for PR #1225 is available at:
https://chalk-lab.github.io/Mooncake.jl/previews/PR1225/
Performance
Performance Ratio:
Ratio of time to compute gradient and time to compute function.
Warning: results are very approximate! See here for more context.