Skip to content
This repository has been archived by the owner on Mar 12, 2021. It is now read-only.

Provide 5-arg mul! #634

Merged
merged 3 commits into from Mar 16, 2020
Merged

Provide 5-arg mul! #634

merged 3 commits into from Mar 16, 2020

Conversation

haampie
Copy link
Member

@haampie haampie commented Mar 15, 2020

Since Julia 1.3 we have a 5-arg mul! for BLAS, but somehow it has not made its way into CuArrays.

In principle this should not be a breaking change, however, I've commented out LinearAlgebra.lmul! because it is not provided by Julia base as far as I know. So that is breaking, but I don't think anybody uses it (?).

Finally it fixes an oversight in LinearAlgebra.mul!(C::CuMatrix{T}, adjA::Adjoint{<:Any, <:CuMatrix{T}}, adjB::Adjoint{<:Any, CuMatrix{T}}) where no <: was used 馃槄

@haampie haampie force-pushed the feature-5-arg-mul branch 2 times, most recently from 9f9fdbf to bb0cd88 Compare March 15, 2020 13:22
@haampie
Copy link
Member Author

haampie commented Mar 15, 2020

Hm, there's ambigious methods when taking a and b as ::Number. Simple fix is to replace that type by the eltype T, but that's not such a nice api.

Edit, decided to go for ::T instead of ::Number. If we really want to be able to do mul!(y, A, x, 1, 1)` or something, then a whole lot of new methods have to be added to avoid ambiguity.

@haampie haampie force-pushed the feature-5-arg-mul branch 2 times, most recently from 947b63c to 50bfe56 Compare March 15, 2020 14:37
@maleadt
Copy link
Member

maleadt commented Mar 15, 2020

Thanks! Next release is going to be breaking so go ahead and remove those methods.
bors try

@haampie
Copy link
Member Author

haampie commented Mar 15, 2020

Should there be a deprecation warning for lmul!?

@maleadt
Copy link
Member

maleadt commented Mar 15, 2020

Should there be a deprecation warning for lmul!?

Seems weird to provide a deprecation for a method in another module? We're just implementing the interface here; if upstream has deprecated/removed something, shouldn't we just follow suit?

@haampie haampie force-pushed the feature-5-arg-mul branch 3 times, most recently from 246b9f4 to 52fbf81 Compare March 15, 2020 18:45
@haampie
Copy link
Member Author

haampie commented Mar 15, 2020

Ok, so I've worked my way through tons of ambiguity warnings and added tests for all combinations of eltypes, transposes and adjoints for mul!. Ready for review!

@maleadt
Copy link
Member

maleadt commented Mar 15, 2020

bors try

bors bot added a commit that referenced this pull request Mar 15, 2020
@bors
Copy link
Contributor

bors bot commented Mar 15, 2020

try

Build succeeded

@maleadt
Copy link
Member

maleadt commented Mar 16, 2020

LGTM, coverage is perfect too. Would it make sense to guard the definitions for 1.3.0 in a if VERSION block, to make sure we can safely remove them once we bump the Julia requirement?

bord d+

@haampie
Copy link
Member Author

haampie commented Mar 16, 2020

Ok, done :)

@maleadt
Copy link
Member

maleadt commented Mar 16, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 16, 2020

Build succeeded

@bors bors bot merged commit 5542a9e into JuliaGPU:master Mar 16, 2020
@haampie haampie deleted the feature-5-arg-mul branch March 16, 2020 13:05
maleadt added a commit that referenced this pull request Mar 18, 2020
This reverts commit 5542a9e, reversing
changes made to 52c4664.
haampie added a commit to haampie/CuArrays.jl that referenced this pull request Mar 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants