SLADE icon indicating copy to clipboard operation
SLADE copied to clipboard

Fixed aarch64 building

Open dleslie opened this issue 3 years ago • 5 comments

  • 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.

dleslie avatar Sep 13 '20 02:09 dleslie

Can that removed x86 SSE flag for dumb library be added back in with proper checks for x86 architecture?

Cacodemon345 avatar Sep 15 '20 19:09 Cacodemon345

@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?

dleslie avatar Sep 15 '20 23:09 dleslie

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.

Cacodemon345 avatar Sep 16 '20 04:09 Cacodemon345

@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

sirjuddington avatar Mar 09 '22 00:03 sirjuddington

The only issue here is that this PR disables SSE instructions for libdumb unconditionally on all platforms.

Cacodemon345 avatar Mar 09 '22 04:03 Cacodemon345

@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()

Blzut3 avatar Jun 24 '23 04:06 Blzut3

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?

Blzut3 avatar Jun 28 '23 04:06 Blzut3

Superseded by merged PR #1544.

Blzut3 avatar Jul 08 '23 19:07 Blzut3