Closed Bug 583958 Opened 10 years ago Closed 9 years ago

Add NEON detection to SSE.h?

Categories

(Core :: XPCOM, defect)

ARM
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: m_kato, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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

I need NEON detection for ARM processor such as Cortex-A8/SnapDragon.
Attached patch patch (obsolete) — Splinter Review
Blocks: 606452
Attachment #472341 - Flags: review?(dbaron)
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.
Justin, what status of bug 585708?  I need this for NEON.
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().
(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.
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.)
FYI, I'm waiting to review this until after bug 585708 lands.
And FYI, I'm waiting to land bug 585708 until the patches in its dependency tree get reviewed.
Attached patch patch v2Splinter Review
Attachment #472341 - Attachment is obsolete: true
Attachment #472341 - Flags: review?(dbaron)
Attachment #514163 - Flags: review?(dbaron)
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?
Duplicate of this bug: 640980
Attached patch Generic ARM/NEON detection (obsolete) — Splinter Review
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.
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 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.
Attachment #518835 - Flags: review+
(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.
Updated patch with revised comment, carrying forward r=jrmuizel
Attachment #518835 - Attachment is obsolete: true
Attachment #519174 - Flags: review+
Comment on attachment 514163 [details] [diff] [review]
patch v2

Sounds like this review request isn't needed anymore; sorry for delaying it so long.
Attachment #514163 - Flags: review?(dbaron)
http://hg.mozilla.org/mozilla-central/rev/3b33622916e2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Depends on: 650162
Depends on: 668672
You need to log in before you can comment on or make changes to this bug.