Improvements to the OpenMP Windows build

[vadikp-intel]

Summary

This check-in makes the following improvements to the OpenMP Windows build:

1. Only generate the second def file when necessary (native Windows import library builds).
2. Properly clean up .def file artifacts.
3. Reduce the re-generated import library build artifacts to the minimum.
4. Refactor the import library related portions of the script for clarity.

tested with MSVC and MinWG/gcc12.0

[mstorsjo]

A couple minor comments, otherwise this seems fine for me too, for mingw builds.

I noticed that when building with clang-cl, though, this fails - but apparently it already did in all the preceding versions. When building with clang-cl, cmake seems to prefer to use llvm-lib instead of MS lib.exe (at least when cross compiling), but llvm-lib doesn't support the /def: option for generating an import library. Thus, I believe OpenMP builds with clang-cl on current git main is broken (since 06d9bf5e64d472db5485815d9c3f70631064bb25 / D143431).

CC @hans @pbo-linaro @omjavaid - does this break anything for your Windows releases (which I believe some are built with clang-cl, and some which do include OpenMP)? Although I guess it's possible to build with something like -DCMAKE_AR=lib.exe to force a preference for the tool which does support the /def option.

I guess the ideal thing would be to implement the option in llvm-lib so this wouldn't be an issue.

openmp/runtime/src/CMakeLists.txt

276

Typo: backwads

287

I would prefer to use the term "MSVC style builds" instead of "native Windows builds", as you can cross compile with MSVC/clang-cl too, and do mingw builds natively on Windows.

[mstorsjo]

Oh, btw, can you clarify what the intended procedure is, for coordinating allocating new ordinals, whenever there's a new export to be added to the dllexports file? This comes up every now and then when a new exported function is added (by someone who knows little about the OpenMP Windows builds), then the Windows build is broken, and someone else has to add that export and give it a number, to unbreak the build again.

[vadikp-intel]

Yes, clang-cl style builds are a known open issue, we are looking at patching llvm-lib.

The intent of this change is to switch applications built with OpenMP to importing by name, rather than ordinal from the runtime. This way, e.g. someone who find themselves in conflict when adding a new API can just move it to a different ordinal without breaking anything for anyone. I am not sure there's been a particular established procedure of assigning ordinals to new OpenMP APIs, but after this change it should not matter, and I think 'dllexport' has already not even technically required ordinals on APIs for the build to succeed.

I think the step is about builds that follow the "native" Windows import library model and have changed the description to say that. 'MSVC style' is OK too, but I think the term is a bit vague and imprecise, e.g. you can build Windows DLLs with import libraries from assembly code as well.

[mstorsjo]

In D144419#4140167, @vadikp-intel wrote:

I think the step is about builds that follow the "native" Windows import library model and have changed the description to say that. 'MSVC style' is OK too, but I think the term is a bit vague and imprecise, e.g.

Well I also find the term "native Windows import library model" extremely vague here - to the point that I simply don't understand on a technical level what you refer to. Mingw toolchains also use the same sort of Windows import libraries as MSVC style based toolchains. What part of the import library model do you consider to differ?

you can build Windows DLLs with import libraries from assembly code as well.

Right, but it doesn't refer to MSVC as in the C/C++ compiler, but it refers to the collective tool interface model (mingw or msvc), where e.g. clang based tools can be used both with a MSVC style interface (with clang-cl, llvm-lib, lld-link, etc) and a mingw style interface (with clang, llvm-ar, ld.lld, llvm-dlltool). In that case, it's primarily the tool interface which affects what you can and can't do. Within CMake, this nuance is primarily signalled with if (MSVC).

(With that in mind, it's indeed a bit of an issue that llvm-lib doesn't support the /def flag.)

[pbo-linaro]

@mstorsjo We noticed a regression when linking libomp.lib for windows-arm64 a week ago, but we lacked time to investigate.
So for now, we just disabled openmp build (on our side) for this platform. So feel free to change this, it's already broken.

[hans]

In D144419#4139886, @mstorsjo wrote:

CC @hans @pbo-linaro @omjavaid - does this break anything for your Windows releases (which I believe some are built with clang-cl, and some which do include OpenMP)?

Yes, I just tried the script and the release build does indeed seem to be broken because of openmp.

Although I guess it's possible to build with something like -DCMAKE_AR=lib.exe to force a preference for the tool which does support the /def option.

Just -DCMAKE_AR=lib.exe doesn't seem to work, as it needs the full path.

Patches welcome for that, or for implementing llvm-lib /def: support. Should we revert D143431 in the meantime?

[vadikp-intel]

Updated the controversial comment to promote toolchain equity.

[mstorsjo]

In D144419#4142780, @vadikp-intel wrote:

Updated the controversial comment to promote toolchain equity.

Thanks. Although, it's not about promoting equality or something like that, it's just about using precise terminology.

Anyway, the updated comment seems ok to me - thanks!

In D144419#4141367, @hans wrote:

Although I guess it's possible to build with something like -DCMAKE_AR=lib.exe to force a preference for the tool which does support the /def option.

Just -DCMAKE_AR=lib.exe doesn't seem to work, as it needs the full path.

Right - that's annoying and inconsistent in cmake that those tools need to be specified with the full path - and the equivalent of -DCMAKE_AR=$(which llvm-ar) isn't trivial in bat files...

Should we revert D143431 in the meantime?

Yeah I guess that's one option.

As for alternative ways forward - I believe implementing the /def option in llvm-lib is a bit more messy than one would want; in MS tools, lib.exe is just a thin wrapper over link.exe, while llvm-lib resides within llvm but lld is a separate project. But it might be possible to hoist those parts of the import lib creation logic from lld into llvm (I think a fair bit of it already has been moved there) to facilitate implementing this option.

But as noted earlier, lld-link doesn't seem to actually produce import libraries that link by ordinal anyway, so this step isn't really necessary there. So if we'd have linker identification in cmake here (not sure offhand if cmake provides that out of the box?), we could just skip the extra import lib step for now too.

[vadikp-intel]

merge check

[mstorsjo]

It seems like this change has broken building OpenMP with MSVC as a toplevel project from the llvm-project/llvm directory. To reproduce, make a build directory as a subdirectory to llvm-project/llvm, and configure it with cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang;openmp". When I try to build with ninja, I get the following error:

ninja: error: 'projects/openmp/runtime/src/libomp.dll.lib', needed by 'lib/libomp.lib', missing and no known rule to make it

It seems like some of the subdirectory path prefixes end up applied inconsistently breaking the dependency chain here, making ninja refuse to proceed.

[hans]

Also, as mentioned above, the Windows LLVM release currently can't build OpenMP, so as it currently stands LLVM 17 will not include the OpenMP runtime for Windows.

[mstorsjo]

In D144419#4219492, @hans wrote:

Also, as mentioned above, the Windows LLVM release currently can't build OpenMP, so as it currently stands LLVM 17 will not include the OpenMP runtime for Windows.

The issue that the build tried to use an option that llvm-lib didn't support, should have been fixed though - llvm-lib has got support for the option now.

When building the Windows LLVM releases, do you use llvm-lib from the previous version to build OpenMP, or does it first bootstrap everything with MSVC, and then use llvm-lib from the current version when building e.g. OpenMP? In the former case, it wouldn't work (until you get a new enough version of llvm-lib), in the latter case it should work now again.

[hans]

In D144419#4219493, @mstorsjo wrote:

In D144419#4219492, @hans wrote:

Also, as mentioned above, the Windows LLVM release currently can't build OpenMP, so as it currently stands LLVM 17 will not include the OpenMP runtime for Windows.

The issue that the build tried to use an option that llvm-lib didn't support, should have been fixed though - llvm-lib has got support for the option now.

Oh, that's good.

When building the Windows LLVM releases, do you use llvm-lib from the previous version to build OpenMP, or does it first bootstrap everything with MSVC, and then use llvm-lib from the current version when building e.g. OpenMP? In the former case, it wouldn't work (until you get a new enough version of llvm-lib), in the latter case it should work now again.

It's the latter. Thanks!

It seems we still need the problem you mentioned fixed for the first stage of the build though.

[vadikp-intel]

Could you describe some more what needs fixing? The Windows build with a MSVC toolset should perhaps use lib.

[mstorsjo]

In D144419#4219864, @vadikp-intel wrote:

Could you describe some more what needs fixing? The Windows build with a MSVC toolset should perhaps use lib.

I'm not sure exactly what should be changed to fix it - I did a quick 5 minute look at it but didn't identify what should be fixed. I'd recommend trying to set up such a build as I described and reproduce it:

In D144419#4219290, @mstorsjo wrote:

To reproduce, make a build directory as a subdirectory to llvm-project/llvm, and configure it with cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang;openmp". When I try to build with ninja, I get the following error:

ninja: error: 'projects/openmp/runtime/src/libomp.dll.lib', needed by 'lib/libomp.lib', missing and no known rule to make it

[mstorsjo]

In D144419#4219864, @vadikp-intel wrote:

Could you describe some more what needs fixing? The Windows build with a MSVC toolset should perhaps use lib.

Sorry, I think I misinterpreted the question. This new issue isn’t about lib vs llvm-lib, but is about the chain of dependencies in cmake, for the build configuration where the build is rooted in the llvm directory instead of the openmp directory, where one path has got a prefix with the path to the openmp directory, while another one hasn’t. When the build is rooted in the openmp, that path prefix is empty and there’s no issue.

[vadikp-intel]

The source of problem is the attempt to re-use the results of the first build in order to avoid rebuilding the sources the second time to just re-generate the import library. This is being done by feeding the main build's import library as a source to the second one (@301) with CMAKE ending up assuming the library is located where the sources are, hence the reference to projects/openmp/runtime/src/libomp.dll.lib instead of what should be lib/libomp.dll.lib in the LLVM based build. At this point, I do not see a good way to just repoint CMAKE to the first build's drop location, and reusing its library would likely require doing a custom target. I think a cleaner and better solution is to simply rebuild the second one from sources (it's a small build). I'll submit a patch.

[mstorsjo]

In D144419#4221935, @vadikp-intel wrote:

The source of problem is the attempt to re-use the results of the first build in order to avoid rebuilding the sources the second time to just re-generate the import library. This is being done by feeding the main build's import library as a source to the second one (@301) with CMAKE ending up assuming the library is located where the sources are, hence the reference to projects/openmp/runtime/src/libomp.dll.lib instead of what should be lib/libomp.dll.lib in the LLVM based build.

Thanks for investigating this bit, that makes the issue much clearer!

At this point, I do not see a good way to just repoint CMAKE to the first build's drop location, and reusing its library would likely require doing a custom target. I think a cleaner and better solution is to simply rebuild the second one from sources (it's a small build). I'll submit a patch.

While the library is rather small and duplicate compilation isn't that much extra work, I took a shot at making it work with both directory layouts, and I think I got it working; we just need to prefix the original import library with an absolute path to the directory where it's stored. I'll post an alternative patch that does this.

FWIW, in one sense we shouldn't really need these files as input to the lib command at all - lib can generate an import library from a def file just fine, without any object files or preexisting library at all - just lib /def:libomp.def /out:libomp.lib. Unfortunately, cmake doesn't really accept a library target without any source files (where the only "source file" is passed in as the def file via STATIC_LIBRARY_OPTIONS). And once you pass one object file to lib, it will check that all the exported symbols exist, so we can't just pass a dummy object file, we do need to pass all these (as you've noticed). FWIW I tried to see if there are other solutions to this, and found https://gitlab.kitware.com/cmake/cmake/-/issues/21638 - so it doesn't seem that there are any good solutions. Although generating the import library with a custom target might not be that bad either (I might give it a try).

openmp/runtime/src/CMakeLists.txt

 # Create *.inc before compiling any sources
 # objects depend on : .inc files
 add_custom_target(libomp-needed-headers DEPENDS kmp_i18n_id.inc kmp_i18n_default.inc)
 add_dependencies(omp libomp-needed-headers)
 
 # Windows specific build rules
 if(WIN32)
   configure_file(libomp.rc.var libomp.rc @ONLY)
-
-  # Create .def and .rc file before compiling any sources
-  add_custom_target(libomp-needed-windows-files DEPENDS ${LIBOMP_LIB_NAME}.def)
-  add_dependencies(omp libomp-needed-windows-files)
   # z_Windows_NT-586_asm.asm requires definitions to be sent via command line
   # It only needs the architecture macro and OMPT_SUPPORT=0|1
   libomp_append(LIBOMP_MASM_DEFINITIONS "-D_M_IA32" IF_TRUE IA32)
   libomp_append(LIBOMP_MASM_DEFINITIONS "-D_M_AMD64" IF_TRUE INTEL64)
   libomp_append(LIBOMP_MASM_DEFINITIONS "-DOMPT_SUPPORT" IF_TRUE_1_0 LIBOMP_OMPT_SUPPORT)
   libomp_append(LIBOMP_MASM_DEFINITIONS "-DOMPD_SUPPORT" IF_TRUE_1_0 LIBOMP_OMPD_SUPPORT)
   libomp_list_to_string("${LIBOMP_MASM_DEFINITIONS}" LIBOMP_MASM_DEFINITIONS)
   set_property(SOURCE z_Windows_NT-586_asm.asm APPEND_STRING PROPERTY COMPILE_FLAGS " ${LIBOMP_MASM_DEFINITIONS}")
else()
     set(LIBOMP_GENERATED_IMP_LIB_FILENAME ${LIBOMP_LIB_FILE}${CMAKE_STATIC_LIBRARY_SUFFIX})
   endif()
   set_target_properties(omp PROPERTIES
     VERSION ${LIBOMP_VERSION_MAJOR}.${LIBOMP_VERSION_MINOR} # uses /version flag
     IMPORT_PREFIX "" IMPORT_SUFFIX "" # control generated import library name when building omp
     ARCHIVE_OUTPUT_NAME ${LIBOMP_GENERATED_IMP_LIB_FILENAME}
   )
 
-  if(MSVC)
-    set(LIBOMP_IMP_LIB_TARGET ompimp)
-  else()
-    set(LIBOMP_IMP_LIB_TARGET omp)
-  endif()
+  set(LIBOMP_GENERATED_DEF_FILE ${LIBOMP_LIB_NAME}.def)
+  add_custom_target(libomp-needed-def-file DEPENDS ${LIBOMP_GENERATED_DEF_FILE})
+  add_dependencies(omp libomp-needed-def-file)
     
-  # Create def files to designate exported functions
-  libomp_get_gdflags(LIBOMP_GDFLAGS) # generate-def.pl flags (Windows only)
+  # Create the main def file with ordinals to use for building the runtime dll to maintain backwards compatible exports order
+  libomp_get_gdflags(LIBOMP_GDFLAGS)
   libomp_string_to_list("${LIBOMP_GDFLAGS}" LIBOMP_GDFLAGS)
-  set(LIBOMP_GENERATED_DEF_FILE ${LIBOMP_LIB_NAME}.def)
-  set(LIBOMPIMP_GENERATED_DEF_FILE_IMP ${LIBOMP_LIB_NAME}.imp.def)
   add_custom_command(
-    OUTPUT  ${LIBOMP_LIB_NAME}.def
-    # one with ordinals to use for building the runtime dll to maintain backwads compatible exports order
+    OUTPUT  ${LIBOMP_GENERATED_DEF_FILE}
     COMMAND ${PERL_EXECUTABLE} ${LIBOMP_TOOLS_DIR}/generate-def.pl ${LIBOMP_GDFLAGS} -D NAME=${LIBOMP_LIB_FILE}
             -o ${LIBOMP_GENERATED_DEF_FILE} ${CMAKE_CURRENT_SOURCE_DIR}/dllexports
     DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/dllexports ${LIBOMP_TOOLS_DIR}/generate-def.pl
-    # one without ordinals to use for building the import library to remove ordinal dependency going forward
-    COMMAND ${PERL_EXECUTABLE} ${LIBOMP_TOOLS_DIR}/generate-def.pl ${LIBOMP_GDFLAGS} -D NAME=${LIBOMP_LIB_FILE} -D NOORDINALS
-            -o ${LIBOMPIMP_GENERATED_DEF_FILE_IMP} ${CMAKE_CURRENT_SOURCE_DIR}/dllexports
-    DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/dllexports ${LIBOMP_TOOLS_DIR}/generate-def.pl
   )
 
