Allow building with Clang and OpenMP

[uilianries]

Hello,

This PR brings the option to build LightGBM with OpenMP, when using Clang, not only Apple Clang.

I can build locally with Linux (Ubuntu 16.04) and Clang 13.0:

> cmake -S . -B build -G "Unix Makefiles" -DCMAKE_INSTALL_PREFIX=“/tmp” -DBUILD_STATIC_LIB="ON" -DUSE_DEBUG="OFF" -DUSE_OPENMP="ON" -DBUILD_CLI="OFF" -DCMAKE_POLICY_DEFAULT_CMP0091="NEW" -DCMAKE_BUILD_TYPE="Release"
-----------------
-- The C compiler identification is Clang 13.0.0
-- The CXX compiler identification is Clang 13.0.0
-- Check for working C compiler: /usr/local/bin/clang
-- Check for working C compiler: /usr/local/bin/clang -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/local/bin/clang++
-- Check for working CXX compiler: /usr/local/bin/clang++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test MM_PREFETCH
-- Performing Test MM_PREFETCH - Success
-- Using _mm_prefetch
-- Performing Test MM_MALLOC
-- Performing Test MM_MALLOC - Success
-- Using _mm_malloc
-- Configuring done
-- Generating done
-- Build files have been written to: /home/foobar/dev/LightGBM/build/Release

