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

UMFPACK: Display timer function used by SuiteSparse_config in report #823

Merged
merged 3 commits into from
Jun 6, 2024

Conversation

mmuetzel
Copy link
Contributor

@mmuetzel mmuetzel commented Jun 3, 2024

UMFPACK is built without OpenMP. (It doesn't use it directly.) Hence, _OPENMP is not defined when the UMFPACK libraries are built.

Get the information about the used timer function directly from SuiteSparse_config.h instead.

Fixes #822.

I used the following reference for the "stringification" of the preprocessor macro:
https://gcc.gnu.org/onlinedocs/cpp/Stringizing.html

@mmuetzel mmuetzel marked this pull request as draft June 3, 2024 20:49
@mmuetzel
Copy link
Contributor Author

mmuetzel commented Jun 3, 2024

Oops. What I wrote is probably not entirely correct.

I was assuming that all libraries would be using SuiteSparse_tic, SuiteSparse_toc or SuiteSparse_time to measure time. However, it looks like some libraries (or demos) are using SUITESPARSE_TIME directly. For that to work, they need to build with OpenMP (if clock_gettime is not available).

Additionally, the proposed change doesn't do what I thought it would because SUITESPARSE_TIME is set to a value that depends on the library in which SuiteSparse_config.h is included (and not on how libsuitesparse_config itself was built).

Should SUITESPARSE_TIME really be used in libraries that depend on libsuitesparse_config? Should these libraries (and demos) better use SuiteSparse_time instead?

@mmuetzel
Copy link
Contributor Author

mmuetzel commented Jun 3, 2024

I added a second commit that should be fixing the issues described in the previous comment.

@mmuetzel mmuetzel marked this pull request as ready for review June 3, 2024 21:21
@mmuetzel
Copy link
Contributor Author

mmuetzel commented Jun 4, 2024

It looks like the changes from this PR triggered a test error on the runner using clang-cl.
It is likely that there is an issue with the OpenMP configuration on that runner indeed. It is linking libraries that are using the Intel OpenMP implementation (the Intel MKL BLAS/LAPACK libraries) and libraries that are using the LLVM OpenMP implementation (the SuiteSparse libraries).
That might also be an issue on the runners that are using MSVC cl. But it looks like they do not have a similar runtime check in the MSVC implementation of OpenMP.

I'll try to look into that soon. (But that will likely result in a different PR.)

@mmuetzel
Copy link
Contributor Author

mmuetzel commented Jun 5, 2024

Converted to draft for now to wait for #824 and rebase.

mmuetzel added 2 commits June 5, 2024 17:15
UMFPACK is built without OpenMP. (It doesn't use it directly.)
Hence, `_OPENMP` is not defined when the UMFPACK libraries are built.

Get the information about the used timer function directly from
SuiteSparse_config.h instead.

Fixes DrTimothyAldenDavis#822.
Record in "SuiteSparse_config.h" if working timer functions are available
in libsuitesparse_config.

Always indirect to `SuiteSparse_time` when dependent libraries use
`SUITESPARSE_TIME`.

Add new preprocessor macro `SUITESPARSE_CONFIG_TIMER` which records the
function that is used by libsuitesparse_config for its timer functions.
Use that new preprocessor macro in the reports of UMFPACK.
@mmuetzel mmuetzel marked this pull request as ready for review June 5, 2024 16:05
@mmuetzel
Copy link
Contributor Author

mmuetzel commented Jun 5, 2024

This seems to be working now.

It looks like GitHub is in the process of updating MSVC from 19.39 to 19.40 on their runners. And the CUDA version that we are installing isn't compatible with that newer version of MSVC.
But that is unrelated to the changes proposed here.

Avoid indirection through the library if the compilation unit including
`SuiteSparse_config.h` happens to use OpenMP.
@DrTimothyAldenDavis DrTimothyAldenDavis merged commit ae25b9b into DrTimothyAldenDavis:dev2 Jun 6, 2024
24 of 26 checks passed
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