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] fix sse detection #10372

Merged
merged 4 commits into from Sep 4, 2016
Merged

[cmake] fix sse detection #10372

merged 4 commits into from Sep 4, 2016

Conversation

wsnipex
Copy link
Member

@wsnipex wsnipex commented Aug 31, 2016

Fixes false SSE detection in some environments, e.g. buildroot for arm

@fetzerch @Memphiz @fritsch for sanity checks and testing

@wsnipex wsnipex added Type: Fix non-breaking change which fixes an issue CMake labels Aug 31, 2016
@wsnipex
Copy link
Member Author

wsnipex commented Aug 31, 2016

jenkins build this please

@fritsch
Copy link
Member

fritsch commented Aug 31, 2016

@wsnipex from what I see - and I don't see much in cmake - it seems you are doing the right thing, checking sse4.1 and so on. If the others are fine, we can test it in master. The none SSE4 cpus get less and less - barely not many ION's available anymore :-)

Only @MilhouseVH is still using one in a productive environment.

@wsnipex
Copy link
Member Author

wsnipex commented Aug 31, 2016

jenkins build this please

@MilhouseVH
Copy link
Contributor

I believe Hitcher is an ION lover too.

@@ -1,7 +1,9 @@
set(SOURCES CopyFrame.cpp)
if(HAVE_SSE4_1)

This comment was marked as spam.

@fetzerch
Copy link
Member

Looks good to me, thx!

@Memphiz
Copy link
Member

Memphiz commented Aug 31, 2016

Currently unable to check this out :( are you sure with those -mblah flags for clang?

@wsnipex
Copy link
Member Author

wsnipex commented Aug 31, 2016

didn't change the flags, I just added avx. Those I'm not sure about, but since sse* seems to be the same as on gcc, I guess they are the same for avx as well.

@wsnipex
Copy link
Member Author

wsnipex commented Sep 1, 2016

updated. We now enable all SSE versions <= 4.1 by default(if available).
@fritsch @FernetMenta : I've replaced all the SSE macros in AE, so its now possible to disable SSE use.
Disabling SSE altogether brings up an old issue we only observed on i386 before, so it seems we don't compile without it.
see http://pastebin.com/cq4aK60E

@wsnipex
Copy link
Member Author

wsnipex commented Sep 1, 2016

jenkins build this please

@wsnipex wsnipex force-pushed the cmake_sse branch 2 times, most recently from bdffffc to ffa3825 Compare September 1, 2016 10:07
@wsnipex
Copy link
Member Author

wsnipex commented Sep 1, 2016

jenkins build this please

@wsnipex
Copy link
Member Author

wsnipex commented Sep 1, 2016

jenkins build this please

@Memphiz
Copy link
Member

Memphiz commented Sep 4, 2016

@wsnipex looking at the jenkins output it looks ok'ish (for ios no extensions are found as expected). For OSX the output is as follows:

http://pastebin.com/kR10RFdv

Not sure if this is correct. I wonder how this works. It finds the extensions that are available on the build host - right? How to ensure that the target where this runs has the same abilities in the end?

Beside that i wonder why it says "-- Cross-Compiling: FALSE" on osx - maybe @fetzerch has an idea? (you know osx is a cross compile as it uses the sdk buildchain, not the host one...)

@Memphiz
Copy link
Member

Memphiz commented Sep 4, 2016

Beside that i i've given the kodi-20160901-906b6a8-cmake_sse-x86_64.dmg a shot (from sept. 1st) and it seems to work ok. Won't be able to test anything else for this PR - so OK from me.

@wsnipex
Copy link
Member Author

wsnipex commented Sep 4, 2016

yes, it checks the build host, same as before my change. The difference is that now you can disable sse extensions, which wasn't possible before. Currently only SSE(1) seems to be absolutely necessary to build(at least on linux), and we only compile with up to SSE2 in AudioEngine. I don't think there are any intel based Macs that don't support this.
The SSE4.1 based libary is linux only and is runtime checked

@fetzerch
Copy link
Member

fetzerch commented Sep 4, 2016

Beside that i wonder why it says "-- Cross-Compiling: FALSE" on osx - maybe @fetzerch has an idea?
(you know osx is a cross compile as it uses the sdk buildchain, not the host one...)

CMAKE_CROSSCOMPILE is just a variable that is set if the toolchain has CMAKE_SYSTEM_NAME set (as for IOS: https://github.com/xbmc/xbmc/blob/master/tools/depends/target/Toolchain.cmake.in#L18).
We could set it for OSX too, it just needs a few adaptions then. For example CMAKE_CROSSCOMPILE currently defines if unit testing should be enabled.

@wsnipex wsnipex merged commit 5873170 into xbmc:master Sep 4, 2016
@wsnipex wsnipex deleted the cmake_sse branch September 4, 2016 15:41
@MartijnKaijser MartijnKaijser added this to the Krypton 17.0-beta2 milestone Sep 6, 2016
@MartijnKaijser MartijnKaijser added this to the Krypton 17.0-beta2 milestone Sep 6, 2016
@FernetMenta
Copy link
Contributor

this broke cmake build with XCode target

CMake Error at /Users/DE213022/progs/src/xbmc/xbmc/rendering/CMakeLists.txt:8 (target_compile_options):
  Cannot specify compile options for target "rendering" which is not built by
  this project.

@Memphiz
Copy link
Member

Memphiz commented Sep 18, 2016

in addition - its fine when generating gnu make files - strange ...

fetzerch added a commit to fetzerch/xbmc that referenced this pull request Sep 19, 2016
Fixes errors such as:
CMake Error at xbmc/utils/CMakeLists.txt:194
(target_compile_options):
  Cannot specify compile options for target "utils" which is not built
  by this project.

Introduced by xbmc#10372.

For Xcode (and VS) core_add_library adds the files to compile to the
libkodi target. Therefore options have to be set for CORE_LIBRARY
which is set by core_add_library and expands to the correct library
depending on the generator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants