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

Type-stability fixes for matmul #14335

Merged
merged 1 commit into from
Dec 9, 2015
Merged

Type-stability fixes for matmul #14335

merged 1 commit into from
Dec 9, 2015

Conversation

timholy
Copy link
Member

@timholy timholy commented Dec 9, 2015

This one is quite funny: we have

julia> v = [1,2]
2-element Array{Int64,1}:                                                                                                                                                          
 1                                                                                                                                                                                 
 2

julia> using Base.Test

julia> @inferred(v*v')
2x2 Array{Int64,2}:
 1  2
 2  4

but also

julia> C = Array(Int, 2, 2)
2x2 Array{Int64,2}:
 140643182283632  140634494951504
 140634541475488                0

julia> @inferred(A_mul_Bc!(C, v, v))
ERROR: return type Array{Int64,2} does not match inferred return type Any
 in error at ./error.jl:21

The reason turned out to be a bit subtle:

julia> Base.LinAlg.matmul2x2!(C, 'N', 'C', v, v)
ERROR: MethodError: `matmul2x2!` has no method matching matmul2x2!(::Array{Int64,2}, ::Char, ::Char, ::Array{Int64,1}, ::Array{Int64,1})
Closest candidates are:
  matmul2x2!{T,S,R}(::AbstractArray{R,2}, ::Any, ::Any, ::AbstractArray{T,2}, ::AbstractArray{S,2})

It seems that when no method exists, inference is returning Any rather than Union{} for the return type?

So the underlying cause is the branch for 2x2 or 3x3 matrices, the fact that the corresponding functions are defined only for AbstractMatrix inputs, and the fact that generic_matmatmul! can take inputs which are not AbstractMatrix objects.

Also fixes another type instability in copy! (it was returning Union{Bool,typeof(B)} which probably has no performance impact, but we might as well fix it.

@timholy timholy added bug Indicates an unexpected problem or unintended behavior backport pending 0.4 labels Dec 9, 2015
@timholy
Copy link
Member Author

timholy commented Dec 9, 2015

Might want to wait a bit on merging this to see whether there's an easy, backportable fix for #14336.

@timholy
Copy link
Member Author

timholy commented Dec 9, 2015

Looks like #14336 might have to wait on #265, and we all know what that means. I'll merge this, and it also should be backported.

timholy added a commit that referenced this pull request Dec 9, 2015
@timholy timholy merged commit 106ad4d into master Dec 9, 2015
@timholy timholy deleted the teh/matmul_typestablity branch December 9, 2015 16:28
timholy added a commit that referenced this pull request Jan 9, 2016
(cherry picked from commit 6db0a4f)
ref #14335
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants