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

Specialize LinearAlgebra.BLAS.dot for strided vectors of floats. #39751

Merged
merged 1 commit into from
Feb 28, 2021

Conversation

ghost
Copy link

@ghost ghost commented Feb 19, 2021

Fixes #37767.

(tested using openblas64)

@ghost ghost force-pushed the db/37767 branch 2 times, most recently from d738142 to 86d3762 Compare February 19, 2021 20:00
@ViralBShah ViralBShah added the domain:linear algebra Linear algebra label Feb 21, 2021
@ViralBShah
Copy link
Member

Welcome @dagbr! Just holding this one while we try to get #39455 merged. I think this shouldn't lead to a conflict, but there's a lot of linear algebra stuff changing simultaneously, and just being cautious for now.

@dkarrasch
Copy link
Member

Out of curiosity, what exactly was the issue and how did you resolve it?

@ghost
Copy link
Author

ghost commented Feb 21, 2021

I have amended my commit to remove a misleading comment.
@dkarrasch
It seems StridedVector{Float64}'s are treated without taking into account views with negative step size.
A StridedVector can be a SubArray arising from a view like v = view([1.0,2.0,3.0], 3:-1:2) with negative stride.
When calling OpenBLAS, we should pass a pointer to first element in memory, which is at pointer(v,length(v)) if the stride is negative.
My guess is that, in the case of a SubArray, the conversion to Ptr{Float64} yielded some other pointer, I think it could be Base.unsafe_convert(Ptr{Float64},v) which is equal to pointer(v,1) (at least when I played with it in the REPL).
I think this can happen because the wrappers in blas.jl for the dot functions from BLAS take any AbstractArray.

@ghost ghost mentioned this pull request Feb 21, 2021
@ViralBShah
Copy link
Member

#39455 is merged. So that is no longer blocking this PR.

@ghost ghost mentioned this pull request Feb 26, 2021
@dkarrasch dkarrasch added the status:merge me PR is reviewed. Merge when all tests are passing label Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segmentation fault in dot function
3 participants