Skip to content

Detect alignment when pcl_find_sse is not called & fix detection of Eigen#6261

Merged
larshg merged 2 commits intoPointCloudLibrary:masterfrom
rhuitl:detect-alignment-without-pcl-find-sse
Apr 6, 2025
Merged

Detect alignment when pcl_find_sse is not called & fix detection of Eigen#6261
larshg merged 2 commits intoPointCloudLibrary:masterfrom
rhuitl:detect-alignment-without-pcl-find-sse

Conversation

@rhuitl
Copy link
Contributor

@rhuitl rhuitl commented Apr 3, 2025

As discussed on #6255, we are extracting the checks for posix_memalign() and _mm_malloc() from pcl_find_sse.cmake in order to always test for them. Previously, they were not checked if PCL is compiled with custom CMAKE_CXX_FLAGS.

The second commit fixes the test that sets PCL_USES_EIGEN_HANDMADE_ALIGNED_MALLOC for builds where Eigen is installed in a non-standard location (e.g. Conan, but possibly also Homebrew). The problem is that try_compile does not set include paths and preprocessor definitions unless CMAKE_TRY_COMPILE_CONFIGURATION is set (https://gitlab.kitware.com/cmake/cmake/-/issues/22414). This made the test fail at the #include <Eigen/Core> in https://github.com/PointCloudLibrary/pcl/blob/master/CMakeLists.txt#L454.

What worked for me is to add

set(CMAKE_TRY_COMPILE_CONFIGURATION ${CMAKE_BUILD_TYPE})

@rhuitl rhuitl force-pushed the detect-alignment-without-pcl-find-sse branch from 6de63d4 to 0d41f8c Compare April 3, 2025 14:20
Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please change check_cxx_source_runs to check_cxx_source_compiles for both checks (for reason described in #6255 (comment) )
  • There should be an easy way for the user to tell CMake to not perform the two check_cxx_source_runs/compiles checks. Previously, this was e.g. possible by setting PCL_ENABLE_SSE=OFF. I would suggest to wrap the checks in if(NOT DEFINED HAVE_MM_MALLOC) and if(NOT DEFINED HAVE_POSIX_MEMALIGN), respectively.

rhuitl added 2 commits April 4, 2025 13:16
…) is not used.

This ensures the methods are used when SSE is enabled by setting CMAKE_CXX_FLAGS.
@rhuitl
Copy link
Contributor Author

rhuitl commented Apr 4, 2025

Done! Before I push, one remark though: the test will be skipped when you run cmake -DHAVE_POSIX_MEMALIGN=1, but there is no way to skip it AND saying that it's unavailable. -DHAVE_POSIX_MEMALIGN=0 would skip the check, but

#if defined(MALLOC_ALIGNED)

would probably still evaluate to true.

@mvieth
Copy link
Member

mvieth commented Apr 4, 2025

Done! Before I push, one remark though: the test will be skipped when you run cmake -DHAVE_POSIX_MEMALIGN=1, but there is no way to skip it AND saying that it's unavailable. -DHAVE_POSIX_MEMALIGN=0 would skip the check, but

#if defined(MALLOC_ALIGNED)

would probably still evaluate to true.

I am not sure I follow:

#cmakedefine HAVE_POSIX_MEMALIGN

will be replaced by #define HAVE_POSIX_MEMALIGN if HAVE_POSIX_MEMALIGN is TRUE or ON or similar in CMake, otherwise /* #undef HAVE_POSIX_MEMALIGN */ . (see https://stackoverflow.com/questions/42719401/what-is-the-cmakedefine-preprocessor-directive , https://cmake.org/cmake/help/latest/command/configure_file.html#transformations )

check_cxx_source_compiles would also set HAVE_POSIX_MEMALIGN to false if the compilation fails (see https://cmake.org/cmake/help/latest/module/CheckCXXSourceCompiles.html ).

@rhuitl rhuitl force-pushed the detect-alignment-without-pcl-find-sse branch from 0d41f8c to 6163b2f Compare April 4, 2025 12:38
@rhuitl
Copy link
Contributor Author

rhuitl commented Apr 4, 2025

You are right, I forgot about #cmakedefine.

@mvieth mvieth added this to the pcl-1.15.1 milestone Apr 4, 2025
Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me 👍

@larshg larshg merged commit 575168b into PointCloudLibrary:master Apr 6, 2025
13 checks passed
@mvieth mvieth added the changelog: fix Meta-information for changelog generation label Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: fix Meta-information for changelog generation module: cmake

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants