Default enable SSE only if compiler supports it

[sloretz]

This fixes an issue seen in ROS Noetic where SSE is default enabled on
an aarch64 system. CMake seems to think the host supports SSE, but the
compiler does not.

I don't know the root cause of the issue. Maybe it's because the ROS 1 buildfarm uses qemu on x86 systems, and the method CMake uses to determine SSE support shows that the machine beneath qemu supports it?

Fixes #513

---

This change is 

[SeanCurtis-TRI]

+@jamiesnape Could you take a once over on this one? It makes sense to me: ultiamtely, it distinguishes between what the *host* reports vs the *compiler*.

Reviewed 1 of 1 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jamiesnape)

[sherm1]

Also with a question below. Would be great to get a quick look from @jamiesnape -- just a small change here.

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jamiesnape and @sloretz)

---

CMakeLists.txt, line 115 at r1 (raw file):

else()
  # Unset SSE_FLAGS so it doesn't get used in subdirectories
  set(SSE_FLAGS "")

Just curious -- @sloretz why unset this particular variable? Does it have some global meaning that other local variables like FCL_TARGET_SUPPORT_X64_SSE do not?

[SeanCurtis-TRI]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jamiesnape and @sloretz)

---

CMakeLists.txt, line 115 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Just curious -- @sloretz why unset this particular variable? Does it have some global meaning that other local variables like FCL_TARGET_SUPPORT_X64_SSE do not?

That's funny. I had the same question, and then I realized he sets those flags based on host to perform a compiler test. The result of that test is stored in the _compiler_supports_sse variable. If we determine that the compiler doesn't support sse, we then need to clear that variable so it doesn't pollute, as the comment says, subdirectories CMakeLists.txt contexts.

I spent a brief moment considering if a different articulation of the logic would eliminate the need to clear these flags again, but decided there probably isn't (or, at least, not obviously). It's rendered somewhat confusing because there are a numb of SSE related symbols all churning around.

[SeanCurtis-TRI]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jamiesnape)

[SeanCurtis-TRI]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jamiesnape)

[jamiesnape]

This code is getting really ugly (not the fault of he author of this PR) and I wonder whether a library should ever be setting SSE flags, but aside from my comment the code is an improvement, so

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sloretz)

---

CMakeLists.txt, line 115 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Just curious -- @sloretz why unset this particular variable? Does it have some global meaning that other local variables like FCL_TARGET_SUPPORT_X64_SSE do not?

Looks like it is used in src/CMakeLists.txt. Not sure why we are setting it globally and per-target, however. I think I would wrap the usage there with if(FCL_USE_X64_SSE) ... endif() if it is not already set.

[sherm1]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sloretz)

---

CMakeLists.txt, line 115 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Looks like it is used in src/CMakeLists.txt. Not sure why we are setting it globally and per-target, however. I think I would wrap the usage there with if(FCL_USE_X64_SSE) ... endif() if it is not already set.

Oh, I see. A cleaner alternative might be not to use SSE_FLAGS here to hold the proposed flags -- how about using a local variable like _proposed_sse_flags and then set SSE_FLAGS only if the host and compiler tests both pass?

[sloretz]

Just curious -- @sloretz why unset this particular variable? Does it have some global meaning that other local variables like FCL_TARGET_SUPPORT_X64_SSE do not?

No special meaning other than it's used in src/CMakeLists.txt

Looks like it is used in src/CMakeLists.txt. Not sure why we are setting it globally and per-target, however. I think I would wrap the usage there with if(FCL_USE_X64_SSE) ... endif() if it is not already set.

Yup, link below:

fcl/src/CMakeLists.txt

Line 51 in 968fd7e

target_compile_options(${PROJECT_NAME} PUBLIC ${SSE_FLAGS})

Oh, I see. A cleaner alternative might be not to use SSE_FLAGS here to hold the proposed flags -- how about using a local variable like _proposed_sse_flags and then set SSE_FLAGS only if the host and compiler tests both pass?

I'm happy to make any changes to the logic there's consensus on.

[sherm1]

Thoughts @jamiesnape & @SeanCurtis-TRI ?

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sloretz)

[SeanCurtis-TRI]

I do like the idea of setting a local variable with the flags and only setting SSE_FLAGS if we mean to do so. The "set-and-unset-as-necessary" paradigm would be a bit brittle. That said, is it worth it? After all, it's one small rock on top of a pile of ugly rocks. The real thing would be to have someone at @jamiesnape's level actually go through and make this sane.

Conclusion: I'm content to take it as is (having passed CI) and defer polishing the rock to later.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sloretz)

[jamiesnape]

Let's merge then. This is not the biggest problem with the build system.

I probably will have some time at some point to redesign the build for modern CMake. Really the build on this project should be clean and simple (one library, some tests, and relatively few dependencies); it has just grown organically over a very long time trying to support old versions of CMake and many versions of third--party dependencies. That support is probably no longer needed, which will make cleaning up fairly quick.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sloretz)

[SeanCurtis-TRI]

Sounds like something to look forward to.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sloretz)

[rhaschke]

@sloretz, I'm going to prepare a new ROS release.