Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CMake] Add AVX for windows #4598

Merged
merged 4 commits into from
Apr 9, 2021
Merged

[CMake] Add AVX for windows #4598

merged 4 commits into from
Apr 9, 2021

Conversation

larshg
Copy link
Contributor

@larshg larshg commented Feb 7, 2021

No description provided.

@larshg larshg force-pushed the AddAVXDefinitions branch from 9a477f3 to 4e8efca Compare February 7, 2021 22:07
@larshg larshg added changelog: enhancement Meta-information for changelog generation module: cmake labels Feb 8, 2021
SunBlack
SunBlack reviewed Feb 9, 2021
View reviewed changes
cmake/pcl_find_avx.cmake Outdated Show resolved Hide resolved
cmake/pcl_find_avx.cmake Outdated Show resolved Hide resolved
@larshg larshg force-pushed the AddAVXDefinitions branch 2 times, most recently from dd8f6cd to 5658471 Compare February 9, 2021 08:50
SunBlack
SunBlack reviewed Feb 11, 2021
View reviewed changes
cmake/pcl_find_avx.cmake Outdated Show resolved Hide resolved
@Neumann-A
Copy link

General question: You know that playing around with such compiler flags does not belong into a CMakeLists.txt in good CMake?
(Meaning clean separation of toolchain/build script)

Maybe you should start renaming all those compiler flag options to PCL_TOOLCHAIN_ENABLE_AVX and move them out of the CMakeLists.txt into a separate cmake file (pcl_toolchain_setup.cmake)

@larshg
Copy link
Contributor Author

larshg commented Feb 18, 2021

@Neumann-A Not really - do you have a nice example in mind, to get inspiration from?

@larshg larshg force-pushed the AddAVXDefinitions branch from 417993e to d64a7da Compare March 3, 2021 10:48
@larshg larshg requested a review from mvieth March 23, 2021 15:48
@mvieth
Copy link
Member

mvieth commented Mar 23, 2021

Do I understand it correctly that e.g. arch:AVX defines __AVX__, so that something similar to SSE_DEFINITIONS is not necessary? If yes, a short comment describing that would be nice.
Does arch:AVX2 also imply arch:AVX? From your code I assume it does, but a comment about that would also be nice.
I wonder if it makes sense to extend this to GCC and Clang some time in the future. When not crosscompiling, the march=native flag is set, but when crosscompiling, AVX(2) is seemingly not used.

@SunBlack
Copy link
Contributor

Do I understand it correctly that e.g. arch:AVX defines __AVX__, so that something similar to SSE_DEFINITIONS is not necessary? If yes, a short comment describing that would be nice.
Does arch:AVX2 also imply arch:AVX? From your code I assume it does, but a comment about that would also be nice.

Yep, see here:

The AVX preprocessor symbol is defined when the /arch:AVX, /arch:AVX2 or /arch:AVX512 compiler option is specified. The AVX2 preprocessor symbol is defined when the /arch:AVX2 or /arch:AVX512 compiler option is specified. The AVX512F, AVX512CD, AVX512BW, AVX512DQ and AVX512VL preprocessor symbols are defined when the /arch:AVX512 compiler option is specified. For more information, see Predefined Macros. The /arch:AVX2 option was introduced in Visual Studio 2013 Update 2, version 12.0.34567.1. Limited support for /arch:AVX512 was added in Visual Studio 2017, and expanded in Visual Studio 2019.

I wonder if it makes sense to extend this to GCC and Clang some time in the future. When not crosscompiling, the march=native flag is set, but when crosscompiling, AVX(2) is seemingly not used.

May be indeed a good option to add AVX support of CLANG/GCC here to. It seems similar to MSVC (didn't found an official documentation)

@larshg
Copy link
Contributor Author

larshg commented Mar 28, 2021

Add inline comment.

@SunBlack
Copy link
Contributor

Wouldn't it make sense to add AVX for Linux too?

@larshg
Copy link
Contributor Author

larshg commented Apr 1, 2021

I'm not that well versed in Linux / cross compiling so I would prefer to just merge this one and one with better knowledge of linux can adjust it accordingly. Or someone make PR towards this PR.

larshg
larshg commented Apr 1, 2021
View reviewed changes
cmake/pcl_find_avx.cmake Outdated Show resolved Hide resolved
@SunBlack
Copy link
Contributor

SunBlack commented Apr 2, 2021

Okay, then this PR ready to merge from my point of view :-)

SunBlack
SunBlack reviewed Apr 3, 2021
View reviewed changes
cmake/pcl_find_avx.cmake Outdated Show resolved Hide resolved
@larshg @SunBlack
Update cmake/pcl_find_avx.cmake
e46094e 
Co-authored-by: SunBlack <SunBlack@users.noreply.github.com>
@kunaltyagi kunaltyagi mentioned this pull request Apr 7, 2021
Closed
@larshg larshg merged commit fcc948a into PointCloudLibrary:master Apr 9, 2021
@larshg larshg deleted the AddAVXDefinitions branch April 9, 2021 07:38
@larshg larshg mentioned this pull request Aug 5, 2021
Closed
@kaitou1419 kaitou1419 mentioned this pull request Nov 2, 2021
Closed
mvieth pushed a commit to mvieth/pcl that referenced this pull request Dec 27, 2021
@larshg @SunBlack @mvieth
[CMake] Add AVX for windows (PointCloudLibrary#4598)
4faf7be 
* Add AVX for windows
* Update cmake/pcl_find_avx.cmake

Co-authored-by: SunBlack <SunBlack@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SunBlack SunBlack SunBlack left review comments

@kunaltyagi kunaltyagi kunaltyagi approved these changes

@mvieth mvieth mvieth approved these changes

Assignees
No one assigned
Labels
changelog: enhancement Meta-information for changelog generation module: cmake
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@larshg @Neumann-A @mvieth @SunBlack @kunaltyagi