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

[cmake] Optimizations for ARM #10397

Merged
merged 6 commits into from Sep 6, 2016

Conversation

@fetzerch
Copy link
Member

commented Sep 4, 2016

A couple of (mostly) ARM related fixes. Kudos to @mapfau (most of the things popped up in his odroid work).

See commit messages for details.

Ping: @wsnipex, @hudokkow

@mapfau: mind testing this again?

@@ -11,6 +11,11 @@ else()
elseif(CPU MATCHES "i.86")
set(ARCH i486-linux)
add_options(CXX ALL_BUILDS "-msse")
elseif(CPU STREQUAL arm)

This comment has been minimized.

Copy link
@wsnipex

wsnipex Sep 4, 2016

Member

MATCHES, cpu will hardly ever be exactly "arm".

elseif(CPU STREQUAL arm)
set(NEON True)
set(ARCH arm)
elseif(CPU STREQUAL aarch64)

This comment has been minimized.

Copy link
@wsnipex

wsnipex Sep 4, 2016

Member

please also check for "arm64" here

@@ -18,6 +18,7 @@ else()
set(ARCH arm-linux-gnueabihf)

This comment has been minimized.

Copy link
@wsnipex

wsnipex Sep 4, 2016

Member

NEON must always be off for rpi1

This comment has been minimized.

Copy link
@fetzerch

fetzerch Sep 4, 2016

Author Member

NEON is not set to true, so the option will default to off. do you want me to add an additional check in the common ArchSetup.cmake file? Not sure if that's necessary, cause if someone turns that open on, he likely knows what he's doing.

This comment has been minimized.

Copy link
@wsnipex

wsnipex Sep 4, 2016

Member

Never underestimate some users ability to turn every possible option on ;)
I'd simply force it off here, to be on the safe side

@@ -18,6 +18,7 @@ else()
set(ARCH arm-linux-gnueabihf)
elseif(CPU MATCHES "cortex-a7")
set(ARCH arm-linux-gnueabihf)
set(NEON True)
else()

This comment has been minimized.

Copy link
@wsnipex

wsnipex Sep 4, 2016

Member

can you also add RPI3? should be cortex-a53, correct @popcornmix?

@wsnipex

This comment has been minimized.

Copy link
Member

commented Sep 4, 2016

Nice.
while you are at it, can you remove the dependency on a Toolchain file for Rbpi? There are users out there that build directly on the rpi.

@fetzerch fetzerch force-pushed the fetzerch:cmake_arm branch from 8c4607c to 8442c84 Sep 4, 2016
@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2016

thx, done

@fetzerch fetzerch force-pushed the fetzerch:cmake_arm branch from ab7463d to e004dd0 Sep 4, 2016
@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2016

jenkins build this please

@fetzerch fetzerch force-pushed the fetzerch:cmake_arm branch from e004dd0 to a0acdaf Sep 5, 2016
@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2016

  • Disabled neon also for x86 and x86_64 (linux and osx)
  • AML only for android and linux (was incorrectly set to optional dep on osx, or ios)
  • Fixed a missing include for atoi on android, not sure if that is caused by NEON flags

jenkins build this please

set(NEON True)
elseif(CPU MATCHES aarch64 OR CPU MATCHES arm64)
set(ARCH aarch64)
set(NEON False)

This comment has been minimized.

Copy link
@wsnipex

wsnipex Sep 5, 2016

Member

should be True for aarch64

This comment has been minimized.

Copy link
@stefansaraev

stefansaraev Sep 5, 2016

Contributor

nope. neon is always present on arch64 and -mfpu=neon is not valid gcc opt.

fetzerch added 6 commits Sep 4, 2016
- Load includes before setting options. Otherwise CORE_SYSTEM_NAME
  cannot be used.
- Variable is named CORE_SYSTEM_NAME instead CORE_SYSTEM_TYPE.
There is some hardware that uses AML on Linux (ODROID-C1).
Allows to compile on arm based targets.
This enables to build natively on the Raspberry Pi.
@fetzerch fetzerch force-pushed the fetzerch:cmake_arm branch from a0acdaf to 218ea09 Sep 5, 2016
@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2016

  • Option overrides in platform ArchSetup.cmake files must be cached vars since they are now set before the option is defined. This is due to the moved includes in the first commit.
  • iOS/AppleClang doesn't support neon flags

jenkins build this please

@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2016

@wsnipex good to go?

@wsnipex wsnipex merged commit e8ed187 into xbmc:master Sep 6, 2016
2 of 3 checks passed
2 of 3 checks passed
jenkins.kodi.tv You are a failure. Fix the code and try again......
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fetzerch fetzerch deleted the fetzerch:cmake_arm branch Sep 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.