Don't link to `libomp` when targeting MinGW.

[mmuetzel]

There is no libomp library when targeting MinGW (at least when building with GCC). Make that work-around(?) specific to a MSVC toolchain.

[mmuetzel]

There isn't any comment in the file or in the commit that added that special case for WIN32: https://github.com/ROCm/rocALUTION/commit/078e7a7f70fb209843b262f4cf8bd7bbbd04fbab#diff-148715d6ea0c0ea0a346af3f6bd610d010d490eca35ac6a9b408748f7ca9e3f4R88 I'd guess this was used to work around an issue that has since been fixed in CMake(?).

Would it be ok to remove that special case for Windows entirely and just link to the OpenMP::OpenMP_CXX target for all platforms? Nothing more than that should be necessary.

If using OpenMP requires additional flags with some (broken?) toolchains, these additional flags shouldn't be hard-coded into the build rules imho. Instead a user (with a broken toolchain?) should set flags manually when configuring. In this case, that might be -DOpenMP_CXX_LIB_NAMES=libomp.

What is your opinion on that?

[jsandham]

There isn't any comment in the file or in the commit that added that special case for WIN32: 078e7a7#diff-148715d6ea0c0ea0a346af3f6bd610d010d490eca35ac6a9b408748f7ca9e3f4R88 I'd guess this was used to work around an issue that has since been fixed in CMake(?).

Would it be ok to remove that special case for Windows entirely and just link to the OpenMP::OpenMP_CXX target for all platforms? Nothing more than that should be necessary.

If using OpenMP requires additional flags with some (broken?) toolchains, these additional flags shouldn't be hard-coded into the build rules imho. Instead a user (with a broken toolchain?) should set flags manually when configuring. In this case, that might be -DOpenMP_CXX_LIB_NAMES=libomp.

What is your opinion on that?

Good point. Let me check with the team about why the original change was made and report back.

[cgmb]

If using OpenMP requires additional flags with some (broken?) toolchains, these additional flags shouldn't be hard-coded into the build rules imho. Instead a user (with a broken toolchain?) should set flags manually when configuring. In this case, that might be -DOpenMP_CXX_LIB_NAMES=libomp.

I'd like to agree, but I'm not sure we can really ask that of our internal users. I think the path forward to getting rid of this would be to fix the toolchain if it's broken. So, step one is to figure out why that workaround is needed. Step two would be to file a bug with the appropriate component. Step three would be to remove the workaround.

[mmuetzel]

I'd like to agree, but I'm not sure we can really ask that of our internal users. I think the path forward to getting rid of this would be to fix the toolchain if it's broken. So, step one is to figure out why that workaround is needed. Step two would be to file a bug with the appropriate component. Step three would be to remove the workaround.

Given which compiler and target you are seeing this issue with and how you were able to work around it, I'm pretty sure it is the same CMake issue we were seeing in SuiteSparse: https://github.com/DrTimothyAldenDavis/SuiteSparse/issues/750

That issue has been fixed in CMake 3.29.

I've updated the PR to be more specific when the work-around is applied. That should allow your internal users to still build without any additional flags while avoiding issues with other compilers (e.g., GCC).

[mmuetzel]

It looks like some of the CI failed. But I cannot access the logs. Are the failures related to changes from this PR?

[ntrost57]

It's file formatting failure, introduced with PR #244. I have fixed it.