811023 - Libtremor compiled without arm assembly fast paths

Status: RESOLVED INVALID

(Core :: Audio/Video, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

I came across this while doing some experimental stuff to try to get Fennec building with Thumb-1 on ARMv6 processors. It looks like libtremor is compiled without the _ARM_ASSEM_ #define when MOZ_THUMB2 is defined, which seems wrong. It was causing a failure when building for arm without MOZ_THUMB2, because then _ARM_ASSEM_ *was* defined, but the assembly code in question uses Thumb2 instructions which weren't allowed.

Patch of what I think makes more sense. I also pushed this to try at https://tbpl.mozilla.org/?tree=Try&rev=38edcda7c9e9 to make sure it doesn't blow anything up.

Comment 1

[Timothy B. Terriberry (:derf)]

See bug 511348 comment 27. The asm was originally disabled because of build bustage when compiling _with_ MOZ_THUMB2. But let's see what try says now.

Can you post the build failures when compiling without MOZ_THUMB2?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: asm errors

This is the end of the build output when I try to build with (arm && !thumb2). Note that this is building with -j9 so there are a few files that all fail together; if I run with -j1 only a subset of these errors show up, depending on which "broken" file gets compiled first. This also means that there might be other files that are "broken" that are not listed in this build log output.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

And yeah, from the try failures it looks like this doesn't build with thumb2 either. So maybe we should just take out that makefile chunk entirely, so that the _ARM_ASSEM_ never gets defined?

Comment 4

[Timothy B. Terriberry (:derf)]

The LO_REGS thing looks like you actually are passing -mthumb from somewhere, but aren't setting the MOZ_THUMB2 define. I'd be curious to see your .mozconfig and the actual command lines being invoked here. However, I am okay with simply disabling _ARM_ASSEM_ entirely (the benefit from it is fairly small).

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy B. Terriberry (:derf) from comment #4)
> The LO_REGS thing looks like you actually are passing -mthumb from
> somewhere, but aren't setting the MOZ_THUMB2 define.

Yes, that is exactly what I'm doing.

 I'd be curious to see
> your .mozconfig and the actual command lines being invoked here. However, I
> am okay with simply disabling _ARM_ASSEM_ entirely (the benefit from it is
> fairly small).

This is the actual command line for tremor_floor0.c:

/usr/bin/ccache /home/kats/android/ndk/toolchains/arm-linux-androideabi-4.6.3/prebuilt/linux-x86/bin/arm-linux-androideabi-gcc -o tremor_floor0.o -c -fvisibility=hidden -D_ARM_ASSEM_ -DNO_NSPR_10_SUPPORT -I/home/kats/zspace/mozilla-git/media/libtremor/include/tremor -I/home/kats/zspace/mozilla-git/media/libtremor/lib -I. -I../../../dist/include  -I/home/kats/zspace/mozilla-git/obj-android/dist/include/nspr -I/home/kats/zspace/mozilla-git/obj-android/dist/include/nss      -fPIC -isystem /home/kats/android/ndk/platforms/android-5/arch-arm/usr/include  -pedantic -Wall -Wpointer-arith -Wdeclaration-after-statement -Werror=return-type -Wtype-limits -Wempty-body -Wno-unused -Wno-overlength-strings -Wno-long-long -mandroid -fno-short-enums -fno-exceptions -march=armv6 -mthumb -mthumb-interwork -mfpu=vfp -fno-strict-aliasing -ffunction-sections -fdata-sections -pipe  -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -fno-reorder-functions -fomit-frame-pointer  -isystem /home/kats/android/ndk/platforms/android-5/arch-arm/usr/include  -include ../../../mozilla-config.h -DMOZILLA_CLIENT -MD -MF .deps/tremor_floor0.o.pp  /home/kats/zspace/mozilla-git/media/libtremor/lib/tremor_floor0.c

The relevant chunk of my mozconfig is this:

# ARMv6 (Thumb)
ac_add_options --with-arch=armv6
export MOZ_PKG_SPECIAL=armv6
ac_add_options --disable-ion
ac_add_options --with-thumb=yes
ac_add_options --with-thumb-interwork=yes

I also have a modification to build/autoconf/arch.m4 (and js/src/build/autoconf/arch.m4) that looks like this:

diff --git a/build/autoconf/arch.m4 b/build/autoconf/arch.m4
index 0310234..4b2fab3 100644
--- a/build/autoconf/arch.m4
+++ b/build/autoconf/arch.m4
@@ -105,7 +105,11 @@ esac
 
 case "$MOZ_THUMB" in
 yes)
-    MOZ_THUMB2=1
+    if test "$MOZ_ARCH" = "armv6"; then
+        MOZ_THUMB2=
+    else
+        MOZ_THUMB2=1
+    fi
     thumb_flag="-mthumb"
     ;;
 no)


The intention here is to build for ARMv6 (i.e. -march=armv6) with thumb enabled (-mthumb). gcc automatically uses Thumb-1 instead of Thumb-2, so I made sure MOZ_THUMB2 was not defined in this scenario. There are a bunch of pieces of code in our tree that use Thumb2, but most of them guard the relevant code with a MOZ_THUMB2 or equivalent ifdef, so flipping MOZ_THUMB2 off pushes those code paths into slower C fallbacks as expected/desired.

Comment 6

[Timothy B. Terriberry (:derf)]

(In reply to Kartikaya Gupta (:kats) from comment #5)
> The intention here is to build for ARMv6 (i.e. -march=armv6) with thumb
> enabled (-mthumb). gcc automatically uses Thumb-1 instead of Thumb-2, so I

Okay, I wasn't aware we ever tried to do that. The _ARM_ASSEM_ stuff definitely won't work in that case. So the options are either make a MOZ_THUMB define that we can check like we do the MOZ_THUMB2 one, or disable _ARM_ASSEM_ entirely.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy B. Terriberry (:derf) from comment #6)
> Okay, I wasn't aware we ever tried to do that.

I don't know if we tried to that before, but I'm trying now :)

> The _ARM_ASSEM_ stuff
> definitely won't work in that case. So the options are either make a
> MOZ_THUMB define that we can check like we do the MOZ_THUMB2 one, or disable
> _ARM_ASSEM_ entirely.

Ok, that's fair. Thinking about it more I think it makes sense to add a MOZ_THUMB define anyway so maybe I'll do that. However that means the change to the libtremor Makefile will have to land with the other changes I'm doing, if it ever lands at all. So I think it makes sense to close this bug out for now and then we can deal with it when I have that stuff working. (bug 792134 in case you want to follow along).

Comment 8

[Timothy B. Terriberry (:derf)]

(In reply to Kartikaya Gupta (:kats) from comment #7)
> doing, if it ever lands at all. So I think it makes sense to close this bug
> out for now and then we can deal with it when I have that stuff working.
> (bug 792134 in case you want to follow along).

Okay, makes sense. I'm canceling the review for now, but feel-free to ask again when you have a new patch.