Add CUDA specific copy rules#3363
Conversation
|
the dot case implies that this is defined on the wrong method. For now don't worry about gpuarrays. Define rules for https://github.com/JuliaGPU/CUDA.jl/blob/cac0ac97adcd43c23abf0b3c8bb3f99364d9b590/CUDACore/lib/cudadrv/memory.jl#L421 and and and similar unsafe_copyto down |
So IIUC the current methods I defined rules for are a level too high. The current extension I wrote these in does not have CUDA as a dep --- should I add it as a weakdep, or move this PR to Also, assuming the pointer-level rules subsume it, should I drop the array-level rule I have here or keep it as a fallback? |
|
Yeah basically. Re where they live either is a reasonable choice -- mostly one of whose methods/internals seem more used. In the case of the current cuda kernel launch rules those require a lot of the cuda internals so make sense to live there. These don't really have any internals so could live anywhere imo. A relevant fyi, current cuda.jl main doesn't pkg resolve with enzyme atm due to the GPUCompiler 2 bump, though also ci there isn't running regardless and @kshyatt was looking into that . Maybe makes sense to have here then. But I'm fine either way |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3363 +/- ##
==========================================
+ Coverage 75.04% 76.81% +1.76%
==========================================
Files 66 66
Lines 22852 22326 -526
==========================================
- Hits 17150 17149 -1
+ Misses 5702 5177 -525 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@wsmoses i think this is ready |
| return EnzymeRules.width(config) == 1 ? x.dval : x.dval[batch] | ||
| end | ||
|
|
||
| function EnzymeRules.augmented_primal( |
There was a problem hiding this comment.
I think this is needed---I tried removing it and all the copy-rule gradients come back zero. Without it the reverse unsafe_copyto! rules accumulate into an unconnected pointer because unlike say pointer(::Array), pointer(::CuArray) isn't an intrinsic Enzyme shadows (at least that is how i understand it)
There was a problem hiding this comment.
Er that is a bug then, can you upload an isolated as mwe possible case with enzyme.api.printall!(true) on
| using LinearAlgebra: dot | ||
| using Test | ||
|
|
||
| @testset "CUDA memory copies" begin |
There was a problem hiding this comment.
Would be good to check this works with all the CUDA memory types (Device, Host, Unified)
There was a problem hiding this comment.
Added device / unified / host coverage. Device passes; unified and host are @test_broken for now. The blocker is the pointer(::StridedCuArray) rule: for unified/host memory pointer(::CuArray). This is related to the other issue i am trying to figure out so leaving it as a flag for now
|
Same comment as #3375 (comment) the CUDA API ones should live in EnzymeAD/Enzyme? |
|
@wsmoses here is the MWE. using CUDA, Enzyme
f(x) = sum(abs2, Array(CuArray(x)))
x = Float32[1, 2, 3]
dx = zeros(Float32, 3)
get(ENV, "PRINTALL", "0") == "1" && Enzyme.API.printall!(true)
Enzyme.autodiff(Reverse, f, Active, Duplicated(x, dx))
Enzyme.API.printall!(false)
println("dx = ", dx)
println("expected = ", 2 .* x)
println("correct? = ", dx == 2 .* x)For me, with |
| function _zero!(ptr::CuArrayPtr{T}, off::Integer, n::Integer) where {T} | ||
| n == 0 && return nothing | ||
| buffer = _stage(ptr, off, n) | ||
| fill!(buffer, zero(eltype(buffer))) |
There was a problem hiding this comment.
this can call the same zero as CuPtr{T}, right?
There was a problem hiding this comment.
i do not think so as memset is defined only for CuPtr{T}
There was a problem hiding this comment.
i would assume a convert or something exists given https://github.com/JuliaGPU/CUDA.jl/blob/8fc49feda7cf563fa9fb4900e329dfecc99ed0fd/CUDACore/src/array.jl#L848
There was a problem hiding this comment.
hmm i tried the following:
using CUDA
mem = CUDA.alloc(CUDA.ArrayMemory{Float32}, (4,))
convert(CuPtr{Float32}, mem) # ERROR: ArgumentError: Illegal conversion of a ArrayMemory{Float32, 1} to a CuPtr{Float32}
Base.unsafe_convert(CuPtr{Float32}, mem) # ERROR: ArgumentError: Illegal conversion of a ArrayMemory{Float32, 1} to a CuPtr{Float32}
CUDA.memset(reinterpret(CuPtr{UInt8}, pointer(mem)), UInt8(0), 16) # ERROR: CUDA error: invalid argument (code 1, ERROR_INVALID_VALUE)If i understand the code snippet linked, pointer(A) is already a CuPtr, because A is a DenseCuArray. So convert(CuPtr{U}, pointer(A)) is a CuPtr -> CuPtr retype
There was a problem hiding this comment.
this just feels like a missing memset impl in cuda.jl [and we should open an issue for]?
### Array memory
Array memory is a special kind of memory that is optimized for 2D and 3D access patterns. The memory is opaquely managed by the CUDA runtime, and is typically only used on combination with texture intrinsics.
``@docs
CUDA.ArrayMemory
CUDA.alloc(::Type{CUDA.ArrayMemory{T}}, ::Dims) where T
``
### Pointers
To work with these buffers, you need to `convert` them to a `Ptr`, `CuPtr`, or in the case
of `ArrayMemory` an `CuArrayPtr`. You can then use common Julia methods on these pointers,
such as `unsafe_copyto!`. CUDA.jl also provides some specialized functionality that does not
match standard Julia functionality:
``@docs
CUDA.unsafe_copy2d!
CUDA.unsafe_copy3d!
CUDA.memset
``
In any case let's make our lives easier then and just leave the method unimplemented for CuArrayPtr for now
| end | ||
|
|
||
| @inline _commit!(::Union{Ptr, CuPtr}, offset, buffer, n) = nothing | ||
| function _commit!(ptr::CuArrayPtr, offset, buffer, n) |
There was a problem hiding this comment.
what is this whole commit thing and why is it needed?
| n == 0 && return nothing | ||
| typeof(src) === typeof(dest) && src == dest && soff == doff && return nothing | ||
|
|
||
| dsrc = _stage(src, soff, n) |
There was a problem hiding this comment.
get rid of the stage/commit stuff
| axpy!(one(eltype(dst)), src, dst) | ||
| return nothing | ||
| end | ||
| _accumulate!(dst::Array, src::Array) = (dst .+= src; nothing) |
There was a problem hiding this comment.
we should benchmark if we should still use axpy here, I would hope it to be better than broadcasting?
There was a problem hiding this comment.
eltype n broadcast(us) axpy(us) speedup(bcast/axpy)
Float32 1 7.896 13.734 0.57
Float32 16 8.115 13.123 0.62
Float32 256 8.668 13.615 0.64
Float32 4096 8.096 13.318 0.61
Float32 65536 8.483 13.476 0.63
Float32 1048576 8.266 13.597 0.61
Float32 16777216 96.727 95.242 1.02
Float64 1 9.326 13.458 0.69
Float64 16 9.092 13.423 0.68
Float64 256 9.014 14.109 0.64
Float64 4096 8.579 13.794 0.62
Float64 65536 8.256 14.423 0.57
Float64 1048576 9.157 13.786 0.66
Float64 16777216 256.104 257.62 0.99
apparently not ?
There was a problem hiding this comment.
s a v a g e.
but sure, add a comment that we could do axpy [called like X], but it is faster [copying this table] to use a bcast
| dsrc = _stage(src, soff, n) | ||
| ddest = _stage(dest, doff, n) | ||
| if dsrc isa CuArray && ddest isa CuArray | ||
| _accumulate!(dsrc, ddest) |
There was a problem hiding this comment.
just do this inside overrides of _accumulate!
|
|
||
| function _accumulate_and_zero!(src, soff, dest, doff, n::Integer) | ||
| n == 0 && return nothing | ||
| typeof(src) === typeof(dest) && src == dest && soff == doff && return nothing |
@wsmoses added these copy rules per your suggestion and my understanding. Here is what it now allows that prior would break:
Array(x)/collect(x)pulling aCuArrayback to host mid-function. before this it'd just die with "no augmented/forward pass found for cuMemcpyDtoHAsync_v2". e.g., both fwd and revsum(abs2, Array(x))gives back 2x like you'd want.CuArray(y)/copyto!(gpu, host)however, this did not fix #3235 and need further fixes for
sum(abs2, CuArray(x))dotalso failsI figured i wouldn't add anything beyond these copy rules unless you think its in scope for this PR otherwise i can continue to target this