This is an archive of the discontinued LLVM Phabricator instance.

Switch the Windows OpenMP import library to import by name rather than ordinal.

Authored by vadikp-intel on Feb 6 2023, 12:06 PM.



The current OpenMP build script generates the Windows import library that imports by ordinal. This requires ordinal numbering coordination between contributors adding new APIs to the runtime and has in the past caused collisions requiring breaking changes to resolve them. Switching the library to import by name helps to avoid such collisions while also increasing compatibility between the different Windows versions of the OpenMP runtime (e.g. stock, MSVC's, etc.). The change keeps the existing ordinal ordering in the runtime to maintain compatibility of applications built with previous OpenMP releases. Note: due to incomplete 'llvm-lib' compatibility with 'lib' currently the solution only works with MSVC or ICC based Windows builds.

Diff Detail

Event Timeline

vadikp-intel created this revision.Feb 6 2023, 12:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 12:06 PM
vadikp-intel requested review of this revision.Feb 6 2023, 12:06 PM

Can you update the patch with context?

Please add the MSVC folks as reviewers too.

natgla accepted this revision.Feb 8 2023, 9:37 AM
This revision is now accepted and ready to land.Feb 8 2023, 9:37 AM

Can you comment on the removal of kmp_import.cpp? Is it not needed anymore because Microsoft is moving away from the vcomp interface? If it is indeed unnecessary, then shouldn't we just remove the file entirely?

kmp_import.cpp turned up as a random side-effect of this change as the latter piggy-backed on the existing build step that rebuilt the import library dragging kmp_import in (for unrelated reasons, apparently to enforce some rules for code built with a toolset mix) With this change, kmp_imports is no longer necessary because the intent looks to already be covered elsewhere in the code now being linked (kmp_import might be some workaround from days past). I did not include its removal here for two reasons a) it is not directly related to the intent of the ordinals change and b) we (MSVC and Intel) are discussing what the desired behavior here should going forward for LLVM OpenMP flavors. I will remove kmp_import (if it is indeed no longer required) with the check-in that effects what we want this to be.

Jonathan, if all looks good could you commit on my behalf?

This patch broke building in mingw mode, with errors like this:

ninja: error: multiple rules generate runtime/src/libomp.dll.a [-w dupbuild=err]

I've got a patch I can submit to fix that. But this patch also has a couple of other issues.


This patch adds a new nice define LIBOMP_GENERATED_DEF_FILE here, but on line 240, we still refer to it by hardcoding its value, add_custom_target(libomp-needed-windows-files DEPENDS ${LIBOMP_LIB_NAME}.def). It would be good if both references would use the same cmake variable, since right now, it seems like you can adjust its name, but that only causes breakage.


Here, you're generating two files with one single cmake add_custom_command. This means that cmake essentially doesn't know that the second file, LIBOMPIMP_GENERATED_DEF_FILE_IMP is generated. When cleaning, it doesn't get removed, and if you just build ninja runtime/src/libomp.lib without having built the rest before, then cmake/ninja won't know about generating the def file before. So I'd suggest generating that second file with a separate command so that cmake knows and can track the file (and probably adding a suitable depenency for the ompimp target so that it generates the def file as needed).


Previously, the finally installed import library contained the compiled kmp_import.cpp too, which contains symbols like _You_must_link_with_exactly_one_OpenMP_library. These are now lost.


By adding all source files here, you end up compiling all the object files twice - even if they aren't really strictly used for the second invocation (where we just invoke lib.exe to regenerate an import library). Not sure if cmake is ok with that, or if it would need a dummy source file at least.

Compiling all files twice feels wasteful; the openmp runtime isn't that big so it's not a huge deal, but it seems quite redundant and inelegant.

mstorsjo added inline comments.Feb 14 2023, 1:54 AM

Sorry, I see that this particular aspect was actually already discussed.