Test all storage modes #314
Conversation
| @@ -170,14 +181,16 @@ end | |||
|
|
|||
| function Base.unsafe_convert(::Type{MtlPointer{T}}, x::MtlArray) where {T} | |||
| buf = x.data[] | |||
| synchronize() | |||
There was a problem hiding this comment.
I don’t think we need to synchronize here. MtlPointers will only be used on device.
There was a problem hiding this comment.
Without this the kernelabstractions copyto test fails.
There was a problem hiding this comment.
So the problem is, that pointer(A) might mean device or host pointer. #316 might fix that.
There was a problem hiding this comment.
Here is what CUDA.jl does https://github.com/JuliaGPU/CUDA.jl/blob/bb49887198f258ffcb186d81df4a787453428b38/src/CUDAKernels.jl#L41
There was a problem hiding this comment.
Since this PR passes all tests, I suggest we merge this and then you can finish up the fix in #316.
There was a problem hiding this comment.
FWIW, it's really not acceptable to sync on every pointer conversion, as that would remove almost all opportunity for asynchronous execution (as most GPU operations involve a conversion to a pointer).
There was a problem hiding this comment.
Addresses #312 (comment) but does not fix #312.
it does seem to me like your changes would fix #312
I commented that before I decided to submit the fixes. The |
christiangnrd
force-pushed
the
test-storagemodes
branch
from
229b815
to
2886b8f
Compare
christiangnrd
force-pushed
the
test-storagemodes
branch
from
355a86e
to
a3f3377
Compare
|
I think everything in this PR is ready to go except isn't it a bit of a code smell for there to be a Edit: Looking at the CUDA.jl code, seems like it's normal. |
| if is_private(x) | ||
| throw(ArgumentError("cannot take the CPU address of a $(typeof(x))")) | ||
| end | ||
| synchronize() |
There was a problem hiding this comment.
It would probably be good to port the opportunistic synchronization from CUDA.jl
There was a problem hiding this comment.
Can that be a separate PR? I don't have time at the moment to look into that and this PR in its current state seems to fix #312 and make CI more robust.
Addresses #312 (comment) and closes #312.