-
Notifications
You must be signed in to change notification settings - Fork 88
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
Split batched solver compilation #1629
base: develop
Are you sure you want to change the base?
Conversation
259f2c1
to
8c25a83
Compare
This should have a huge impact, excerpt from the HIP 5.14 debug build log
|
core/solver/batch_dispatch.hpp
Outdated
#define GKO_BATCH_INSTANTIATE_STOP(macro, ...) \ | ||
macro(__VA_ARGS__, \ | ||
::gko::batch::solver::device::batch_stop::SimpleAbsResidual); \ | ||
template macro( \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the template
here (and in the other macros below) could be removed, if the value/index type instantiation macros would accept variable number or arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't work until C++20. A macro with (arg, ...)
requires two arguments before c++20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, the idea looks good, but the pipelines are failing.
One thing against this approach is the readability and maintainability is seriously affected. The already complex batched code is even more complex and annoying to read now. We should maybe see if instead we dont do this split approach and instead maybe do what Jacobi does and have fewer cases as default, and only have full instantiations when necessary.
8c25a83
to
870ad69
Compare
IMO the Jacobi instantiation is more complex than what is here. The kernel and the instantiations are directly together, instead of being generated by CMake, which makes it easier to follow for me. But I agree that the batch system needs an overhaul in general. |
d04f06c
to
fa6d091
Compare
fa6d091
to
e59ab55
Compare
An alternative approach: https://github.com/ginkgo-project/ginkgo/tree/batch-optim |
This seems to be quite orthogonal to this PR. With full optimizations enabled, there would be the same issue as before, so the fix from this PR is still needed. I don't see a reason why we should burden people that want the full optimizations enabled with those long compile times, for which we already have a fix available. |
@MarcelKoch, can you please rebase this when you have some time and we can try to get it merged ? |
48fe94b
to
045ad1c
Compare
@pratikvn this should now be properly rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some header guards are missing, but otherwise looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor typos
b4c2490
to
85b80df
Compare
8634bd6
to
6cdfded
Compare
- adds header guard Co-authored-by: Pratik Nayak <[email protected]>
Co-authored-by: Tobias Ribizel <[email protected]>
6cdfded
to
8258bc9
Compare
This PR splits up the compilation of the batched solvers in order to reduce the compilation times. It splits up the instantiations of the kernel launches depending on the number of vectors in shared memory. This is based on the same CMake mechanism as for the csr and fbcsr kernels.