-
Notifications
You must be signed in to change notification settings - Fork 543
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
Using FindThreads instead of hardcode -lpthread
.
#13118
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Maybe @benvanik or @GMNGeoffrey could take a quick look also?
@@ -334,15 +334,17 @@ if(CMAKE_CXX_FLAGS AND "${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") | |||
string(REPLACE "/GR" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}") | |||
endif() | |||
|
|||
# Find and add threads as dependency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's not necessary, but I think it would be nice to explicitly initialize thread deps to empty here, just so that we aren't expanding an undefined variable later. I think modern cmake may actually have a flag to error on undefined variables that we could turn on (IIRC we used to support cmake versions that were too old to enable this)
Hmm there's no way for me to enable auto-merge, but still allow you to fix my comment. To unblock things, I'm just going to turn on auto-merge and approve the CI run. If you see my comment before it completes, go ahead and add that, but otherwise it will just merge and I'll look into whether we can make cmake error on undefined variables |
To fix #13114 From what I have read in `iree_copts.cmake`, if it is not targeting android and thread is enabled, link to pthread by default for all of the libraries. So I just `link_libraries(Threads::Threads)`, assuming there is no libraries don't like accepting pthread and want to override the default. It compiles successfully on my machine. Here is the generated compile command: ``` ccache /usr/bin/c++ -O3 -DNDEBUG -lm CMakeFiles/iree_samples_simple_embedding_simple_embedding_vmvx_sync.dir/device_vmvx_sync.c.o CMakeFiles/iree_samples_simple_embedding_simple_embedding_vmvx_sync.dir/simple_embedding.c.o -o ../../../../dist/misc/bin/simple_embedding_vmvx_sync ../../../../dist/misc/lib/libiree_samples_simple_embedding_simple_embedding_test_bytecode_module_vmvx_c.a ../../../../dist/misc/lib/libiree_base_base.a ../../../../dist/misc/lib/libiree_hal_hal.a ../../../../dist/misc/lib/libiree_hal_drivers_local_sync_sync_driver.a ../../../../dist/misc/lib/libiree_hal_local_local.a ../../../../dist/misc/lib/libiree_hal_local_loaders_vmvx_module_loader.a ../../../../dist/misc/lib/libiree_modules_hal_hal.a ../../../../dist/misc/lib/libiree_vm_bytecode_module.a ../../../../dist/misc/lib/libiree_base_internal_fpu_state.a ../../../../dist/misc/lib/libiree_hal_utils_buffer_transfer.a ../../../../dist/misc/lib/libiree_hal_utils_deferred_command_buffer.a ../../../../dist/misc/lib/libiree_hal_utils_resource_set.a ../../../../dist/misc/lib/libiree_base_internal_arena.a ../../../../dist/misc/lib/libiree_base_internal_atomic_slist.a ../../../../dist/misc/lib/libiree_hal_utils_semaphore_base.a ../../../../dist/misc/lib/libiree_vm_bytecode_utils_utils.a ../../../../dist/misc/lib/libflatcc_parsing.a -lm ../../../../dist/misc/lib/libiree_hal_local_executable_loader.a ../../../../dist/misc/lib/libiree_hal_local_executable_environment.a ../../../../dist/misc/lib/libiree_modules_vmvx_vmvx.a ../../../../dist/misc/lib/libiree_base_internal_cpu.a ../../../../dist/misc/lib/libiree_builtins_ukernel_ukernel.a ../../../../dist/misc/lib/libiree_builtins_ukernel_arch_x86_64_x86_64.a ../../../../dist/misc/lib/libiree_builtins_ukernel_arch_x86_64_x86_64_avx2_fma.a ../../../../dist/misc/lib/libiree_builtins_ukernel_arch_x86_64_x86_64_avx512_base.a ../../../../dist/misc/lib/libiree_builtins_ukernel_arch_x86_64_x86_64_avx512_vnni.a ../../../../dist/misc/lib/libiree_modules_hal_utils_buffer_diagnostics.a ../../../../dist/misc/lib/libiree_modules_hal_types.a ../../../../dist/misc/lib/libiree_hal_hal.a ../../../../dist/misc/lib/libiree_base_internal_path.a ../../../../dist/misc/lib/libiree_vm_impl.a ../../../../dist/misc/lib/libiree_base_internal_synchronization.a ../../../../dist/misc/lib/libiree_base_base.a ../../../../dist/misc/lib/libiree_base_tracing.a -lpthread ``` Kind of a crazy big junk but the important thing is `-lpthread` is correctly placed at the back of the command, after `libiree_base_internal_synchronization.a`. I also did not change anything regarding webassembly and riscv as I'm not sure whether FindThreads will also deal with linking for those two platform or not.
To fix iree-org#13114 From what I have read in `iree_copts.cmake`, if it is not targeting android and thread is enabled, link to pthread by default for all of the libraries. So I just `link_libraries(Threads::Threads)`, assuming there is no libraries don't like accepting pthread and want to override the default. It compiles successfully on my machine. Here is the generated compile command: ``` ccache /usr/bin/c++ -O3 -DNDEBUG -lm CMakeFiles/iree_samples_simple_embedding_simple_embedding_vmvx_sync.dir/device_vmvx_sync.c.o CMakeFiles/iree_samples_simple_embedding_simple_embedding_vmvx_sync.dir/simple_embedding.c.o -o ../../../../dist/misc/bin/simple_embedding_vmvx_sync ../../../../dist/misc/lib/libiree_samples_simple_embedding_simple_embedding_test_bytecode_module_vmvx_c.a ../../../../dist/misc/lib/libiree_base_base.a ../../../../dist/misc/lib/libiree_hal_hal.a ../../../../dist/misc/lib/libiree_hal_drivers_local_sync_sync_driver.a ../../../../dist/misc/lib/libiree_hal_local_local.a ../../../../dist/misc/lib/libiree_hal_local_loaders_vmvx_module_loader.a ../../../../dist/misc/lib/libiree_modules_hal_hal.a ../../../../dist/misc/lib/libiree_vm_bytecode_module.a ../../../../dist/misc/lib/libiree_base_internal_fpu_state.a ../../../../dist/misc/lib/libiree_hal_utils_buffer_transfer.a ../../../../dist/misc/lib/libiree_hal_utils_deferred_command_buffer.a ../../../../dist/misc/lib/libiree_hal_utils_resource_set.a ../../../../dist/misc/lib/libiree_base_internal_arena.a ../../../../dist/misc/lib/libiree_base_internal_atomic_slist.a ../../../../dist/misc/lib/libiree_hal_utils_semaphore_base.a ../../../../dist/misc/lib/libiree_vm_bytecode_utils_utils.a ../../../../dist/misc/lib/libflatcc_parsing.a -lm ../../../../dist/misc/lib/libiree_hal_local_executable_loader.a ../../../../dist/misc/lib/libiree_hal_local_executable_environment.a ../../../../dist/misc/lib/libiree_modules_vmvx_vmvx.a ../../../../dist/misc/lib/libiree_base_internal_cpu.a ../../../../dist/misc/lib/libiree_builtins_ukernel_ukernel.a ../../../../dist/misc/lib/libiree_builtins_ukernel_arch_x86_64_x86_64.a ../../../../dist/misc/lib/libiree_builtins_ukernel_arch_x86_64_x86_64_avx2_fma.a ../../../../dist/misc/lib/libiree_builtins_ukernel_arch_x86_64_x86_64_avx512_base.a ../../../../dist/misc/lib/libiree_builtins_ukernel_arch_x86_64_x86_64_avx512_vnni.a ../../../../dist/misc/lib/libiree_modules_hal_utils_buffer_diagnostics.a ../../../../dist/misc/lib/libiree_modules_hal_types.a ../../../../dist/misc/lib/libiree_hal_hal.a ../../../../dist/misc/lib/libiree_base_internal_path.a ../../../../dist/misc/lib/libiree_vm_impl.a ../../../../dist/misc/lib/libiree_base_internal_synchronization.a ../../../../dist/misc/lib/libiree_base_base.a ../../../../dist/misc/lib/libiree_base_tracing.a -lpthread ``` Kind of a crazy big junk but the important thing is `-lpthread` is correctly placed at the back of the command, after `libiree_base_internal_synchronization.a`. I also did not change anything regarding webassembly and riscv as I'm not sure whether FindThreads will also deal with linking for those two platform or not.
To fix #13114
From what I have read in
iree_copts.cmake
, if it is not targeting android and thread is enabled, link to pthread by default for all of the libraries. So I justlink_libraries(Threads::Threads)
, assuming there is no libraries don't like accepting pthread and want to override the default. It compiles successfully on my machine.Here is the generated compile command:
Kind of a crazy big junk but the important thing is
-lpthread
is correctly placed at the back of the command, afterlibiree_base_internal_synchronization.a
.I also did not change anything regarding webassembly and riscv as I'm not sure whether FindThreads will also deal with linking for those two platform or not.