Closed Bug 716766 Opened 8 years ago Closed 6 years ago
[Skia] Add runtime detection of NEON support
We should modify skia so that we can compile with NEON enabled, but still run on devices that don't support it. There is an existing (badly rotted) patch at: http://code.google.com/p/chromium/issues/detail?id=67954 Anyone know enough NEON to attempt getting this working again?
It appears that our existing code for doing this on android-ish is http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/arm.cpp#127 . It would be cool if skia had a hook we could use to implemen NEON detection with our existing arm.h code, but something tells me that might not be a popular option upstream.
We can't quite do mandatory because we run on some devices that don't support NEON, but James has the details.
Assignee: milan → snorp
We need to check with the Skia guys to figure out what's best here. I don't know how the optional neon bits work there.
George, can you contact Skia people and ask them about this?
Upstream should have resolved this issue by now, but I will double check with them. Leaving the needinfo open for me as I don't want to address this until we've updated our Skia revision.
Comment on attachment 743454 [details] [diff] [review] use __ARM_HAVE_OPTIONAL_NEON_SUPPORT Review of attachment 743454 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to look at this now as it shouldn't take too long, and we've finally updated our skia rev. r- mainly because we modify generate_mozbuild.py now instead of moz.build.
Attachment #743454 - Flags: review?(gwright) → review-
Also submitted upstream @ https://codereview.chromium.org/189263015
Attachment #8387890 - Flags: review?(snorp)
The fonthost hunks in the change to moz.build are due to regenerating moz.build after I created this patch: https://hg.mozilla.org/mozilla-central/rev/0f8652814898 The rest should be fairly self explanatory.
I think -mfpu=neon allows the compiler to use neon instructions whenever it wants, which is bad (since we run on devices without neon). I am not sure, though.
No, it's correct, because those files are only ever used if Skia detects NEON is available at runtime.
Looks like armv6 is busted, otherwise I guess I'm ok with this.
Yep, looks like I just need to add the neon files in: if CONFIG['HAVE_ARM_NEON']: And it'll all work fine. Just doing a local build test now.
Use BUILD_ARM_NEON to protect the NEON files.
Attachment #8388811 - Flags: review?(snorp) → review+
Attachment #8387890 - Flags: review?(snorp) → review+
Updated try push with BUILD_ARM_NEON: https://tbpl.mozilla.org/?tree=Try&rev=c914b7631fbb
Relevant upstream bug: https://codereview.chromium.org/189263015 (so the patch won't need to be held locally)
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/a582eefb45b2 for various android crashes: https://tbpl.mozilla.org/php/getParsedLog.php?id=36159994&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=36160179&tree=Mozilla-Inbound
I don't think the crashes are related to this patch. snorp, can you give a second opinion?
Flags: needinfo?(gwright) → needinfo?(snorp)
(In reply to George Wright (:gw280) from comment #21) > I don't think the crashes are related to this patch. snorp, can you give a > second opinion? Yeah, doesn't look likely to me.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.