Don't link gomp on MSVC (openmp-runtime-comp)#144
Merged
Conversation
`gomp` is GNU's OpenMP runtime and does not exist on the MSVC toolchain, so building with `openmp-runtime-comp` on x86_64-pc-windows-msvc fails with `LNK1181: cannot open input file 'gomp.lib'`. MSVC's /openmp emits a /DEFAULTLIB:VCOMP directive into every object, so its runtime links automatically and no explicit link is needed. Skip the gomp link on the MSVC target env only, in both the source-build and system-library paths. Other toolchains, including *-pc-windows-gnu (MinGW/GCC), still link it as before. Refs jkawamoto#64
Owner
|
Thank you for the detailed investigation and explanation! That makes sense, especially the distinction between MSVC and windows-gnu here, and thanks for pointing out why I also appreciate the additional context about the Windows hangs and the remaining Ruy-related issue. The proposed change looks good to me. I’ll merge the PR. |
Contributor
Author
|
Thanks for the quick review and merge, really appreciate it! Do you have a rough idea of when this might go out in a release? No rush at all. Just so I know whether to keep patching it locally in the meantime. Thanks again! |
Owner
|
Thanks again for the awesome PR. Just wanted to let you know that I've just published v0.9.22 with your fix included. Appreciate your help! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi, thanks for these bindings. While looking into the Windows hang from #64 I found the
openmp-runtime-compfeature can't link on MSVC, which is a small fix.openmp-runtime-compunconditionally linksgomp, which is GNU's OpenMP runtime anddoesn't exist on the MSVC toolchain. Building with the feature on
x86_64-pc-windows-msvcfails:MSVC doesn't need an explicit link here:
/openmpemits a/DEFAULTLIB:VCOMPdirective into every object, so its runtime is pulled in automatically. So the fix is
to skip the
gomplink on MSVC only. Every other toolchain, includingwindows-gnu(MinGW uses GCC and has no such directive), still links it as before. This applies to
both the source-build path (
build_ctranslate2) and thesystem-feature path(
link_system_libraries), which each linkgompunderopenmp-runtime-comp:I used
CARGO_CFG_TARGET_ENVrather thancfg!(target_env = ...)since the latterreflects the host in a build script. Linux, macOS and windows-gnu are unaffected.
Why this matters on Windows: with
OPENMP_RUNTIME=NONE(the default here), CT2's CPUparallelism uses a
static thread_local BS::thread_poolwhose destructor deadlocks onthread exit. Building CT2 with OpenMP compiles that path out, so
openmp-runtime-compis currently the only way to avoid that hang on Windows, and right now the feature
can't even link on MSVC.
This doesn't fix everything on Windows on its own. There's a second deadlock in the Ruy
backend's
thread_local ruy::Contextthat OpenMP doesn't touch, currently avoided bythe
#[cfg(windows)] impl Dropthat bypasses the UniquePtr drop (which leaks themodel). The real fix for that is upstream in CTranslate2 OpenNMT/CTranslate2#2076, after which the
Drop bypass can be reverted.
Thanks!