----Running------
> cmake --build "/home/foobar/dev/LightGBM/build/Release" '--' '-j3'
-----------------
Scanning dependencies of target lightgbm_capi_objs
[  1%] Building CXX object CMakeFiles/lightgbm_capi_objs.dir/src/c_api.cpp.o
Scanning dependencies of target lightgbm_objs
[  3%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/boosting/boosting.cpp.o
[  5%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/boosting/cuda/cuda_score_updater.cpp.o
[  7%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/boosting/gbdt.cpp.o
[  9%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/boosting/gbdt_model_text.cpp.o
[ 11%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/boosting/gbdt_prediction.cpp.o
[ 13%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/boosting/prediction_early_stop.cpp.o
[ 15%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/boosting/sample_strategy.cpp.o
[ 15%] Built target lightgbm_capi_objs
[ 17%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/cuda/cuda_utils.cpp.o
[ 19%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/bin.cpp.o
[ 21%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/config.cpp.o
[ 23%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/config_auto.cpp.o
[ 25%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/cuda/cuda_column_data.cpp.o
[ 26%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/cuda/cuda_metadata.cpp.o
[ 28%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/cuda/cuda_row_data.cpp.o
[ 30%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/cuda/cuda_tree.cpp.o
[ 32%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/dataset.cpp.o
[ 34%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/dataset_loader.cpp.o
[ 36%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/file_io.cpp.o
[ 38%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/json11.cpp.o
[ 40%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/metadata.cpp.o
[ 42%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/parser.cpp.o
[ 44%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/train_share_states.cpp.o
[ 46%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/io/tree.cpp.o
[ 48%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/metric/cuda/cuda_binary_metric.cpp.o
[ 50%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/metric/cuda/cuda_pointwise_metric.cpp.o
[ 51%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/metric/cuda/cuda_regression_metric.cpp.o
[ 53%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/metric/dcg_calculator.cpp.o
[ 55%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/metric/metric.cpp.o
[ 57%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/network/linker_topo.cpp.o
[ 59%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/network/linkers_mpi.cpp.o
[ 61%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/network/linkers_socket.cpp.o
[ 63%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/network/network.cpp.o
[ 65%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/objective/cuda/cuda_binary_objective.cpp.o
[ 67%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/objective/cuda/cuda_multiclass_objective.cpp.o
[ 69%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/objective/cuda/cuda_rank_objective.cpp.o
[ 71%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/objective/cuda/cuda_regression_objective.cpp.o
[ 73%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/objective/objective_function.cpp.o
[ 75%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/cuda/cuda_best_split_finder.cpp.o
[ 76%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/cuda/cuda_data_partition.cpp.o
[ 78%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/cuda/cuda_histogram_constructor.cpp.o
[ 80%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/cuda/cuda_leaf_splits.cpp.o
[ 82%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/cuda/cuda_single_gpu_tree_learner.cpp.o
[ 84%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/data_parallel_tree_learner.cpp.o
[ 86%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/feature_parallel_tree_learner.cpp.o
[ 88%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/gpu_tree_learner.cpp.o
[ 90%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/gradient_discretizer.cpp.o
[ 92%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/linear_tree_learner.cpp.o
[ 94%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/serial_tree_learner.cpp.o
[ 96%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/tree_learner.cpp.o
[ 98%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/treelearner/voting_parallel_tree_learner.cpp.o
[ 98%] Built target lightgbm_objs
Scanning dependencies of target _lightgbm
[100%] Linking CXX static library /home/foobar/dev/LightGBM/build/Release/lib_lightgbm.a
[100%] Built target _lightgbm

[jameslamb]

Thanks for your interest in LightGBM and for submitting a patch.

Please let us know if you need help addressing the errors seen in CI jobs... this change appears to break the use of OpenMP with AppleClang on macOS.

For example, https://dev.azure.com/lightgbm-ci/lightgbm-ci/_build/results?buildId=15395&view=logs&j=31d31d6b-9897-5af8-16dd-840b6b6b4fac&t=83900217-b42d-5232-5e45-a25e1385a5c6

-- The C compiler identification is AppleClang 11.0.3.11030032
-- The CXX compiler identification is AppleClang 11.0.3.11030032
...
-- Could NOT find OpenMP_C (missing: OpenMP_C_FLAGS OpenMP_C_LIB_NAMES) 
-- Could NOT find OpenMP_CXX (missing: OpenMP_CXX_FLAGS OpenMP_CXX_LIB_NAMES) 
-- Could NOT find OpenMP (missing: OpenMP_C_FOUND OpenMP_CXX_FOUND)
...
[100%] Linking CXX shared library /Users/runner/work/1/s/lib_lightgbm.so
Undefined symbols for architecture x86_64:
  "___kmpc_barrier", referenced from:
      _.omp_outlined..22 in linear_tree_learner.cpp.o
      _.omp_outlined..94 in linear_tree_learner.cpp.o
...
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[uilianries]

I'll re-visit it during this week. Sorry my delay.

[jmoralez]

Just a suggestion. I don't think STREQUAL works in the way you're trying to use it here, you should probably change it to something like if(CMAKE_CXX_COMPILER_ID MATCHES "Clang"). reference.

[jameslamb]

I've updated this branch to latest master and pushed the changes @jmoralez suggested. Let's see what happens.

I still don't understand the need for this PR though. LightGBM has 10+ CI jobs running on Ubuntu which compile the library with clang and OpenMP support.

LightGBM/.vsts-ci.yml

Lines 82 to 85 in 7435cd8

	- job: Linux_latest
	###########################################
	variables:
	COMPILER: clang-17

For example, see the logs from a recent one on master:

...
Selecting previously unselected package libgomp1:amd64.
Preparing to unpack .../33-libgomp1_12.3.0-1ubuntu1~22.04_amd64.deb ...
Unpacking libgomp1:amd64 (12.3.0-1ubuntu1~22.04) ...
...
Setting up libgomp1:amd64 (12.3.0-1ubuntu1~22.04) ...
...
- The C compiler identification is Clang 17.0.6
-- The CXX compiler identification is Clang 17.0.6
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/clang-17 - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/clang++-17 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Building library with integrated OpenCL components
-- Found OpenMP_C: -fopenmp=libomp  
-- Found OpenMP_CXX: -fopenmp=libomp  
-- Found OpenMP: TRUE   
-- Populated OpenCL Headers
...

(build link)

It sure looks to me like:

• clang-17 was used as the compiler
• OpenMP (libgomp) and its headers were found
• compilation succeeded
• the project's Python tests (which use that shared library lib_lightgbm.so) all succeeded

@uilianries can you give us some more information about your setup? I'd like to try to understand exactly why this PR's patch is required for your situation, so we can maybe modify LightGBM's CI to catch such situations in the future.

[uilianries]

@uilianries can you give us some more information about your setup? I'd like to try to understand exactly why this PR's patch is required for your situation, so we can maybe modify LightGBM's CI to catch such situations in the future.

@jameslamb If it works with Ubuntu and Clang 17, so I'm totally fine with it. It was just try to unblock in case building with Clang in Linux.

[jameslamb]

unblock in case building with Clang in Linux

Thanks but this is what I'm asking about. I haven't yet been able to reproduce the error you faced in conan-io/conan-center-index#18759.

Are you sure it was this change and not, say, that you needed to install OpenMP?

[uilianries]

Are you sure it was this change and not, say, that you needed to install OpenMP?

@jameslamb To install no, to link OpenMP with LightGBM. It's a patch applied until the version 4.1.0. The version 4.2.0 is fine and does not need it. When trying to build with Clang + Linux + OpenMP + 4.1.0 it fails because that condition does not match, so, there is no target link target_link_libraries(lightgbm_objs PUBLIC OpenMP::OpenMP_CXX)

[jameslamb]

It's a patch applied until the version 4.1.0. The version 4.2.0 is fine and does not need it.

hmmmmm that's even more confusing

If you look in the git blame view (link), you'll see that this particular condition has been in LightGBM's CMakeLists.txt for 3+ years.

v4.1.0 came out just 5 months ago (link) and v4.2.0 just 2 months ago (link)

It should be the case that this is enough to build LightGBM with OpenMP:

LightGBM/CMakeLists.txt

Lines 156 to 176 in 0a9a6bb

	if(USE_OPENMP)
	if(APPLE)
	find_package(OpenMP)
	if(NOT OpenMP_FOUND)
	# libomp 15.0+ from brew is keg-only, so have to search in other locations.
	# See https://github.com/Homebrew/homebrew-core/issues/112107#issuecomment-1278042927.
	execute_process(COMMAND brew --prefix libomp
	OUTPUT_VARIABLE HOMEBREW_LIBOMP_PREFIX
	OUTPUT_STRIP_TRAILING_WHITESPACE)
	set(OpenMP_C_FLAGS "-Xpreprocessor -fopenmp -I${HOMEBREW_LIBOMP_PREFIX}/include")
	set(OpenMP_CXX_FLAGS "-Xpreprocessor -fopenmp -I${HOMEBREW_LIBOMP_PREFIX}/include")
	set(OpenMP_C_LIB_NAMES omp)
	set(OpenMP_CXX_LIB_NAMES omp)
	set(OpenMP_omp_LIBRARY ${HOMEBREW_LIBOMP_PREFIX}/lib/libomp.dylib)
	find_package(OpenMP REQUIRED)
	endif()
	else()
	find_package(OpenMP REQUIRED)
	endif()
	set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}")
	endif()

And I believe it's what is making all of those different builds (across multiple compilers, compiler versions, and operating systems) in this project succeed.

Reading through https://cmake.org/cmake/help/latest/command/target_link_libraries.html, I'm concerned that maybe the change in this PR will conflict with that code block I linked to above. I'll restart the failing CI jobs so we can get a more complete picture.

Maybe your v4.1.0 build had OpenMP installed in a non-standard place? Or maybe it was using a very old version of CMake?

[jameslamb]

I'm closing this due to lack of response. If you have time and interest in the future in describing the problem specifically and working with us to fix it, we'd be happy to consider this proposal again.