PointCloudLibrary / pcl

Point Cloud Library (PCL)
https://pointclouds.org/
Other
9.93k stars 4.61k forks source link

AVX2 makes PCL crash #5806

Open Vakarian15 opened 1 year ago

Vakarian15 commented 1 year ago

Describe the bug

I am trying to downsample a pcl::PointXYZRGBNormal PointCloud using a VoxelGrid filter. AVX2 makes my program crash, I tried it with 1.12.0 and 1.13.1, both of them have the same problem.

To Reproduce

int main()
{
    pcl::PointCloud<pcl::PointXYZRGBNormal>::Ptr cloud(new pcl::PointCloud<pcl::PointXYZRGBNormal>());
    pcl::PointCloud<pcl::PointXYZRGBNormal>::Ptr cloud_filtered(new pcl::PointCloud<pcl::PointXYZRGBNormal>());

    // Fill in the cloud data
    pcl::PCDReader reader;
    reader.read("test.pcd", *cloud); 

    // Create the filtering object
    pcl::VoxelGrid<pcl::PointXYZRGBNormal> sor;
    sor.setInputCloud(cloud);
    sor.setLeafSize(0.01f, 0.01f, 0.01f);
    sor.filter(*cloud_filtered);
    return (0);
}

If I disable AVX2, it works. If I replace pcl::PointCloud<pcl::PointXYZRGBNormal> with pcl::PCLPointCloud2, it works. So I'm not sure it is a problem of pcl::PointXYZRGBNormal or pcl::VoxelGrid. Here is the sample project including the dataset: PCLTest.zip

Screenshots/Code snippets Stack track image

image

Your Environment (please complete the following information):

Additional context

In 1.13.1, both pcl::PointXYZRGBNormal and pcl::VoxelGrid have PCL_MAKE_ALIGNED_OPERATOR_NEW macro. So I have no idea what the problem is. Thanks in advance.

larshg commented 1 year ago

@Vakarian15 can you try find the configure log file from VCPKG build of PCL - maybe we can see something in the log, whether PCL was build with AVX support or not. If its not and you enable it, in your project, it can fail with these errors.

Vakarian15 commented 1 year ago

@Vakarian15 can you try find the configure log file from VCPKG build of PCL - maybe we can see something in the log, whether PCL was build with AVX support or not. If its not and you enable it, in your project, it can fail with these errors.

@larshg Thank you for your swift response. Here is the configure log config-x64-windows-CMakeCache.txt.log It has PCL_ENABLE_AVX:BOOL=ON. Not sure whether this means it has AVX series support or only support AVX. But I still get the same error when I enable AVX instead of AVX2 in my project

larshg commented 1 year ago

That was the cmake cache and yes, the variable you refer to, is that it should test for AVX(2), but I can't find any "HAVE_AVX" or the like.

I was requesting the configuration output when running cmake. It should be like: C:\vcpkg\buildtrees\pcl\install-x64-windows-rel-out.log Or C:\vcpkg\buildtrees\pcl\configure-x64-windows-rel-out.log

Its example from the azure pipline, but hopefully you can find similiar :-)

larshg commented 1 year ago

I can find HAVE_SSE4_2_EXTENSIONS:INTERNAL=1 Which indicates you have at least sse 4.2.

Vakarian15 commented 1 year ago

Here are the files config-x64-windows-out.log install-x64-windows-rel-out.log And you are right, I cannot find AVX flags. I tired to create a overlay port, by adding -DPCL_ENABLE_AVX=ON to profile.cmake. Delete buildtrees/pcl folder and install pcl with the overlay port image And here is the new config log config-x64-windows-out.log -DPCL_ENABLE_AVX=ON was passed to cmake successfully. But the problem is not fixed. Could you please have a look? @larshg Is there something I did wrong?

larshg commented 1 year ago

Ahh, vcpkg have a patch for SSE https://github.com/microsoft/vcpkg/blob/master/ports/pcl/fix-check-sse.patch

But not for AVX and they don't parse default CXX settings, so it will never check for AVX instructions. You can try make a patch similiar to the SSE one and add this to the vcpkg portfile.

Vakarian15 commented 1 year ago

Ahh, vcpkg have a patch for SSE https://github.com/microsoft/vcpkg/blob/master/ports/pcl/fix-check-sse.patch

But not for AVX and they don't parse default CXX settings, so it will never check for AVX instructions. You can try make a patch similiar to the SSE one and add this to the vcpkg portfile.

I see, thanks for your time!

Vakarian15 commented 1 year ago

I made a patch for AVX Screenshot 2023-09-13 111549 Then add the patch to protfile.cmake Screenshot 2023-09-13 111637 Screenshot 2023-09-13 111659 Then remove pcl from vcpkg and reinstall with overlay ports And here is the new log file config-x64-windows-out.log It says "Performing Test HAVE_AVX2 - Success" in the log. But the problem is not fixed.

larshg commented 1 year ago

And you have enabled AVX in your project as well as copied new dlls etc?

Vakarian15 commented 1 year ago

Yes, I even copied the whole vcpkg\installed\x64-windows\bin folder. I tried allinone installer, it doesn't work. I also tired to build from source with -DPCL_ENABLE_AVX=ON, it doesn't work either.

Aposhian commented 1 year ago

I was able to get AVX2 to work by using only custom point types with EIGEN_ALIGN32 (and then making sure to define PCL_NO_PRECOMPILE). It would be nice to have preprocessor directives for PCL that made it easier to change max alignment, similar to how Eigen does it upstream.

Neumann-A commented 9 months ago

I find it funny that there are two connected issues so close together. I think #5805 is the problem. PCL is using EIGEN_ALIGN16 which probably does not work with /arch:AVX2. I would say: Let the compiler decide the alignment or make it dependent on the target architecture.

mvieth commented 8 months ago

Hi, thank you for your suggestions, but I tested and I see no reason to assume that EIGEN_ALIGN16 causes the problem:

Same as Lars, I think the problem is likely that vcpkg built PCL without AVX/AVX2, so the precompiled classes use a "vanilla" malloc instead of Eigen's custom aligned malloc. When the user code is compiled with AVX/AVX2, Eigen's aligned_free is used (not a "vanilla" free). The mismatch between the "vanilla" malloc and Eigen's aligned_free causes the problem. When using PCL_NO_PRECOMPILE, the PCL code is compiled together with the user code, and all flags and architecture options are consistent. Adding a patch for AVX (like the patch that vcpkg has for SSE) is a good first step, but I believe it did not take effect yet because there is a second check for "${CMAKE_CXX_FLAGS}" STREQUAL "${CMAKE_CXX_FLAGS_DEFAULT}" : https://github.com/PointCloudLibrary/pcl/blob/master/CMakeLists.txt#L150 So AVX and AVX2 are found, but the flags are not passed to CMAKE_CXX_FLAGS. I will make some more tests, and then create a PR for PCL, and perhaps also submit a patch to vcpkg.

Aposhian commented 8 months ago

As another data point, I encountered this bug on Linux with installing PCL binaries from apt, so it isn't just vcpkg. I haven't tried using the built-in EIGEN_ALIGN16 while simply specifying PCL_NO_PRECOMPILE.