Option to link MIGraphX to MSVC 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.

[apwojcik]

Providing CMAKE_MSVC_RUNTIME_LIBRARY in the command line will not work (in terms of ease of use) for multi-config generators. You do not provide CMAKE_BUILD_TYPE for them when configuring a build because multi-config generators do not use that variable. Instead, you call cmake --build ... --config <build_type>, and then the generator expressions are kicking off to resolve the name of the runtime library. I cannot imagine requiring someone to configure a build to proceed with generator expressions on the command line. Neither library is broken. The authors provided a friendly interface for someone who wishes to configure and build a library easily without deep knowledge of CMake and generator expressions. Please keep in mind that CMAKE_MSVC_RUNTIME_LIBRARY is primarily designed for Visual Studio, which is a multi-config generator.

[pfultz2]

We shouldn't be conditionally adding sqlite based on miopen. MIOpen is specific to the migraphx_gpu target and this is the core migraphx library used by all targets.

[kentqian]

Could you add the description, like "Link MSVC runtime library statically instead of dynamically" instead of leaving blank?