547946 - The configure script enables NEON if the compiler supports it, even if the platform does not.

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Jacob Bramley [:jbramley]]

Building mozilla-central on an ARMv7 platform reports NEON support (which implies a minimum of ARMv7) but no ARMv6 SIMD support.

Further investigation shows that the ARMv6 SIMD test runs the compiler with the default options, but -march=armv6 is required for the compiler to accept the instructions.

The patch to correct this is probably trivial, and I will attach it once I'm satisfied that it's correct. I say "probably trivial" because there is _always_ scope for something to go horribly wrong!

Comment 1

[Jacob Bramley [:jbramley]]

Of course, with a different version of GCC, this bug might not appear. My GCC (4.3.3/Ubuntu) defaults to ARMv5T, so SIMD support is not detected for me.

Comment 2

[Jacob Bramley [:jbramley]]

Actually, I think the real problem here is that NEON _is_ detected, because the configure script adds "-mfpu=neon" (etc) to CFLAGS before running the test. Surely, if the default is ARMv5T, that will fail when it actually compiles some NEON code with the basic CFLAGS? For some reason it doesn't, and I don't really understand why not. Here are the obvious explanations:

  • No NEON code is actually generated. (This is hard to believe; Pixman includes NEON code, and it's definitely getting -DUSE_ARM_NEON.)

  • Some configure magic is used to set the required CFLAGS after support is detected. I don't know configure well enough to know where to start looking, but "grep" turns up nothing useful.

I will experiment further with build options, but I'll put this to one side for now because there are more important things to look at and some of you guys (who have far more knowledge of "configure") may have some insight.

Comment 3

[Jacob Bramley [:jbramley]]

Attachment: Don't force the compiler switch that enables NEON.

The attached patch removes the CFLAGS assignment for the NEON check. If the default CFLAGS enables NEON, it will be detected, but otherwise it will not.

 • This is important on platforms such as Ubuntu Jaunty (and Karmic), where NEON is supported by the toolchain but is not enabled by default in the kernel.

 • In addition, I ran into a problem where the configure script detected NEON code but failed to build Pixman because it uses NEON instructions. This doesn't happen by default; I suspect that Pixman must filter on (USE_ARM_NEON && ARMv7), or something similar. In any case, the attached patch appears to do the right thing here.

Disclaimer:
I don't know if this is the preferred method for editing build parameters because I'm not really familiar with it (or autotools conventions in general).

Comment 4

[Jacob Bramley [:jbramley]]

Comment on attachment 428900 [details] [diff] [review]
Don't force the compiler switch that enables NEON.

Current tip seems to have some weirdness, so this requires further investigation. Specifically, it seems to set -march=armv5te, which surely precludes the use of NEON.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #4)
> Comment on attachment 428900 [details] [diff] [review]
> Don't force the compiler switch that enables NEON.
> 
> Current tip seems to have some weirdness, so this requires further
> investigation. Specifically, it seems to set -march=armv5te, which surely
> precludes the use of NEON.

The default depends on the platform, but if you use --enable-thumb2, we assume armv7 regardless of platform. What platform are you building for?

Comment 6

[Jacob Bramley [:jbramley]]

Oh, I noticed this recently building for ARM. My GCC defaults to ARMv5TE so I used to override it using {C,CPP,CXX}FLAGS, but it started complaining recently that my '-mcpu=cortex-a8' was conflicting with '-march=armv5te', presumably added by the build system.

Comment 7

[Jacob Bramley [:jbramley]]

Thumb-2 and ARMv7 stuff has been documented in bug 618789. It is unrelated to this bug so I haven't created a "depends on" link.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Don't build yuv_convert_arm.cpp when there is no neon support

There is a related issue: yuv_convert_arm.cpp is built unconditionally on arm, even when neon instructions are not understood by the assembler.

Comment 9

[Mike Hommey [:glandium]]

Note that there is a runtime detection function, but it doesn't seem to be used.

Comment 10

[Siarhei Siamashka]

(In reply to comment #8)
> Created attachment 521750 [details] [diff] [review]
> Don't build yuv_convert_arm.cpp when there is no neon support
> 
> There is a related issue: yuv_convert_arm.cpp is built unconditionally on arm,
> even when neon instructions are not understood by the assembler.

NEON instructions are understood by any assembler (unless the assembler is extremely old), but they also need ".fpu neon" directive to tell the assembler that NEON instructions are legal in this code. One of the hacks is to add this directive directly into inline assembly block and don't tweak any compiler flags. Of course, whether to call this function has to be decided at runtime based on NEON availability.

Another possibility is to move this function to a separate .S file, naturally also having ".fpu neon" directive. Separate assembly files do not depend on CFLAGS at all.

Comment 11

[Mike Hommey [:glandium]]

(In reply to comment #10)
> (In reply to comment #8)
> > Created attachment 521750 [details] [diff] [review]
> > Don't build yuv_convert_arm.cpp when there is no neon support
> > 
> > There is a related issue: yuv_convert_arm.cpp is built unconditionally on arm,
> > even when neon instructions are not understood by the assembler.
> 
> NEON instructions are understood by any assembler (unless the assembler is
> extremely old), but they also need ".fpu neon" directive to tell the assembler
> that NEON instructions are legal in this code. One of the hacks is to add this
> directive directly into inline assembly block and don't tweak any compiler
> flags. Of course, whether to call this function has to be decided at runtime
> based on NEON availability.

Actually, the file does have .fpu neon. Now that I look at it again, the problem I was having was actually with the pld instruction, which wasn't supported on my target.

Comment 12

[Siarhei Siamashka]

I see, in this case it is a bit more severe problem if you are interested in a proper support of old ARM hardware (more specifically armv4t). The only good option that I know is to move this code into a separate .S file and use something like the following directives:
        .arch armv7a
        .fpu neon
        .object_arch armv4

Some more details are in:
http://lists.freedesktop.org/archives/pixman/2010-March/000123.html
http://lists.freedesktop.org/archives/pixman/2010-March/000128.html
http://cgit.freedesktop.org/pixman/commit/?id=68d8d832

Comment 13

[(not currently active) Ted Mielczarek]

Is this fixed by the patches in bug 626035?

Comment 14

[Mike Hommey [:glandium]]

(In reply to comment #13)
> Is this fixed by the patches in bug 626035?

It should.

Comment 15

[Mike Hommey [:glandium]]

This should be solved with bug 626035, that has now landed.