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

Construct MulAddMul at gemm_wrapper! call sites #34601

Merged
merged 3 commits into from
Feb 1, 2020

Conversation

tkf
Copy link
Member

@tkf tkf commented Jan 31, 2020

This is yet another PR to fix #34013. The idea is to move alpha and beta to the type domain as early as possible. Maybe this is better than #34384 as it relies less on inlining and constant propagation?

@KristofferC
Copy link
Sponsor Member

Let's go with this one?

@tkf
Copy link
Member Author

tkf commented Feb 1, 2020

Yeah, I think this is better. The latest commits in #34384 started breaking the tests.

@KristofferC
Copy link
Sponsor Member

Win32 CI hang happens on the other PRs as well.

@KristofferC KristofferC merged commit 2da42e0 into JuliaLang:master Feb 1, 2020
@StefanKarpinski
Copy link
Sponsor Member

The part of #34384 that moves the calls to matmul2x2! and matmul3x3! before all the heavywight checking that they don't require seems like an independently good change, however, since it should reduce overhead in those cases. Separating higher overhead call to BLAS into its own subroutine also seems like a good thing. Not sure why this was suddenly decided to be much better.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Feb 1, 2020

Because it didn't require inlining or constant propagation, it seemed less brittle to compiler changes.

@StefanKarpinski
Copy link
Sponsor Member

Sure, that's good. The other parts of that change are good anyway though, without the parts that force inlining and constant propagation—i.e. that version of gemm_wrapper! should be easier for the compiler to deal with.

KristofferC pushed a commit that referenced this pull request Feb 5, 2020
* Construct MulAddMul at gemm_wrapper! call sites

* Add branches manually in MulAddMul constructor

This is suggested by chethega in:
#29634 (comment)

* Update stdlib/LinearAlgebra/src/generic.jl

Co-Authored-By: Kristoffer Carlsson <[email protected]>

Co-authored-by: Kristoffer Carlsson <[email protected]>
(cherry picked from commit 2da42e0)
@KristofferC KristofferC mentioned this pull request Feb 5, 2020
26 tasks
KristofferC added a commit that referenced this pull request Apr 11, 2020
* Construct MulAddMul at gemm_wrapper! call sites

* Add branches manually in MulAddMul constructor

This is suggested by chethega in:
#29634 (comment)

* Update stdlib/LinearAlgebra/src/generic.jl

Co-Authored-By: Kristoffer Carlsson <[email protected]>

Co-authored-by: Kristoffer Carlsson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mul! performance regression on master
3 participants