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.19k stars 266 forks source link

Fix UMFPACK compilation errors in Visual Studio 2017 #855

Closed whuaegeanse closed 4 months ago

whuaegeanse commented 4 months ago

When building suitesparse with Visual Studio 2017, I encountered some compilation errors related to UMFPACK. After investigation, I found that it was caused by Visual Studio 2017 not supporting c11 (_Pragma). This PR fixed this problem.

mmuetzel commented 4 months ago

Thank you for tracking this down.

If I understand Microsoft's documentation correctly, _Pragma is already supported for Visual Studio 2014: https://learn.microsoft.com/en-us/cpp/preprocessor/pragma-directives-and-the-pragma-keyword?view=msvc-140

Could you please show the exact error message that you are seeing before you made that change? Maybe, we are missing to select the C11 or C++11 standard for some targets?

whuaegeanse commented 4 months ago
  1. UMFPACK is a C project.
  2. _Pragma is available in C only when you specify the [/std:c11 or /std:c17]

_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. For C++, it's available in all /std modes, including the default.

3.The /std:c11 option is available starting in Visual Studio 2019 version 16.8.

/std:c11 The /std:c11 option enables ISO C11 conformance. It's available starting in Visual Studio 2019 version 16.8. /std:c17 The /std:c17 option enables ISO C17 conformance. It's available starting in Visual Studio 2019 version 16.8. (https://learn.microsoft.com/en-us/cpp/build/reference/std-specify-language-standard-version?view=msvc-140)

mmuetzel commented 4 months ago

Thank you for clarifying. I was (erroneously) assuming that the /std:c11 option would be available in Visual Studio 2014 if they are referring to it in the documentation for that version.

In that case, it would be nice if you could remove the _MSC_VER condition and use __pragma unconditionally if HAVE_PRAGMA_LOOP_IVDEP or HAVE_PRAGMA_LOOP_NO_VECTOR is defined. Afaict, only MSVC compatible compilers have support for #pragma loop( XYZ ). So, it is not necessary to check for that again.

mmuetzel commented 4 months ago

This change seems to be causing warnings like the following during compilation with Visual Studio 2022 on the GitHub-runners: https://github.com/DrTimothyAldenDavis/SuiteSparse/actions/runs/9957325104/job/27509087596?pr=855#step:16:1022

D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(115): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(135): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(266): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(282): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(642): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(663): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(689): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(736): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(761): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(820): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(838): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(884): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(968): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(995): warning C4081: expected 'identifier'; found 'string'

That might mean that the pragma is ignored with that compiler.

Does it work correctly for you with Visual Studio 2017?

mmuetzel commented 4 months ago

Could it be that MSVC doesn't cope with spaces in the pragma? Does it work if you use # define UMFPACK_IVDEP __pragma(loop(ivdep)) (without the spaces around ivdep)?

whuaegeanse commented 4 months ago

This change seems to be causing warnings like the following during compilation with Visual Studio 2022 on the GitHub-runners: https://github.com/DrTimothyAldenDavis/SuiteSparse/actions/runs/9957325104/job/27509087596?pr=855#step:16:1022

D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(115): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(135): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(266): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(282): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(642): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(663): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(689): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(736): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(761): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(820): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(838): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(884): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(968): warning C4081: expected 'identifier'; found 'string'
D:\a\SuiteSparse\SuiteSparse\UMFPACK\Source\umf_assemble.c(995): warning C4081: expected 'identifier'; found 'string'

That might mean that the pragma is ignored with that compiler.

Does it work correctly for you with Visual Studio 2017?

Sorry for the late reply. I missed the MSVC warning above, and indeed I didn't encounter the previous compilation errors in Visual Studio 2017.

DrTimothyAldenDavis commented 4 months ago

I'm back from vacation and getting caught up. I revised the PR to remove the spaces from the pragmas, to see if that changes the warnings.

DrTimothyAldenDavis commented 4 months ago

Removing the spaces in the pragmas causes all the warning C4081: expected 'identifier'; found 'string' messages to go away.