Check CMAKE_OSX_ARCHITECTURES and only define relevant optimizations #59
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
Author
|
CC @drowe67 to review proposed README contents. @barracuda156, feel free to give this a shot and see how it goes. |
Collaborator
Author
|
BTW, I compiled this PR into codec2/freedv-gui and it appears I can still decode the example 2020 WAV file on macOS. Additionally, forcing the minimum macOS version to 10.5 doesn't seem to have affected x86_64 and ARM builds as the compiler automatically chose higher versions as approrpiate: |
Owner
|
Thanks @tmiw - README looks fine. |
Collaborator
Author
Cool. Once @barracuda156 confirms that this PR works, it can be merged. |
@tmiw Let me try and update you. |
README.md
Outdated
|
|
||
| Experimental version of LPCNet that has been used to develop FreeDV 2020 - a HF radio Digital Voice mode for over the air experimentation with Neural Net speech coding. Possibly the first use of Neural Net speech coding in real world operation. | ||
|
|
||
| *Note: while this should be able to compile on any architecutre with a modern C compiler, this package only supports Intel and ARM architectures. Usage on others will likely exhibit extremely low performance and possibly have bugs.* |
There was a problem hiding this comment.
Just a typo, but:
architecutre
architecture
|
@tmiw Applying just changes to CMakeLists from eecf55d I still get an error due to Neon flags picked: (I have dropped Macports patches here.) |
Collaborator
Author
|
You'll need to add the following to the CMake call: |
@tmiw The problem is here: Line 131 in eecf55d We cannot assume that the build must be universal; in fact, it is possible only on G5 and 10.5, and even then building just for I.e. requiring |
Collaborator
Author
|
@barracuda156, how about now? |
Collaborator
Author
|
I haven't heard anything yet so I'm going to assume we're good. @drowe67, okay to merge? |
|
@tmiw Sorry, missed the message. I will check this tomorrow. Away from my PPC hardware atm, but I think this can work in Rosetta too. |
Owner
|
@tmiw - yes I'm OK if you want to merge. |
|
Building from eecf55d |
|
Or did I pick a wrong commit? 9f251e8 is the latest? I will try that now. UPD. Same error. In effect, in Rosetta the build ignores CMAKE_OSX_ARCHITECTURES altogether, forcing AVX: |
Collaborator
Author
|
In that commit, |
@tmiw We will not want to force universal build, to be honest. To begin with, it simply cannot work with GCC, regardless of the arch, since no GCC after gcc-4.2 supports universal builds. Logically, IMO, we just need to provide an option to override defaults via configure (i.e. avoiding manual hackery). Let defaults be whatever makes better sense, and if implementing logic to detect architecture is problematic or undesirable, then just provide a configure switch to make things work in a non-default case. In fact, something like |
Collaborator
Author
|
OK, I think it should take into account all current feedback now. Once I get a confirmation that it builds, I'll merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per previous PLT discussion, this PR reimplements #57 in a manner that should have lower impact:
CMAKE_OSX_ARCHITECTURES.CMAKE_OSX_ARCHITECTURESis set to x86_64 and arm64 unless specifically overridden at the command line (i.e.cmake -DCMAKE_OSX_ARCHITECTURES=ppc\;ppc64 ..)*_PRESENTis only enabled for the relevant architectures based on the contents ofCMAKE_OSX_ARCHITECTURES.