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

Include HermOrSym in character-based mul! dispatch #49865

Merged
merged 7 commits into from
May 24, 2023
Merged

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented May 17, 2023

This extends #49806 to HermOrSym types. With this PR, we are down to 33 mul! methods. Can't wait to have -5 methods! 🤣

This is in some parts a bit ugly due to the re-wrapping, but that happens only in the very generic paths, so shouldn't affect the fast paths.

@dkarrasch dkarrasch added the domain:linear algebra Linear algebra label May 17, 2023
@dkarrasch
Copy link
Member Author

@nanosoldier runtests()

@nanosoldier runbenchmarks("linalg", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@dkarrasch
Copy link
Member Author

@nanosoldier runbenchmarks("linalg", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here.

@vtjnash
Copy link
Sponsor Member

vtjnash commented May 19, 2023

Just wanted to say that this work to get rid of all those ugly buggy slow Union dispatch has been awesome!

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@dkarrasch
Copy link
Member Author

@nanosoldier runtests(["MaskedArrays", "UCX", "POMDPTools", "DifferentialForms", "ExponentialUtilities", "Test", "MendelImpute", "TrajectoryOptimization", "HarmonicBalance", "DataDrivenLux"])

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@dkarrasch
Copy link
Member Author

Okay, so here is the last remaining issue. The character-based dispatch approach now includes symmetric and hermitian matrices. In case the combination of wrappers or the type of the scalars alpha and beta don't satisfy specific rules and we don't end up using some BLAS function, we come to use the most generic multiplication algorithm. That one, however, is currently only written for Normal, Transpose and adjoint (C) matrices. So we need to rewrap the symmetric matrices and treat them as plain. This adds a small memory allocation, which pops up in pkgeval in TrajectoryOptimization.jl. Are there options like aggressive constant propagation or some gentle redesign that anybody sees to avoid the extra allocations? One option is to write out the multiplication code for each of the character combinations, but that means quite a heavy increase in code. By the way, this does not happen in the small 2x2 and 3x3 cases, which are still allocation-free (due to writing out the combinations). Any comment/proposal would be highly appreciated to make this thing happen.

@KristofferC
Copy link
Sponsor Member

Are there options like aggressive constant propagation or some gentle redesign that anybody sees to avoid the extra allocations?

I think some extra allocation is fine here considering the extremely bad behavior we are seeing w.r.t mul! in e.g. package load times. If it becomes an issue in practice more work can probably be put in to try remove that allocation.

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.

4 participants