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.

Sign up for GitHub

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

Jump to bottom

Dispatch more cases to BLAS.gemm! #33229

Merged
merged 2 commits into from
Sep 13, 2019
Merged

Dispatch more cases to BLAS.gemm! #33229

merged 2 commits into from
Sep 13, 2019

Conversation

tkf
Copy link
Member

@tkf tkf commented Sep 11, 2019

This PR tweaks dispatch of mul! so that more combinations of types can use BLAS.gemm!. Namely, alpha and beta does not have to be BlasFloat anymore.

After:

julia> n = 100
       A = rand(n, n)
       B = rand(n, n)
       C = zeros(n, n);

julia> @btime mul!($C, $B, $A, -1, 0);
  31.085 μs (0 allocations: 0 bytes)

julia> @btime mul!($C, $B, $A, -1.0, 0.0);
  31.228 μs (0 allocations: 0 bytes)

Before:

julia> @btime mul!($C, $B, $A, -1, 0);
  704.054 μs (69 allocations: 3.56 KiB)

julia> @btime mul!($C, $B, $A, -1.0, 0.0);
  31.068 μs (0 allocations: 0 bytes)

@fredrikekre pointed it out in JuliaLang/LinearAlgebra.jl#663

@fredrikekre fredrikekre added linear algebra Linear algebra performance Must go faster potential benchmark Could make a good benchmark in BaseBenchmarks labels Sep 11, 2019
andreasnoack
andreasnoack reviewed Sep 12, 2019
View reviewed changes
stdlib/LinearAlgebra/src/matmul.jl
@inline function mul!(C::StridedMatrix{T}, A::StridedVecOrMat{T}, B::StridedVecOrMat{T},
alpha′::Number, beta′::Number) where {T<:BlasFloat}
alpha, beta = promote(alpha′, beta′, zero(T))
if alpha isa T && beta isa T
Copy link
Member

Choose a reason for hiding this comment

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

Why not use a separate method for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you suggesting to write

@inline function mul!(C::StridedMatrix{T}, A::StridedVecOrMat{T}, B::StridedVecOrMat{T},
                      alpha::$TA, beta::$TB) where {T<:BlasFloat}
    ...

with some expressions $TA and $TB? Is it possible? We need to express "alpha and beta would be promoted to T" in the type domain. I don't know how to do it.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't see the accents. So no, it's not possible.

@tkf
Copy link
Member Author

tkf commented Sep 12, 2019

@andreasnoack Maybe alpha′ and beta′ were not great variable names. I changed to them to α and β so that difference is much clearer, I think.

I started to think using \prime is OK only for one- or two-letter variable names where the difference is visually significant due to the change in width.

@tkf
Copy link
Member Author

tkf commented Sep 13, 2019

Can it be backported to 1.3? I think this is a bit dangerous performance pitfall and the patch does not seem to be very destructive.

@andreasnoack andreasnoack merged commit 51b3227 into JuliaLang:master Sep 13, 2019
@andreasnoack andreasnoack added the triage This should be discussed on a triage call label Sep 13, 2019
@andreasnoack
Copy link
Member

I've added the triage label. They'll decide if it's okay to try backporting.

@dkarrasch
Copy link
Member

I'd say this is clearly a bugfix. It's very hard to imagine that anybody would have relied on the specific behavior (same result with less speed) in code written in the past month or so...

@KristofferC KristofferC added backport 1.3 and removed triage This should be discussed on a triage call labels Sep 13, 2019
@tkf tkf deleted the perf5mul branch September 17, 2019 04:07
KristofferC pushed a commit that referenced this pull request Sep 18, 2019
@tkf @KristofferC
Dispatch more cases to BLAS.gemm! (#33229)
4e5f01c 
* Dispatch more cases to BLAS.gemm!

* Use α and β instead of alpha′ and beta′

(cherry picked from commit 51b3227)
@KristofferC KristofferC mentioned this pull request Sep 18, 2019
Merged
18 tasks
KristofferC pushed a commit that referenced this pull request Sep 19, 2019
@tkf @KristofferC
Dispatch more cases to BLAS.gemm! (#33229)
c780b9c 
* Dispatch more cases to BLAS.gemm!

* Use α and β instead of alpha′ and beta′

(cherry picked from commit 51b3227)
@tkf tkf mentioned this pull request Oct 30, 2019
Merged
11 tasks
@dkarrasch dkarrasch mentioned this pull request Nov 1, 2019
Merged
@dkarrasch dkarrasch mentioned this pull request Nov 14, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreasnoack andreasnoack andreasnoack left review comments

Assignees
No one assigned
Labels
linear algebra Linear algebra performance Must go faster potential benchmark Could make a good benchmark in BaseBenchmarks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tkf @andreasnoack @dkarrasch @KristofferC @fredrikekre