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

AMD: Avoid stubs in favor of object libraries. #875

Closed
wants to merge 1 commit into from

Conversation

mmuetzel
Copy link
Contributor

Avoid the stub files that were used to build objects for the int64_t interface. Instead, use object libraries that build the same source files once with DLONG and once without it.

This also avoids building all objects twice (for the shared and the static library), effectively reducing the build time approximately by a factor of two.

Something similar could probably be done for most of the SuiteSparse libraries. So, build time could probably be reduced by a lot (when building shared and static libraries).
@DrTimothyAldenDavis: Please, let me know if this is acceptable and I can try to look into that.

Avoid the stub files that were used to build objects for the int64_t
interface. Instead, use object libraries that build the same source files
once with `DLONG` and once without it.

This also avoids building all objects twice (for the shared and the
static library), effectively reducing the build time approximately by a
factor of two.
@DrTimothyAldenDavis
Copy link
Owner

I used this strategy long ago, before I had a cmake build system. Just Makefiles. However, I found it confusing. Where is the function amd_l_defaults ( ... ) defined? It was hard to find. Now it's simple: just look at Source/amd_l_defaults.c. Yes, that's just a simple 'C template' wrapped around amd_defaults.c, but you can find amd_l_defaults by just looking for the file of the same name, just add the ".c".

I had a hard time navigating my own code in this old system, and so did others. So when I went to a cmake build system, I changed my codes to use this new mechanism.

Yes, it might cut the build time in half for packages like AMD, COLAMD, UMFPACK, and CHOLMOD, but it makes the code easier (for me) to navigate, and that's more important.

I don't use this strategy for my largest code, GraphBLAS, which takes the longest to compile, because that package doesn't have so many variants with different names (there is a single GrB_mxm for all matrices and all data types and all semirings, regardless of what they are).

So it only impacts codes that compile pretty quickly anyway.

So, while it does cut the build time, and I do appreciate the input, but I'd rather stick with the current code structure.

@mmuetzel
Copy link
Contributor Author

mmuetzel commented Oct 18, 2024

Thank you for the feedback and explaining your thought process.

My original motivation for this PR was to remove the stub files. But I understand your motivation for preferring to keep them.
The reduction of the number of objects that need to be built for the shared and static libraries was "just" a positive side effect. But thinking of it, it should be possible to have that "side effect" while keeping the stubs.

I'll close this PR and potentially open a new one with the "side effect" only (if that works)...

@mmuetzel mmuetzel closed this Oct 18, 2024
@mmuetzel
Copy link
Contributor Author

That was simpler than I expected. See #876.

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