583958 - Add NEON detection to SSE.h?

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Makoto Kato [:m_kato]]

By bug 513422, MMX/SSE detection for x86/x86_64 is added.

I need NEON detection for ARM processor such as Cortex-A8/SnapDragon.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: patch

Comment 2

[Justin Lebar (not reading bugmail)]

See also bug 585708.

Comment 3

[Justin Lebar (not reading bugmail)]

I would very much like to base this patch on top of bug 585708's patch, since SSE.h is not particularly usable in its current state.

Comment 4

[Makoto Kato [:m_kato]]

Justin, what status of bug 585708?  I need this for NEON.

Comment 5

[Justin Lebar (not reading bugmail)]

bug 585708 just needs a review.  dbaron is currently tagged, although there was talk at some point of asking Vlad to review it; I forget what came of that.

Do you know if we compile for ARM with -mneon (or it's somehow presumed)?  If not, we certainly want to add this to bug 585708 so you can separate out your NEON code into separate files that get compiled with -mneon.

If on the other hand we are presuming that NEON exists, gcc is free to use NEON instructions anywhere, and you don't need to guard on has_neon().

Comment 6

[Makoto Kato [:m_kato]]

(In reply to comment #5)
> bug 585708 just needs a review.  dbaron is currently tagged, although there was
> talk at some point of asking Vlad to review it; I forget what came of that.
> 
> Do you know if we compile for ARM with -mneon (or it's somehow presumed)?  If
> not, we certainly want to add this to bug 585708 so you can separate out your
> NEON code into separate files that get compiled with -mneon.

No, Android and Maemo doesn't use -mfpu=neon.  This fix is needed to detect whether CPU supports NEON.  Also, Since I am writing all NEON optimization by inline assembler (not use built-in C function), it is unnecessary to separate file.

Comment 7

[Justin Lebar (not reading bugmail)]

Ah, inline assembly, not intrinsics.  Sure.

In that case, I'd be ok with this patch going ahead as-is, although I'd really prefer just to fix SSE.h per bug 585708 before adding more code which relies on the current interface, which doesn't work for x86 intrinsics.

(Off topic, I'm kind of surprised that we target phones which lack NEON.)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FYI, I'm waiting to review this until after bug 585708 lands.

Comment 9

[Justin Lebar (not reading bugmail)]

And FYI, I'm waiting to land bug 585708 until the patches in its dependency tree get reviewed.

Comment 10

[Makoto Kato [:m_kato]]

Attachment: patch v2

Comment 11

[Justin Lebar (not reading bugmail)]

Comment on attachment 514163 [details] [diff] [review]
patch v2

>+#if defined(ANDROID)
>+static bool
>+has_neon()
>+{
>+  FILE *f = fopen("/proc/cpuinfo", "r");
>+  if (f) {
>+    char buf[1024];
>+
>+    fread(buf, sizeof(char), 1024, f);
>+    fclose(f);
>+    if (strstr(buf, "neon")) {
>+      return true;
>+    }
>+  }
>+  return false;
>+}

How do you know buf is null-terminated?

Comment 13

[Timothy B. Terriberry (:derf)]

Attachment: Generic ARM/NEON detection

I also needed this, and wrote a competing patch (attached) before I found out about this one.

I think it makes more sense to keep the ARM code separate from the SSE code, as they are not really related at all.

Comment 14

[Timothy B. Terriberry (:derf)]

Just adding that I've tested my patch on both Maemo and Android, and x86 Linux, and does the right thing in each case (as noted in bug 640980). I've also now tested it on a Tegra2, and it correctly disables NEON.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 518835 [details] [diff] [review]
Generic ARM/NEON detection

I had a quick look and assuming this just a port of SSE.h to ARM saw nothing objectionable. It might be worth only using the /proc/cpuinfo instead of /proc/self/auxv on Android like pixman does.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #15)
> Comment on attachment 518835 [details] [diff] [review]
> Generic ARM/NEON detection
> 
> I had a quick look and assuming this just a port of SSE.h to ARM saw nothing
> objectionable. It might be worth only using the /proc/cpuinfo instead of
> /proc/self/auxv on Android like pixman does.

The comment about scratchbox is a little unclear. It makes it seem like /proc/self/auxv would work in scratchbox, but we fail detection because we're using /proc/cpuinfo and that this is undesirable.

Comment 17

[Timothy B. Terriberry (:derf)]

Attachment: Generic ARM/NEON detection (version 2)

Updated patch with revised comment, carrying forward r=jrmuizel

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 514163 [details] [diff] [review]
patch v2

Sounds like this review request isn't needed anymore; sorry for delaying it so long.

Comment 19

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/3b33622916e2