Repository navigation
Fix BLAS calls on views oneMKL can't address - #724
Merged
Merged
Conversation
The BLAS-1 wrappers take any strided array and pass it to oneMKL as `length(x)` elements with increment `stride(x, 1)`. For a view of a matrix that skips rows that layout is wrong, so `norm`, `rmul!`, `axpy!`, `axpby!`, `rotate!` and `reflect!` returned wrong results and wrote outside the view. Limit the wrappers to strided vectors and contiguous arrays, and have the LinearAlgebra methods fall back to broadcasts over the flattened arrays for anything else, since BLAS pairs elements linearly regardless of shape. The ComplexF16 variants of `scal!` and `axpy!` compute in ComplexF32 and wrote the result back through `convert`, which only works for a dense array; use a broadcast instead. `dotc` and `dotu` likewise widen with a broadcast now, so they accept views.
oneMKL follows BLAS and walks a vector with a negative increment backwards from its lowest address, while the wrappers passed the pointer of a reversed view's first element, the highest one. As a result `norm`, `dot`, `BLAS.asum`, `axpy!`, `axpby!`, `rmul!`, `rotate!` and `reflect!` on `view(x, n:-1:1)` returned wrong results or accessed the wrong memory. Pass a pointer to the lowest address together with the increment, like LinearAlgebra's `vec_pointer_stride`, and use the absolute increment for nrm2, asum and scal, where the order doesn't matter. iamax and iamin report the first of several extrema, so they run on a contiguous copy of a reversed vector instead. This also fixes direct calls like `oneMKL.nrm2(r)`. Fixes #712
…thods The storage-level `mul!` methods passed any strided `SubArray` to oneMKL with `lda = max(1, stride(A, 2))` and `incx = stride(x, 1)`, and the triangular `lmul!`/`ldiv!`/`rmul!`/`rdiv!` methods checked nothing. Views that skip rows therefore gave silently wrong results, reversed vectors were walked from the wrong end, and reversed columns made oneMKL throw `invalid_argument` for `lda`, which escapes the support library's C interface and aborts the process. The triangular methods were reachable through public API, e.g. `lmul!(UpperTriangular(A), view(x, n:-1:1))`. Matrices, and vectors passed as matrices, now need a leading dimension of at least the number of rows, and vectors a positive stride. `mul!` falls back to `GPUArrays.generic_matmatmul!` otherwise, as it already did for other unsupported layouts. The matrix-vector `mul!` now also accepts a strided output vector, which the layout check keeps away from oneMKL when needed. The triangular methods run on contiguous copies and copy the result back. Fixes #700
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #724 +/- ##
==========================================
+ Coverage 83.13% 83.98% +0.85%
==========================================
Files 52 51 -1
Lines 4376 4322 -54
==========================================
- Hits 3638 3630 -8
+ Misses 738 692 -46 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
The BLAS-1 methods (
norm,dot,BLAS.asum,axpy!,axpby!,rmul!,rotate!,reflect!) and the storage-levelmul!and triangular methods pass any strided view to oneMKL withincx = stride(x, 1)andlda = max(1, stride(A, 2)), without checking that oneMKL can address that layout. Reversed views get a negative increment, which BLAS walks backwards from the lowest address while the view's pointer is at its highest element, so the result is silently wrong. Views that skip rows get passed aslength(x)elements with a single increment, which reads and writes outside the view. Reversed columns give a negativelda, and oneMKL'sinvalid_argumentescapes the support library's C interface and aborts the process:The triangular case is reachable through public API, e.g.
lmul!(UpperTriangular(A), view(x, n:-1:1)).This ports JuliaGPU/CUDA.jl#3411 and JuliaGPU/CUDA.jl#3409. The level-1 wrappers now take strided vectors or dense arrays only, and pass a pointer to the lowest address together with the signed increment, like LinearAlgebra's
vec_pointer_stride; nrm2, asum, scal, iamax and iamin use the absolute increment since the order doesn't matter (oneMKL rejects a negative one for iamax/iamin, so their index is flipped back). The LinearAlgebra BLAS-1 methods fall back to broadcasts for strided arrays that aren't vectors or dense. Storage-levelmul!only calls oneMKL when every matrix has a unit row stride and a leading dimension of at least its row count, and every vector a positive stride, falling back toGPUArrays.generic_matmatmul!otherwise as it already did for unsupported element types. The triangular methods run on contiguous copies and copy the result back.Not covered: the level-2 wrappers (
gemv!,symv!, ...) still take raw strides when called directly, as in CUDA.jl, and the support library should turnoneapi::mkl::exceptioninto an error code instead of aborting, which needs a JLL rebuild.Tested on an Iris Xe (Julia 1.12): the reproducers from both issues fail on main and pass here, and the new tests cover reversed and row-skipping views for every BLAS-1 method, the storage-level
mul!paths (gemm, gemv, symm, hemm, symv, hemv) and the triangular products and solves.Fixes #712
Fixes #700