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

SPQR: Make GPUQREngine and SuiteSparse_GPURuntime subprojects #385

Merged
merged 4 commits into from
Oct 19, 2023

Conversation

mmuetzel
Copy link
Contributor

@mmuetzel mmuetzel commented Sep 8, 2023

This is an early test to check if it makes sense to make GPUQREngine a subproject of SPQR.

Feedback welcome.

@mmuetzel
Copy link
Contributor Author

This change could serve as a test-bed for how a root CMakeLists.txt file could use the existing CMakeLists.txt files of the currently existing projects.

With the changes here, it should still be possible to build GPUQREngine as a stand-alone project (by configuring with the CMakeLists.txt file in the subfolder).
Is that something that would be useful (and required) when adding a root CMakeLists.txt file?

If we'd require that users need to use that potentially new root CMakeLists.txt file (i.e., stop supporting building a single library by configuring with the CMakeLists.txt file in the respective subfolder), these CMakeLists.txt files could probably be simplified by dropping some of the existing logic.

I haven't thought it through completely, but we'd probably need some additional logic if that use case should be supported.

@mmuetzel
Copy link
Contributor Author

mmuetzel commented Sep 19, 2023

Rebased on a current head to resolve some merge conflicts.

Following #408 (comment), we could also move SuiteSparse_GPURuntime to the SPQR folder. That could be a lower-impact first step on the way to absorbing those two libraries into SPQR_CUDA as proposed there.
That would also avoid having to duplicate the internal headers because we could treat those four libraries as interdependent.

@mmuetzel mmuetzel changed the title SPQR: Make GPUQREngine a subproject. SPQR: Make GPUQREngine and SuiteSparse_GPURuntime subprojects Sep 19, 2023
@DrTimothyAldenDavis
Copy link
Owner

Yes, it would be a good idea to move GPUQREngine and SuiteSparse_GPURuntime into the SPQR folder as subprojects, just like SPQR_CUDA.

The original intent was for SuiteSparse_GPURuntime to be a support library for any other SuiteSparse library that needed CUDA utilities, but it has turned out that I've needed it only for SPQR. In any case, if that changes in the future, I could go back to a stand alone library again.

@mmuetzel
Copy link
Contributor Author

I went ahead and moved SuiteSparse_GPURuntime to a subfolder of SPQR.
I did no longer take any attempt to have the two sub-projects be buildable as standalone projects. The four libraries are interdependent by design anyway.

I agree that it would still be possible to move SuiteSparse_GPURuntime back to a "proper" standalone library. But that should be done with some care to avoid including headers in other libraries that are private to this library.

I believe that this PR should be ready to be merged now. However, I expect some merge conflicts with other open PRs (e.g. #411). It might be better to merge those PRs before this PR.
I'll be happy to rebase this PR on top of the other ones if they are merged.

@mmuetzel
Copy link
Contributor Author

Forgot to mention: This also fixes the part of #409 regarding those two libraries.

@mmuetzel
Copy link
Contributor Author

@DrTimothyAldenDavis: Could this PR be merged next? That would unblock the remaining changes for #409.

@DrTimothyAldenDavis
Copy link
Owner

Got it. So I should merge the following PR's, in this order, right?

#385, #419, #443, #444, and then #445 ?

@mmuetzel
Copy link
Contributor Author

Got it. So I should merge the following PR's, in this order, right?

#385, #419, #443, #444, and then #445 ?

That looks good to me.
Thanks!

@mmuetzel
Copy link
Contributor Author

Or maybe better: #385, #443, #444 and then #445.

#419 probably needs to be rebased after this change is merged.

@DrTimothyAldenDavis DrTimothyAldenDavis merged commit da6dea4 into DrTimothyAldenDavis:dev2 Oct 19, 2023
34 checks passed
@DrTimothyAldenDavis
Copy link
Owner

Done! I also released a minor fix: SuiteSparse v7.2.2, where I found a glitch when computing the # of OpenMP to use when assembling the last supernode in a superndal factorization. Likely no effect on performance, and no effect on accuracy of the result. I just might have gotten the wrong number of threads to use (but I always limit the # of threads to a sane number: 1 to the total # of threads available).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants