[Openmp-commits] [PATCH] D144419: Improvements to the OpenMP Windows build

[Martin Storsjö via Phabricator via Openmp-commits]

mstorsjo added subscribers: pbo-linaro, omjavaid, hans.
mstorsjo added a comment.

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 <https://reviews.llvm.org/rG06d9bf5e64d472db5485815d9c3f70631064bb25> / D143431 <https://reviews.llvm.org/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.



================
Comment at: openmp/runtime/src/CMakeLists.txt:276
+    
+  # Create the main def file with ordinals to use for building the runtime dll to maintain backwads compatible exports order
+  libomp_get_gdflags(LIBOMP_GDFLAGS)
----------------
Typo: `backwads`


================
Comment at: openmp/runtime/src/CMakeLists.txt:287
+  if(MSVC)
+    # For native Windows builds we need to regenerate the import library to import by name
+    set(LIBOMP_IMP_LIB_TARGET ompimp)
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144419/new/

https://reviews.llvm.org/D144419