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.

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

Fix SSE flags for Clang / AppleClang and Cross Compile #2256

Closed
svenevs opened this issue Mar 19, 2018 · 5 comments
Closed

Fix SSE flags for Clang / AppleClang and Cross Compile #2256

svenevs opened this issue Mar 19, 2018 · 5 comments
Assignees
Milestone

Comments

@svenevs
Copy link
Contributor

svenevs commented Mar 19, 2018

  1. Addition of -march=native is currently skipped for all clang.
  2. Cross compiling should never include -march=native for any compiler, currently only excluded for GCC.

Will fix after #2100. (Assign to: @svenevs) 😄

@SergioRAgostinho SergioRAgostinho added the needs: pr merge Specify why not closed/merged yet label Mar 19, 2018
@SergioRAgostinho SergioRAgostinho added this to the pcl-1.9.0 milestone Aug 26, 2018
@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented Sep 6, 2018

@svenevs You can fire up the "oven" for this one. #2100 should be merged in the next few days. 👍

@svenevs
Copy link
Contributor Author

svenevs commented Sep 7, 2018

Oven is at temperature ;)

@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented Sep 7, 2018

I went ahead of myself and actually submitted the PR #2416 as you can see 😅 . Can you review it (last commit only) just to guaranteed I covered all points you mentioned in this Issue?

@svenevs
Copy link
Contributor Author

svenevs commented Sep 7, 2018

Ok I'll take a closer look this weekend when I'm back at a Linux box. From the other PR it sounded like you all are pushing for more modern cmake. Generally speaking, tinkering with CMAKE_CXX_FLAGS is discouraged in favor of doing everything target based (target_compile_{features,options}). It keeps things cleaner / makes the targets easier to export assuming you're doing PUBLIC|PRIVATE|INTERFACE appropriately.

You can also then safely use the CheckCXXCompilerFlag module (still guarded underneath checks for cross compiling in this specific case). That makes it so you don't need to worry about clang vs AppleClang vs gcc. You could also then check -xHost for Intel without actually needing to check the compiler ids.

Anyway I'll provide feedback on the PR validating when I can check on both Linux and OSX this weekend. I don't really have time to help with a more thorough removal of CMAKE_CXX_FLAGS but I can definitely check the PR adds the flags for clang so they get in there for what sounds like an imminent release 😄

@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented Sep 7, 2018

Ok I'll take a closer look this weekend when I'm back at a Linux box. From the other PR it sounded like you all are pushing for more modern cmake.

Slowly. The change was still driven by a problematic bug with the flags.

It keeps things cleaner / makes the targets easier to export assuming you're doing PUBLIC|PRIVATE|INTERFACE appropriately.

It isn't being done appropriately. The CMake definitely needs revamping but there are more important (project level) short terms goals.

Anyway I'll provide feedback on the PR validating when I can check on both Linux and OSX this weekend. I don't really have time to help with a more thorough removal of CMAKE_CXX_FLAGS but I can definitely check the PR adds the flags for clang so they get in there for what sounds like an imminent release smile.

I tried OS X with Apple's Clang already. I assume Linux will work ok as well but give it a try if you get the chance. There is indeed an imminent release. We've been working over the past months on different items to speed up our release process. Hopefully the changes in our release cycle will be noteworthy.

@SergioRAgostinho SergioRAgostinho removed the needs: pr merge Specify why not closed/merged yet label Sep 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants