Repository navigation
Fix isblasmatrix for GPU StridedViews - #96
Conversation
|
I got a little carried away here. I think we can get rid of the vendor specific extensions and just use |
|
Why is this packed columns (essentially: contiguous data) necessary. I thought |
|
The vendor |
But doesn't that mean precisely that |
In fact this turned out not to be right, the issue is also that my |
| function _densematrix(A::GPUStridedView{T, 2}) where {T} | ||
| A2, CA = Strided.getblasmatrix(A) | ||
| m, n = size(A2) | ||
| lda = max(stride(A2, 2), m, 1) | ||
| M = view(GPUArrays.derive(T, parent(A2), (lda, n), A2.offset), 1:m, 1:n) | ||
| return CA == 'N' ? M : CA == 'T' ? transpose(M) : adjoint(M) | ||
| end | ||
|
|
||
| function Strided.blas_mul!( | ||
| C::GPUStridedView{T, 2}, A::GPUStridedView{T, 2}, B::GPUStridedView{T, 2}, | ||
| α::Number, β::Number | ||
| ) where {T <: LinearAlgebra.BlasFloat} | ||
| isempty(C) && return C | ||
| LinearAlgebra.mul!(_densematrix(C), _densematrix(A), _densematrix(B), convert(T, α), convert(T, β)) | ||
| return C | ||
| end |
There was a problem hiding this comment.
Does this still need updating to be similar to cuBLAS?
There was a problem hiding this comment.
This one is only reached by Metal (or JLArrays) right now I believe? I'm a bit unclear what this comment is asking, sorry!
There was a problem hiding this comment.
Sorry, what I meant is that the code looks somewhat different here in comparison to the cuBLAS extension, with different auxiliary function names as well, do we want to uniformize this or is it on purpose?
There was a problem hiding this comment.
Let me see if I can't get everyone to share these...
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
I think our original approach here was wrong, because we can use the BLAS methods as long as the view that's passed is densely packed. So right now we aren't using BLAS where we could (a dense patch of a higher dimensional tensor, for example) and we are trying to use it in dangerous places. Hopefully this fixes both.