Option to link MIGraphX to MVSC Runtime Library statically or dynamically

[apwojcik]

The content of the PR changed from the original submission. Now the PR only adds an option to MIGraphX to enable linking to the MSVC Runtime Library either statically or dynamically. I left the original description below.

Original description:

The change has been migrated from the uai-develop branch.

Windows MIGraphX is still using Protobuf v19. With this PR, we want to switch to Protobuf v30, the same version used by
Linux. However, the Windows environment and differences between Clang make it not a straightforward upgrade.

Using clang-cl requires explicit MSVC runtime library setup for protobuf-generated libraries (and for libraries that link with generated binaries). By default, protobuf sets the MSVC runtime to link statically, while the rest of MIGraphX has the MSVC runtime linked dynamically. We added setting up the MSVC runtime everywhere with this PR, because we will need this in the near future when we switch to linking the MSVC runtime statically for the entire MIGraphX.

Additionally, we do not need to link against SQLite3 if MIGraphX does not depend on the MIOpen.

[pfultz2]

I think you are doing something wrong. The dependencies in requirements.txt build with rbuild using clang-cl, and it doesnt need filter_target_linked_libraries when linking with migraphx.

[pfultz2]

Additionally, we do not need to link against SQLite3 if MIGraphX does not depend on the MIOpen.

I dont think we even use the get_mlir_perf_for_conv anymore, and it probably doesnt even work anymore and should probably be removed. Can you make a seperate PR to remove perfdb.cpp files?

[apwojcik]

I think you are doing something wrong. The dependencies in requirements.txt build with rbuild using clang-cl, and it doesnt need filter_target_linked_libraries when linking with migraphx.

We use clang to compile, and the change is for the clang compiler (not clang-cl). With clang-cl, the MIGraphX compiles with no warnings. We will perform additional testing on binaries built with clang-cl and consider switching the compiler.

[pfultz2]

I think you are doing something wrong. The dependencies in requirements.txt build with rbuild using clang-cl, and it doesnt need filter_target_linked_libraries when linking with migraphx.

We use clang to compile, and the change is for the clang compiler (not clang-cl). With clang-cl, the MIGraphX compiles with no warnings. We will perform additional testing on binaries built with clang-cl and consider switching the compiler.

Either way this change should go into protobuf. MIGraphX shouldn't be monkey patching it.

[pfultz2]

Are you trying to build migraphx with clang-cl as well?

I dont think we need to do this in here. These seem like toochain variables and can be set with -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded$<$<CONFIG:Debug>:Debug>DLL -DCMAKE_CXX_FLAGS_DEBUG=/D _DEBUG /D _ITERATOR_DEBUG_LEVEL=2

[apwojcik]

Are you trying to build migraphx with clang-cl as well?

I dont think we need to do this in here. These seem like toochain variables and can be set with -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded$<$<CONFIG:Debug>:Debug>DLL -DCMAKE_CXX_FLAGS_DEBUG=/D _DEBUG /D _ITERATOR_DEBUG_LEVEL=2

Yes, we switch the compiler to clang-cl. After upgrading to protobuf v30, we need to set that information manually. Otherwise, there are mismatches between the runtime libraries used by MIGraphX and the protobuf-generated libraries. For some reason, the new protoc is not propagating the runtime library and setting _DEBUG and the iterator level correctly.

We do not use a toolchain file for the compiler. As I mentioned in the updated description, we need to switch the entire MIGraphX to the statically linked MSVC runtime soon. We want to change the runtime library linking with a single CMake option. We don't want to set it globally, only for the targets that require it. The function does what we require.

[pfultz2]

For some reason, the new protoc is not propagating the runtime library

It should propagate the runtime library fine. I set something like this in the toolchain: -Dprotobuf_MSVC_STATIC_RUNTIME=Off -DABSL_MSVC_STATIC_RUNTIME=Off -DCMAKE_POLICY_DEFAULT_CMP0091=NEW -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDLL. I dont know if all those flags are needed but I was able to build and run the tests for onnx and tf on windows with thos defined in the toolchain.

