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

use neon on armv7 #315

Closed
wants to merge 1 commit into from
Closed

use neon on armv7 #315

wants to merge 1 commit into from

Conversation

kali
Copy link
Contributor

@kali kali commented Oct 17, 2016

enable neon support on armv7 platforms.

An alternative was to disable neon code altogether, but part of the code already assumes it's there. I know most devices will have it, but I'm not sure whether or there exist neon-less armv7 around. As far as I can tell, all phones and mainstream stuff should have it.

see also #312

@briansmith
Copy link
Owner

Thanks for the PR.

Is your goal to fix the iOS build? If not, what is the goal of the PR?

It is the case that NEON is typically available on the most common ARMv7 platforms, particularly Android and iOS phones. But, in some cases we avoid using NEON instructions because the chip's NEON support is broken; see crypto/cpu-arm-linux.c for details. So, depending on your goal, this might be the best way of doing things, or there might be a better way.

@kali
Copy link
Contributor Author

kali commented Oct 17, 2016

I'm mostly interested in getting hyper/rustls/ring work on phones, both ios and android. I don't need extraordinary performance, so a non-neon build is perfectly ok for my need.

I may be able to find a few hours to spend on this here and there, but I'll need some guidance: I now understand that using neon without runtime check is relatively dangerous and not mergeable as is (even if I could use it in my specific use-case). Should I try to make the arm build NEON-less as a first step instead ? I tried defining OPENSSL_NO_ASM on arm, it did not fix all the compilation issue, but it may be safer to start that way.

@briansmith
Copy link
Owner

I'm mostly interested in getting hyper/rustls/ring work on phones, both ios and android. I don't need extraordinary performance, so a non-neon build is perfectly ok for my need.

OK. ring is already building successfully on Android, right? So, are you trying to fix the iOS build now? Or, are there problems on Android too?

Should I try to make the arm build NEON-less as a first step instead?

If you are just trying to get iOS to work, then I'll accept pretty much any reasonable patch to fix the iOS build. The best patch would enable NEON for all iOS builds, but if we need to disable some asm code for iOS temporarily, I'm happy to do that; we can then file a follow-up issue to re-enable any assembly-language optimizations we temporarily disable.

I tried defining OPENSSL_NO_ASM on arm, it did not fix all the compilation issue, but it may be safer to start that way.

The OPENSSL_NO_ASM stuff is left over from BoringSSL and isn't supported in ring.

@kali
Copy link
Contributor Author

kali commented Oct 17, 2016

ok. No, nothing problematic on android so far. I built ring successfully with arm and armv7 targets. I will try x86 soon, because I need the simulator/emulator, but I don't expect neon to show up there.

So yeah, I only have issues with iOS. My understanding is that all armv7 iOS have NEON, and I could not find reports of iOS devices with a broken one, so this patch may actually do the right thing in the end :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.126% when pulling 074d84e on kali:master into 4102ae7 on briansmith:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.126% when pulling 074d84e on kali:master into 4102ae7 on briansmith:master.

@briansmith
Copy link
Owner

ok. No, nothing problematic on android so far. I built ring successfully with arm and armv7 targets. I will try x86 soon, because I need the simulator/emulator, but I don't expect neon to show up there.

So yeah, I only have issues with iOS. My understanding is that all armv7 iOS have NEON, and I could not find reports of iOS devices with a broken one, so this patch may actually do the right thing in the end :)

I believe that we want to support Android on ARMv7 devices without (working) NEON. If your changes enable iOS builds to succeed, thencan we change the patch to add -arch $(TARGET_ARCH_BASE) -mfpu=neon only when the target ARCH is ARMv7 and the target OS is iOS?

@kali
Copy link
Contributor Author

kali commented Oct 17, 2016

The change I'm proposing is inside a ifeq ($(TARGET_SYS),ios) section so I think it is already exactly what you describe. And it does fix the iOS builds for armv7 (and armv7s). Other builds I could check look non affected, as we should expect.

@briansmith
Copy link
Owner

The change I'm proposing is inside a ifeq ($(TARGET_SYS),ios)

Ah, got it. Thanks! That wasn't in the diff context I saw and I didn't realize we'd already landed some ios changes.

@briansmith
Copy link
Owner

Thanks a lot. This landed in master as 4efb6f5.

@briansmith briansmith closed this Oct 17, 2016
@briansmith
Copy link
Owner

Are you able to run the tests on iOS ARMv7 too? Does everything pass?

@kali
Copy link
Contributor Author

kali commented Oct 17, 2016

I don't think there is any easy way to run the tests on iOS. Maybe with a jailbroken device, or by re-packaging all the tests in a ad-hoc app.

@briansmith
Copy link
Owner

OK, thanks!

@kali
Copy link
Contributor Author

kali commented Oct 17, 2016

Thanks for merging. If I think of, or end up doing something smart for the tests, I'll make a PR.

@briansmith
Copy link
Owner

Thanks! That would be awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants