Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seperate LinearAlgebra.axp(b)y! and BLAS.axp(b)y!. #44758

Merged
merged 3 commits into from
Apr 2, 2022

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Mar 26, 2022

At present we have BLAS.axp(b)y! === LinearAlgebra.axp(b)y!, which seems strange as BLAS.axp(b)y! should always call BLAS.
This PR seperate them, and make LinearAlgebra.axp(b)y! call BLAS.axp(b)y! if inputs are stride-vector like. (Thus there should be no performance regression.)

@N5N3 N5N3 added the linear algebra Linear algebra label Mar 26, 2022
Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ViralBShah ViralBShah merged commit 567ae8c into JuliaLang:master Apr 2, 2022
@N5N3 N5N3 deleted the seperate-axpy! branch April 2, 2022 15:52
@ViralBShah
Copy link
Member

I'm seeing this when building the docs. I presume this PR is causing it.

┌ Warning: duplicate docs found for 'LinearAlgebra.BLAS.axpy!' in `@docs` block in src/stdlib/LinearAlgebra.md:529-586
│ ```@docs
│ LinearAlgebra.BLAS
│ LinearAlgebra.BLAS.dot
│ LinearAlgebra.BLAS.dotu
│ LinearAlgebra.BLAS.dotc
│ LinearAlgebra.BLAS.blascopy!
│ LinearAlgebra.BLAS.nrm2
│ LinearAlgebra.BLAS.asum
│ LinearAlgebra.BLAS.axpy!
│ LinearAlgebra.BLAS.axpby!
│ LinearAlgebra.BLAS.scal!
│ LinearAlgebra.BLAS.scal
│ LinearAlgebra.BLAS.iamax
│ LinearAlgebra.BLAS.ger!
│ LinearAlgebra.BLAS.syr!
│ LinearAlgebra.BLAS.syrk!
│ LinearAlgebra.BLAS.syrk
│ LinearAlgebra.BLAS.syr2k!
│ LinearAlgebra.BLAS.syr2k
│ LinearAlgebra.BLAS.her!
│ LinearAlgebra.BLAS.herk!
│ LinearAlgebra.BLAS.herk
│ LinearAlgebra.BLAS.her2k!
│ LinearAlgebra.BLAS.her2k
│ LinearAlgebra.BLAS.gbmv!
│ LinearAlgebra.BLAS.gbmv
│ LinearAlgebra.BLAS.sbmv!
│ LinearAlgebra.BLAS.sbmv(::Any, ::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.sbmv(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.gemm!
│ LinearAlgebra.BLAS.gemm(::Any, ::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.gemm(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.gemv!
│ LinearAlgebra.BLAS.gemv(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.gemv(::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.symm!
│ LinearAlgebra.BLAS.symm(::Any, ::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.symm(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.symv!
│ LinearAlgebra.BLAS.symv(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.symv(::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.hemm!
│ LinearAlgebra.BLAS.hemm(::Any, ::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.hemm(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.hemv!
│ LinearAlgebra.BLAS.hemv(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.hemv(::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.trmm!
│ LinearAlgebra.BLAS.trmm
│ LinearAlgebra.BLAS.trsm!
│ LinearAlgebra.BLAS.trsm
│ LinearAlgebra.BLAS.trmv!
│ LinearAlgebra.BLAS.trmv
│ LinearAlgebra.BLAS.trsv!
│ LinearAlgebra.BLAS.trsv
│ LinearAlgebra.BLAS.set_num_threads
│ LinearAlgebra.BLAS.get_num_threads
│ ```
└ @ Documenter.Expanders ~/julia/doc/deps/packages/Documenter/9rkAd/src/Expanders.jl:315
┌ Warning: duplicate docs found for 'LinearAlgebra.BLAS.axpby!' in `@docs` block in src/stdlib/LinearAlgebra.md:529-586
│ ```@docs
│ LinearAlgebra.BLAS
│ LinearAlgebra.BLAS.dot
│ LinearAlgebra.BLAS.dotu
│ LinearAlgebra.BLAS.dotc
│ LinearAlgebra.BLAS.blascopy!
│ LinearAlgebra.BLAS.nrm2
│ LinearAlgebra.BLAS.asum
│ LinearAlgebra.BLAS.axpy!
│ LinearAlgebra.BLAS.axpby!
│ LinearAlgebra.BLAS.scal!
│ LinearAlgebra.BLAS.scal
│ LinearAlgebra.BLAS.iamax
│ LinearAlgebra.BLAS.ger!
│ LinearAlgebra.BLAS.syr!
│ LinearAlgebra.BLAS.syrk!
│ LinearAlgebra.BLAS.syrk
│ LinearAlgebra.BLAS.syr2k!
│ LinearAlgebra.BLAS.syr2k
│ LinearAlgebra.BLAS.her!
│ LinearAlgebra.BLAS.herk!
│ LinearAlgebra.BLAS.herk
│ LinearAlgebra.BLAS.her2k!
│ LinearAlgebra.BLAS.her2k
│ LinearAlgebra.BLAS.gbmv!
│ LinearAlgebra.BLAS.gbmv
│ LinearAlgebra.BLAS.sbmv!
│ LinearAlgebra.BLAS.sbmv(::Any, ::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.sbmv(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.gemm!
│ LinearAlgebra.BLAS.gemm(::Any, ::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.gemm(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.gemv!
│ LinearAlgebra.BLAS.gemv(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.gemv(::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.symm!
│ LinearAlgebra.BLAS.symm(::Any, ::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.symm(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.symv!
│ LinearAlgebra.BLAS.symv(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.symv(::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.hemm!
│ LinearAlgebra.BLAS.hemm(::Any, ::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.hemm(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.hemv!
│ LinearAlgebra.BLAS.hemv(::Any, ::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.hemv(::Any, ::Any, ::Any)
│ LinearAlgebra.BLAS.trmm!
│ LinearAlgebra.BLAS.trmm
│ LinearAlgebra.BLAS.trsm!
│ LinearAlgebra.BLAS.trsm
│ LinearAlgebra.BLAS.trmv!
│ LinearAlgebra.BLAS.trmv
│ LinearAlgebra.BLAS.trsv!
│ LinearAlgebra.BLAS.trsv
│ LinearAlgebra.BLAS.set_num_threads
│ LinearAlgebra.BLAS.get_num_threads

@N5N3
Copy link
Member Author

N5N3 commented Apr 3, 2022

@ViralBShah, I can't reproduce locally.

$ make -C doc
make: Entering directory '/cygdrive/c/Users/MYJ/Documents/Github/julia/doc'
/cygdrive/c/Users/MYJ/Documents/Github/julia/deps/tools/jlchecksum "/cygdrive/c/Users/MYJ/Documents/Github/julia/deps/srccache/UnicodeData-13.0.0.txt"
cp "/cygdrive/c/Users/MYJ/Documents/Github/julia/deps/srccache/UnicodeData-13.0.0.txt" UnicodeData.txt
Building HTML documentation.
/cygdrive/c/Users/MYJ/Documents/Github/julia/usr/bin/julia --startup-file=no --color=yes `cygpath -w /cygdrive/c/Users/MYJ/Documents/Github/julia/doc/make.jl` linkcheck= doctest= buildroot=`cygpath -w /cygdrive/c/Users/MYJ/Documents/Github/julia` texplatform= revise=
[ Info: SetupBuildDirectory: setting up build directory.
[ Info: Doctest: skipped.
[ Info: ExpandTemplates: expanding markdown templates.
[ Info: CrossReferences: building cross-references.
[ Info: CheckDocument: running document checks.
[ Info: Populate: populating indices.
[ Info: RenderDocument: rendering document.
[ Info: HTMLWriter: rendering HTML pages.
┌ Warning: invalid local link: unresolved path in base\file.md
│   link.text =1-element Vector{Any}:
│     Markdown.Code("", "mkpidlock")
│   link.url = "FileWatching.html#FileWatching.Pidfile.mkpidlock"
└ @ Documenter.Writers.HTMLWriter C:\Users\MYJ\Documents\Github\julia\doc\deps\packages\Documenter\9rkAd\src\
Writers\HTMLWriter.jl:2077
┌ Warning: invalid local link: unresolved path in stdlib\LinearAlgebra.md
│   link.text =1-element Vector{Any}:
│     Markdown.Code("", "\\")
│   link.url = "math.html#Base.:\\-Tuple{Any, Any}"
└ @ Documenter.Writers.HTMLWriter C:\Users\MYJ\Documents\Github\julia\doc\deps\packages\Documenter\9rkAd\src\
Writers\HTMLWriter.jl:2077
┌ Warning: invalid local link: unresolved path in stdlib\LinearAlgebra.md
│   link.text =1-element Vector{Any}:
│     Markdown.Code("", "rdiv!")
│   link.url = "../stdlib/LinearAlgebra.html#LinearAlgebra.rdiv!"
└ @ Documenter.Writers.HTMLWriter C:\Users\MYJ\Documents\Github\julia\doc\deps\packages\Documenter\9rkAd\src\
Writers\HTMLWriter.jl:2077
Build finished. The HTML pages are in _build/html.
make: Leaving directory '/cygdrive/c/Users/MYJ/Documents/Github/julia/doc'

@ViralBShah
Copy link
Member

I had mismatched versions - I can't reproduce it either now with a clean build of everything. Thanks for checking.

@maleadt
Copy link
Member

maleadt commented Jan 26, 2023

FWIW, this change broke the dot implementation we have in GPUArrays/oneAPI.jl. MWE:

module Ambig
    using LinearAlgebra

    abstract type AbstractGPUArray{T, N} <: DenseArray{T, N} end
    LinearAlgebra.dot(::AbstractGPUArray, ::AbstractGPUArray) = 1

    struct oneArray{T,N} <: AbstractGPUArray{T,N} end
    oneStridedArray{T,N} = Union{oneArray{N}, Base.ReinterpretArray}
    LinearAlgebra.dot(::oneStridedArray{Float32}, ::oneStridedArray{Float32}) = 2
end

using LinearAlgebra
@show Base.isambiguous(methods(dot, Ambig)[1], methods(dot, Ambig)[2])

This results in an ambiguity on 1.9, while it used to work on 1.8:

Candidates:
  dot(x::oneStridedArray{T}, y::oneStridedArray{T}) where T<:Union{Float32, Float64}
    @ oneAPI.oneMKL ~/Julia/pkg/oneAPI/lib/mkl/linalg.jl:22
  dot(x::GPUArraysCore.AbstractGPUArray, y::GPUArraysCore.AbstractGPUArray)
    @ GPUArrays ~/Julia/pkg/GPUArrays/src/host/linalg.jl:480
  dot(x::Union{Base.ReinterpretArray{T, N, S, A, IsReshaped} where {N, A<:Union{SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real}}, Tuple{AbstractUnitRange, Vararg{Any}}}}, DenseArray}, IsReshaped, S}, Base.ReshapedArray{T, N, A} where {N, A<:Union{Base.ReinterpretArray{T, N, S, A, IsReshaped} where {T, N, A<:Union{SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real}}, Tuple{AbstractUnitRange, Vararg{Any}}}}, DenseArray}, IsReshaped, S}, SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real}}, Tuple{AbstractUnitRange, Vararg{Any}}}}, DenseArray}}, SubArray{T, var"#s970", var"#s969", I, true} where {var"#s970", var"#s969"<:Union{Base.ReinterpretArray{T, N, S, A, IsReshaped} where {N, A<:Union{SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real}}, Tuple{AbstractUnitRange, Vararg{Any}}}}, DenseArray}, IsReshaped, S}, Base.ReshapedArray{T, N, A} where {N, A<:Union{Base.ReinterpretArray{T, N, S, A, IsReshaped} where {T, N, A<:Union{SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real}}, Tuple{AbstractUnitRange, Vararg{Any}}}}, DenseArray}, IsReshaped, S}, SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real}}, Tuple{AbstractUnitRange, Vararg{Any}}}}, DenseArray}}, DenseArray{T}}, I}, DenseArray{T}}, y::Union{Base.ReinterpretArray{T, N, S, A, IsReshaped} where {N, A<:Union{SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real}}, Tuple{AbstractUnitRange, Vararg{Any}}}}, DenseArray}, IsReshaped, S}, Base.ReshapedArray{T, N, A} where {N, A<:Union{Base.ReinterpretArray{T, N, S, A, IsReshaped} where {T, N, A<:Union{SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real}}, Tuple{AbstractUnitRange, Vararg{Any}}}}, DenseArray}, IsReshaped, S}, SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real}}, Tuple{AbstractUnitRange, Vararg{Any}}}}, DenseArray}}, SubArray{T, var"#s970", var"#s969", I, true} where {var"#s970", var"#s969"<:Union{Base.ReinterpretArray{T, N, S, A, IsReshaped} where {N, A<:Union{SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real}}, Tuple{AbstractUnitRange, Vararg{Any}}}}, DenseArray}, IsReshaped, S}, Base.ReshapedArray{T, N, A} where {N, A<:Union{Base.ReinterpretArray{T, N, S, A, IsReshaped} where {T, N, A<:Union{SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real}}, Tuple{AbstractUnitRange, Vararg{Any}}}}, DenseArray}, IsReshaped, S}, SubArray{T, N, A, I, true} where {T, N, A<:DenseArray, I<:Union{Tuple{Vararg{Real}}, Tuple{AbstractUnitRange, Vararg{Any}}}}, DenseArray}}, DenseArray{T}}, I}, DenseArray{T}}) where T<:Union{Float32, Float64}
    @ LinearAlgebra ~/Julia/depot/juliaup/julia-1.9.0-beta3+0.x64.linux.gnu/share/julia/stdlib/v1.9/LinearAlgebra/src/matmul.jl:14

Possible fix, define
  dot(::oneArray{T}, ::oneArray{T}) where T<:Union{Float32, Float64}

The oneStridedArray definition here might look a bit nonsensical, but it's reduced from the full thing here: https://github.com/JuliaGPU/oneAPI.jl/blob/a5c809644e0bed2bdf8188ea080c43ef7fa5160c/src/array.jl#L163-L187

Any suggestions on a workaround? The suggested fix obviously doesn't work for anything but oneArray (e.g. reshaped, contiguous views, etc).

@N5N3
Copy link
Member Author

N5N3 commented Jan 26, 2023

IIRC MKL's dot only supports vec-like input (1 inc for X and 1 inc for Y) on GPU, thus dot(x::oneStridedArray{T}, y::oneStridedArray{T}) seems wrong to me.

If this is a oneAPI.jl-only problem, then I suggest to restrict the definition to dot(x::oneStridedVecLike{T}, y::oneStridedVecLike{T}) (see LinearAlgebra.StridedVecLike)

@maleadt
Copy link
Member

maleadt commented Jan 26, 2023

Fair enough, and that does work around the ambiguity. Thanks!

@maleadt
Copy link
Member

maleadt commented Jan 26, 2023

What's the difference between the StridedVecLike introduced here, and StridedVector?

@N5N3
Copy link
Member Author

N5N3 commented Jan 26, 2023

StridedVecLike should include more supported layout. e.g.

julia> view(zeros(200,200),:,:) isa Base.StridedVector
false

julia> view(zeros(200,200),:,:) isa LinearAlgebra.StridedVecLike
true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants