Page MenuHomePhabricator

Add option to build libomp library using Microsoft Visual C++ naming scheme.
ClosedPublic

Authored by branh on Sep 23 2021, 11:06 AM.

Event Timeline

branh created this revision.Sep 23 2021, 11:06 AM
branh requested review of this revision.Sep 23 2021, 11:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2021, 11:06 AM
branh updated this revision to Diff 374629.Sep 23 2021, 11:12 AM
branh retitled this revision from Changes to enable build of libomp with MSVC naming scheme. to Add option to build libomp library using Microsoft Visual C++ naming scheme..

Update title.

Added several questions inline

openmp/CMakeLists.txt
100

What does the 140 mean? Is this supposed to be the llvm release number?
I suggest to use a more descriptive name for this Cmake variable and not to encode a version number into the variable name.

openmp/runtime/CMakeLists.txt
359

Similar as above, what is 140?
Probably combine LLVM_VERSION_MAJOR and LLVM_VERSION_MINOR?

Why not make destinction Debug/non-Debug here?

openmp/runtime/src/CMakeLists.txt
162

CMAKE_BUILD_TYPE is case sensitive. Why make it case-insensitive here?

242–253

The IMP does not need the 140?

jlpeyton added inline comments.Sep 27 2021, 8:16 AM
openmp/runtime/src/CMakeLists.txt
162

To enable standalone builds as well, we can use DEBUG_BUILD variable (defined in openmp/runtime/CMakeLists.txt)

247

Same here, we can use DEBUG_BUILD

branh updated this revision to Diff 377055.Oct 4 2021, 4:51 PM

Addressing review comments.

branh marked an inline comment as done.Oct 5 2021, 2:16 PM
branh added inline comments.
openmp/runtime/CMakeLists.txt
359

The version number corresponds to the toolset the dll is built for. The last time the Visual C++ toolset broke binary compatibility was Visual Studio 14, so the version number is 14.0.

openmp/runtime/src/CMakeLists.txt
162

Every other use of CMAKE_BUILD_TYPE I found were case insensitive as well.

242–253

Only dlls need the 140. The version # is part of the dll name to allow different versions of the same dll to exist on the machine side-by-side without interfering with each other at runtime/load time. Library files are only used at build time, so they don't need the version number.

branh updated this revision to Diff 381386.Oct 21 2021, 2:05 PM

Squashing commits.

branh added inline comments.Oct 21 2021, 3:59 PM
openmp/runtime/src/CMakeLists.txt
162

@protze.joachim @jlpeyton , I'm still confused by the comments about DEBUG_BUILD. As far as I can tell, a case-insensitive version of CMAKE_BUILD_TYPE (sometimes upper case, sometimes lower case) is the method used to determine build type in every CMakeLists.txt. DEBUG_BUILD is set in a CMakeLists.txt file, but it is only used for header files and setting compiler flags.

Is there a plan to switch to using DEBUG_BUILD in CMakeLists.txt files instead of checking CMAKE_BUILD_TYPE at some point in the future?

branh added inline comments.Nov 1 2021, 9:48 AM
openmp/runtime/CMakeLists.txt
359

As for last question (why not make debug/non-debug distinction here), the dlls are named libomp140(d).dll whereas the libraries are named libomp(d).dll in the MSVC naming scheme. Making the name decision closer to where the name is actually used makes which name is used where clearer and helped ensure I didn't miss a name.

jlpeyton added inline comments.Nov 1 2021, 12:33 PM
openmp/runtime/src/CMakeLists.txt
162

I've created https://reviews.llvm.org/D112951 to alleviate any concerns I would have with your approach for CMAKE_BUILD_TYPE. There are two ways to build libomp

  1. Standalone build which only builds libomp (and not any other part of LLVM)
  2. As a part of LLVM. I'm sure this is the grand majority of builds but don't have any statistics on it.

The standalone build doesn't know what uppercase_CMAKE_BUILD_TYPE is since it is defined in llvm/CMakeLists.txt. D112951 will add uppercase_CMAKE_BUILD_TYPE for the standalone build so there won't be any issue with your approach.

branh added inline comments.Nov 1 2021, 3:54 PM
openmp/runtime/src/CMakeLists.txt
162

@jlpeyton That makes sense. Thank you for clearing that up.

Given your proposed changeset, are there any more changes that you think should be made to this changeset before it is accepted? Should I still modify this changeset to use DEBUG_BUILD?

I only have one other suggestion. I think its fine to use uppercase_CMAKE_BUILD_TYPE since its defined everywhere. The DEBUG_BUILD (and friends) are useful as booleans for append_if() type macro/functions in CMake.

openmp/CMakeLists.txt
100

All the user-definable CMake variables in this CMakeLists.txt should be prefixed with OPENMP_, so I think the variable should be OPENMP_MSVC_NAME_SCHEME.

branh updated this revision to Diff 385908.Nov 9 2021, 11:19 AM

Change name of option to include OPENMP

branh updated this revision to Diff 385910.Nov 9 2021, 11:26 AM

Update name for option to include OPENMP

branh marked an inline comment as done.Nov 9 2021, 11:29 AM
jlpeyton accepted this revision.Nov 9 2021, 1:19 PM

LGTM

This revision is now accepted and ready to land.Nov 9 2021, 1:19 PM
branh added a comment.Nov 10 2021, 9:44 AM

My apologies, I don't have commit access. Could someone commit on my behalf? Thank you in advance.