and setting _DEBUG and the iterator level correctly.

Are you saying the protoc doesnt have these flags set? That seems like a toolchain mismatch, but maybe protobuf edits the CMAKE_CXX_FLAGS(thats the reason for CMP0091).

We do not use a toolchain file for the compiler.

If you are using cget(or rbuild) you are using a toolchain file. And you can set flags in it with cget init -Dprotobuf_MSVC_STATIC_RUNTIME=Off -DABSL_MSVC_STATIC_RUNTIME=Off -DCMAKE_POLICY_DEFAULT_CMP0091=NEW -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDLL.(I do see cget being used on the windows CI). You can also set those in rbuild session with global_defines.

We want to change the runtime library linking with a single CMake option.

Yes thats what the CMAKE_MSVC_RUNTIME_LIBRARY option does in cmake. We should use the standard options provided by cmake.

We don't want to set it globally

Why? Wont that cause issues with some targets built with different runtimes? Everything should be built with the same runtime.

[apwojcik]

I used the wrong depend folder for testing. When I switched to the correct one, I no longer needed the recent changes I had made. I reverted them. Thanks for noticing!

[pfultz2]

This will remove the sqlite tests on the linux runners. They should not be removed.

[pfultz2]

This should be a cached variable.

[apwojcik]

The CMAKE_MSVC_RUNTIME_LIBRARY variable is a CMake variable and not a cache variable. Please see its documentation and usage example here: https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html

Please see how it is being used, for example, by protobuf, abseil-cpp, boost, and more: https://github.com/protocolbuffers/protobuf/blob/c616b4becd58b9e85bbb3488d38be0d31ab5447f/CMakeLists.txt#L207

https://github.com/abseil/abseil-cpp/blob/33bbc266097fea4cd23b7611a3c9e4c697751719/CMakeLists.txt#L65

https://github.com/boostorg/cmake/blob/7bce8efdc57134dc86e70ac3cd914e390a6cf30d/include/BoostRoot.cmake#L130

[apwojcik]

For command-line usage, the PR defines the MIGRAPHX_MSVC_STATIC_RUNTIME cached variable instead. Similar to other 3rd-party projects (i.e., protobuf_MSVC_STATIC_RUNTIME, ONNX_MSVC_STATIC_RUNTIME, ABSL_MSVC_STATIC_RUNTIME, etc.) that control the value of the CMAKE_MSVC_RUNTIME_LIBRARY variable accordingly.

[pfultz2]

The CMAKE_MSVC_RUNTIME_LIBRARY variable is a CMake variable and not a cache variable. Please see its documentation and usage example here: https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html

Thats not the way cmake variables work. Its a cached variable when the user sets from the command-line or in the toolchain or superproject.

Please see how it is being used, for example, by protobuf, abseil-cpp, boost, and more

Ok so abseil and protobuf dont set it correctly. Its completely broken when users set CMAKE_MSVC_RUNTIME_LIBRARY either in the toolchain or from the command-line.

Boost does set it correctly and we can follow what they did. Instead of a cache variable, we need to check if(NOT CMAKE_MSVC_RUNTIME_LIBRARY) before setting this flag.

For command-line usage, the PR defines the MIGRAPHX_MSVC_STATIC_RUNTIME cached variable instead.

Users should be able to use standard cmake variables instead of trying to figure out the custom flags for each project. Especially for something like this where it needs to be the same across all projects.

[pfultz2]

Suggested change

	set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>${MIGRAPHX_MSVC_RUNTIME_SUFFIX}")
	if(NOT CMAKE_MSVC_RUNTIME_LIBRARY)
	set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>${MIGRAPHX_MSVC_RUNTIME_SUFFIX}")
	endif()