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

Fix UMFPACK compilation errors in Visual Studio 2017 #855

Merged

Conversation

whuaegeanse
Copy link
Contributor

@whuaegeanse whuaegeanse commented Jul 15, 2024

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.

@whuaegeanse whuaegeanse changed the title Fix UMFPACK compilation errors in vs2017 Fix UMFPACK compilation errors in Visual Studio 2017 Jul 15, 2024
@mmuetzel
Copy link
Contributor

mmuetzel commented Jul 15, 2024

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
Copy link
Contributor Author

  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
Copy link
Contributor

mmuetzel commented Jul 15, 2024

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
Copy link
Contributor

mmuetzel commented Jul 23, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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

@DrTimothyAldenDavis DrTimothyAldenDavis merged commit 0aed651 into DrTimothyAldenDavis:dev2 Aug 1, 2024
27 checks passed
@whuaegeanse whuaegeanse deleted the fix_umfpack branch August 1, 2024 16:59
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.

3 participants