DrTimothyAldenDavis / SuiteSparse

The official SuiteSparse library: a suite of sparse matrix algorithms authored or co-authored by Tim Davis, Texas A&M University.
https://people.engr.tamu.edu/davis/suitesparse.html
Other
1.15k stars 259 forks source link

UMFPACK: Use (compiler-specific) macro for ivdep pragma #688

Closed mmuetzel closed 8 months ago

mmuetzel commented 8 months ago

Different compilers use different pragmas to hint to the compiler that vector dependencies can be ignored for the subsequent loop.

Add configuration checks to detect the respective pragma and use it.

DrTimothyAldenDavis commented 8 months ago

I've had trouble with adding #pragmas to GraphBLAS. I added this to GraphBLAS a while back (but long after the #pragma ivdep in UMFPACK):

https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/3d440bf029aac32558b78a9f1695663e77badb97/GraphBLAS/Source/Template/GB_compiler.h#L134-L160

Does MS Visual Studio work with _Pragma or does it still need __pragma?

mmuetzel commented 8 months ago

Does MS Visual Studio work with _Pragma or does it still need __pragma?

According to their documentation, both are ok: https://learn.microsoft.com/en-us/cpp/preprocessor/pragma-directives-and-the-pragma-keyword?view=msvc-170&redirectedfrom=MSDN

DrTimothyAldenDavis commented 8 months ago

OK. Maybe I wasn't using the right flags on MSVC when I encountered that problem. I see this discussion on that page:

The _Pragma preprocessing operator _Pragma is similar to the Microsoft-specific __pragma keyword. It was introduced into the C standard in C99, and the C++ standard in C++11. It's available in C only when you specify the /std:c11 or /std:c17 option.

GraphBLAS requires C11, so I guess MSVC must be compiling with /std:c11now.

mmuetzel commented 8 months ago

GraphBLAS requires C11, so I guess MSVC must be compiling with /std:c11now.

That's probably the case: https://learn.microsoft.com/en-us/cpp/build/reference/std-specify-language-standard-version?view=msvc-170

The default C compiler (that is, the compiler when /std:c11 or /std:c17 isn't specified) implements ANSI C89

But - like you wrote - GraphBLAS requires C11: https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/2e28d427875ccc5dbe0241dcce63bc16692db2ec/GraphBLAS/CMakeLists.txt#L286-L293

DrTimothyAldenDavis commented 8 months ago

Yes ... that code in the GraphBLAS CMakeLists.txt is fixed now, but I used to have it wrong, with something like

C_STANDARD_REQUIRED 11 or something. So that's likely why I needed to use __pragma instead of _Pragma. No need to revise GraphBLAS for this though.