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

Aggressive constprop in matvecmul and matmatmul #51961

Merged
merged 6 commits into from
Dec 6, 2023

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Oct 31, 2023

This usually eliminates branches, with occasional improvement in type-stability. I've replaced @inline by aggressive constprop in generic_matmatmul, as I felt that this was the objective.

@jishnub jishnub added the domain:linear algebra Linear algebra label Oct 31, 2023
@dkarrasch
Copy link
Member

as I felt that this was the objective.

Indeed, it was. Is there any indication that this helps with compilation times or something?

@jishnub
Copy link
Contributor Author

jishnub commented Nov 1, 2023

The impact seems fairly minimal in the simple tests that I've carried out, although this might make JET happier.

@jishnub jishnub added the backport 1.10 Change should be backported to the 1.10 release label Nov 3, 2023
@dkarrasch
Copy link
Member

Unfortunately, this seems to not help elimination of branches. At least for me, this makes things as in #51812 (comment) worse regarding compile time. If anything, then I would have hoped for a decrease in compile time and elimination of branches, since the runtime of the character checks is negligible.

@jishnub
Copy link
Contributor Author

jishnub commented Nov 5, 2023

Perhaps we may apply it selectively, as it seems to improve type inference in certain cases (e.g. complex-real matvecmul)?

@KristofferC KristofferC mentioned this pull request Nov 6, 2023
39 tasks
@jishnub jishnub removed the backport 1.10 Change should be backported to the 1.10 release label Nov 6, 2023
@gdalle
Copy link
Contributor

gdalle commented Nov 10, 2023

@dkarrasch
Copy link
Member

See also:

* [Regression for `mul!` from 1.9 to 1.10 JuliaSparse/SparseArrays.jl#469](https://github.com/JuliaSparse/SparseArrays.jl/issues/469)

This is unrelated to constant propagation.

I'm no longer sure we need these annotations, actually. It seems to suffice to inline some intermediate functions. For instance

julia> using LinearAlgebra

julia> A = zeros(10, 10);

julia> @code_typed mul!(copy(A), A', A)
CodeInfo(
1%1 = Base.getfield(A, :parent)::Matrix{Float64}%2 = (%1 === B)::Bool
└──      goto #3 if not %2
2%4 = invoke LinearAlgebra.syrk_wrapper!(C::Matrix{Float64}, 'T'::Char, %1::Matrix{Float64}, $(QuoteNode(LinearAlgebra.MulAddMul{true, true, Bool, Bool}(true, false)))::LinearAlgebra.MulAddMul{true, true, Bool, Bool})::Matrix{Float64}
└──      goto #4
3%6 = invoke LinearAlgebra.gemm_wrapper!(C::Matrix{Float64}, 'T'::Char, 'N'::Char, %1::Matrix{Float64}, B::Matrix{Float64}, $(QuoteNode(LinearAlgebra.MulAddMul{true, true, Bool, Bool}(true, false)))::LinearAlgebra.MulAddMul{true, true, Bool, Bool})::Matrix{Float64}
└──      goto #4
4%8 = φ (#2 => %4, #3 => %6)::Matrix{Float64}
└──      goto #5
5return %8
) => Matrix{Float64}

That means, the compiler understands that only two branches might be used, but e.g. herk_wrapper! is not. This is already achieved with the @inline annotation of the BLAS-dispatch method of generic_matmatmul!, and it does not improve with aggressive constant propagation! So, before we put more load on the compiler, we should make sure that each and every new annotation has a tangible benefit.

@gdalle
Copy link
Contributor

gdalle commented Nov 15, 2023

This is unrelated to constant propagation.

I thought the matmul regression had two sides, a performance side and a type-instability side which may be resolved by constprop? At least that's what @jishnub suggested on Discourse

@dkarrasch
Copy link
Member

The regression is #52137. There may be cases where we need to rewrap an unwrapped matrix which unfortunately allocates. But for these cases aggressive constant propagation doesn't seem to help.

@jishnub jishnub marked this pull request as draft November 20, 2023 13:30
@dkarrasch
Copy link
Member

Sorry, I clicked the wrong button in my VSCode instance.

@jishnub jishnub marked this pull request as ready for review December 6, 2023 07:17
@dkarrasch dkarrasch added status:merge me PR is reviewed. Merge when all tests are passing and removed status:merge me PR is reviewed. Merge when all tests are passing labels Dec 6, 2023
@dkarrasch dkarrasch merged commit cfcc043 into master Dec 6, 2023
7 of 9 checks passed
@dkarrasch dkarrasch deleted the jishnub/constpropmatmatmul branch December 6, 2023 14:08
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.

None yet

3 participants