SLADE
SLADE copied to clipboard
Fixed aarch64 building
- Removed baked-in -msse call
- Added explicit casts for some compiler warnings
Fixes #1206
And now it works fine on my Pinebook Pro and my RPi4.
Can that removed x86 SSE flag for dumb library be added back in with proper checks for x86 architecture?
@Cacodemon345 Yep! But it might be clunky, I think CMake needs this assistance lib to do it "properly": https://github.com/tunabrain/tungsten/blob/master/cmake/OptimizeForArchitecture.cmake
Would you be happy with just enabling it for x86_64?
It is easy to guard the SSE flag behind a simple check of CMAKE_SYSTEM_PROCESSOR. Check if the CPU is x86 and then switch on the flag.
Of course, enabling it just for x86_64 wouldn't be of any benefit, since SLADE still provides 32-bit versions.
@dleslie @Cacodemon345 is this one still relevant? I had left it because it seemed like it still may have needed changes judging by the discussion
The only issue here is that this PR disables SSE instructions for libdumb unconditionally on all platforms.
@sirjuddington Now that Ubuntu 18.04 has reached the end of standard support meaning i386 Ubuntu is no longer supported. In its place with the increase in interest in running ARM workstations I'm interested in providing aarch64 binaries for Slade. Thus I'm interested in seeing this merged.
Adding a check against CMAKE_SYSTEM_PROCESSOR could work, although since there's often not a lot of care to set this value correctly (i.e. building 32-bit binaries on 64-bit hosts), it's probably better to just do a check_c_compiler_flag call and let the compiler tell you if sse is supported on the target. Pretty marginal difference though.
@Cacodemon345 here's my suggested solution:
include(CheckCCompilerFlag)
check_c_compiler_flag(-msse HAVE_SSE)
if(HAVE_SSE)
add_compile_options(-msse)
add_definitions(-D_USE_SSE)
endif()
After thinking about the issue for awhile I realized there was a clever work around to compile Slade for ARM without modifying the source code: Wrap GCC and remove the flags.
#!/bin/bash
declare -a Args=("$@")
for Arg in "${!Args[@]}"; do
if [[ ${Args[$Arg]} == '-msse' || ${Args[$Arg]} == '-D_USE_SSE' ]]; then
unset "Args[$Arg]"
fi
done
exec /usr/bin/gcc "${Args[@]}"
Also adding -Wno-error=narrowing to the cxxflags to avoid the other issue this PR fixes. So my Apt repo now publishes an arm64 build.
Not to be impatient, but since I'm adding this info and making another comment anyway: @dleslie do you intend to update your PR or should I create a new one/take over?
Superseded by merged PR #1544.