-
Notifications
You must be signed in to change notification settings - Fork 621
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
Fix ARM fixed point builds with CMake #292
base: main
Are you sure you want to change the base?
Conversation
There's been many fixes to CMake in the last few months. Are there still issues that need addressing? |
aacebcd
to
7e81f9f
Compare
7e81f9f
to
38336ef
Compare
I've rebased the branch and updated for the new dnn NEON code. It might be a good idea to rename the dotprod defines to match to avoid future confusion, but for now this fixes the immediate issue with CMake fixed-point builds. |
Can you explain what exactly breaks with the current code (worked fine in the tests I was running)? Also, a less invasive fix would be good since I don't know what kind of effect your patches could have on autotools/meson. |
CMake currently supports the NEON intrinsic code, but not the NEON assembly code in celt/arm/celt_pitch_xcorr_arm.s . From what I can tell, the OPUS_ARM_NEON defines are intended to show assembly support and the OPUS_ARM_NEON_INTR defines are intended to show intrinsic support, but both of them seem to have been used interchangeably. Autotools and meson appear to support both, so shouldn't be too affected by this. |
Still, the CMake tests are current passing on ARM, so I'm trying to understand what case the change is meant to fix |
The issue is only noticeable when fixed point is enabled. Floating point builds are unaffected because no assembly sources are built in that configuration. |
Can you provide steps to reproduce? |
CMake builds previously defined both
OPUS_ARM_MAY_HAVE_NEON
andOPUS_ARM_MAY_HAVE_NEON_INTR
despite the fact that only the intrinsics-based functions are supported.