-  if (MSVC)
-    # Regenerate the import library to import by name, not ordinal.
-    #
-    # For mingw, we can't regenerate an import library by passing
-    # CMAKE_LINK_DEF_FILE_FLAG to the static library archiver; that just
-    # ends up creating a regular static library that contains the def file.
-    # For mingw, we would have to call the suitable dlltool for regenerating
-    # an import library. However, neither GNU nor LLVM based mingw tools
-    # generate import libraries that actually link by ordinal, so this step
-    # isn't strictly necessary.
-    #
-    # Also, in mingw builds, LIBOMP_GENERATED_IMP_LIB_FILENAME and
-    # LIBOMP_IMP_LIB_FILE currently end up equal, while they need to differ
-    # for this second step to work.
-    add_library(ompimp STATIC ${LIBOMP_SOURCE_FILES})
-      set_target_properties(ompimp PROPERTIES
-        PREFIX "" SUFFIX "" OUTPUT_NAME "${LIBOMP_IMP_LIB_FILE}"
-      LINKER_LANGUAGE C
-    )
-    set_target_properties(ompimp PROPERTIES STATIC_LIBRARY_OPTIONS
-        "${CMAKE_LINK_DEF_FILE_FLAG}${CMAKE_CURRENT_BINARY_DIR}/${LIBOMPIMP_GENERATED_DEF_FILE_IMP}"
+  if(MSVC)
+    # For toolchains that generated the import library importing by ordinal, re-generate it to import by name
+    set(LIBOMP_IMP_LIB_TARGET ompimp)
+    # Create the auxiliary def file without ordinals to use for building the import library to import by name 
+    set(LIBOMPIMP_GENERATED_DEF_FILE ${LIBOMP_LIB_NAME}.imp.def)
+    add_custom_target(libompimp-needed-def-file DEPENDS ${LIBOMPIMP_GENERATED_DEF_FILE})
+    add_custom_command(
+      OUTPUT  ${LIBOMPIMP_GENERATED_DEF_FILE}
+      COMMAND ${PERL_EXECUTABLE} ${LIBOMP_TOOLS_DIR}/generate-def.pl ${LIBOMP_GDFLAGS} -D NAME=${LIBOMP_LIB_FILE} -D NOORDINALS
+          -o ${LIBOMPIMP_GENERATED_DEF_FILE} ${CMAKE_CURRENT_SOURCE_DIR}/dllexports
+      DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/dllexports ${LIBOMP_TOOLS_DIR}/generate-def.pl
     )
-    add_dependencies(ompimp libomp-needed-headers)
+    # while this is merely generating an import library off the one generated with the runtime dll,
+    # kmp_global.cpp will satisfy the librarian's apparent ask to see the actual exported DATA items
+    set_source_files_properties(${LIBOMP_GENERATED_IMP_LIB_FILENAME} PROPERTIES GENERATED TRUE EXTERNAL_OBJECT TRUE)
+    add_library(${LIBOMP_IMP_LIB_TARGET} STATIC ${LIBOMP_GENERATED_IMP_LIB_FILENAME} kmp_global.cpp)
+    set_target_properties(${LIBOMP_IMP_LIB_TARGET} PROPERTIES
+        PREFIX "" SUFFIX "" OUTPUT_NAME "${LIBOMP_IMP_LIB_FILE}" LINKER_LANGUAGE C
+        STATIC_LIBRARY_OPTIONS "${CMAKE_LINK_DEF_FILE_FLAG}${CMAKE_CURRENT_BINARY_DIR}/${LIBOMPIMP_GENERATED_DEF_FILE}")
+    add_dependencies(${LIBOMP_IMP_LIB_TARGET} omp libompimp-needed-def-file)
   endif()
-
 endif()
 
 # Building the Fortran module files
 # One compilation step creates both omp_lib.mod and omp_lib_kinds.mod
 if(${LIBOMP_FORTRAN_MODULES})
   configure_file(${LIBOMP_INC_DIR}/omp_lib.h.var omp_lib.h @ONLY)
   configure_file(${LIBOMP_INC_DIR}/omp_lib.f90.var omp_lib.f90 @ONLY)
   # Workaround for gfortran to build modules with the