563751 - add configure option to enable building for thumb2 instruction set

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

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

Attachment: patch

With this we trim our binary size by 4Mb on both android and maemo and get across the board speed ups according to fennecbench (5% win for panning, 17% for page load lag etc.)

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

This patch will attempt to build thumb2 on the N810s (which are mistakenly considered maemo 5)

Comment 2

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

Attachment: js patch

Comment 3

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

Attachment: nspr patch

these nspr and js patches only enable thumb2 by default for android and maemo 6 (not maemo 5) based on mfinkle's comment.  I'll make the same change to the main configure patch when pushing.

--enable-thumb2 will still work for maemo 5 if passed to configure

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 443482 [details] [diff] [review]
nspr patch

># HG changeset patch
># User Brad Lassey <[e-mail]>
># Date 1273012775 14400
># Node ID c04eed45c9f4be290fcaedf2c7c3a969b3a2396f
># Parent  b3d707fb93a6799eb5e08a9ada8dacbbc2c49fd4
>imported patch nspr-thumb2
>
>diff -r b3d707fb93a6 -r c04eed45c9f4 nsprpub/configure.in
>--- a/nsprpub/configure.in	Tue May 04 18:38:03 2010 -0400
>+++ b/nsprpub/configure.in	Tue May 04 18:39:35 2010 -0400

> dnl ========================================================
>+dnl = Maemo checks
>+dnl ========================================================
>+
>+MAEMO_SDK_TARGET_VER=-1
>+
>+AC_ARG_WITH(maemo-version,
>+[  --with-maemo-version=MAEMO_SDK_TARGET_VER
>+                        Maemo SDK Version],

It seems a little weird to add all this machinery here just to enable thumb. Could you drop this from the NSPR configure, and instead just pass --enable-thumb2 to NSPR's sub-configure if MOZ_THUMB2 is set by the top-level configure?

>+dnl ========================================================
>+dnl = Enable building the Thumb2 instruction set
>+dnl ========================================================
>+AC_ARG_ENABLE(thumb2,
>+ [  --enable-thumb2              Enable Thumb2 instruction set],
>+ [ if test "$enableval" = "yes"; then
>+     MOZ_THUMB2=1,
>+   fi ])
>+
>+
>+if test -n "$MOZ_THUMB2"; then

Seems like you probably want to check that the target CPU is arm, no?

>+  CFLAGS="$CFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb"
>+  CXXFLAGS="$CXXFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb"
>+  ASFLAGS="$ASFLAGS -march=armv7-a -mthumb"

This should check GNU_CC.

>+  AC_SUBST(CLFAGS)
>+  AC_SUBST(CXXFLAGS)
>+  AC_SUBST(ASFLAGS)

You don't need these AC_SUBSTs, they're already substituted below. Also,

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 443481 [details] [diff] [review]
js patch

Basically the same comments as the NSPR patch.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 443418 [details] [diff] [review]
patch

># HG changeset patch
># User Brad Lassey <[e-mail]>
># Date 1273002047 14400
># Node ID 6292aa8974cdd5b82d2b196db506619828574297
># Parent  7939af53a1d16eebdcf7b84a16c5ffc5caf33e1b
>[mq]: thumb2-configure-option
>
>diff -r 7939af53a1d1 -r 6292aa8974cd configure.in
>--- a/configure.in	Wed Apr 28 17:30:00 2010 -0700
>+++ b/configure.in	Tue May 04 15:40:47 2010 -0400
>@@ -4915,6 +4915,7 @@
> MOZ_SPLASHSCREEN=
> MOZ_STORAGE=1
> MOZ_SVG=1
>+MOZ_THUMB2=
> MOZ_TIMELINE=
> MOZ_TOOLKIT_SEARCH=1
> MOZ_UI_LOCALE=en-US
>@@ -6766,7 +6767,11 @@
>       if test -z "$_LIB_FOUND"; then
>          AC_MSG_ERROR([Hildon FM-2 is required when building for Maemo])
>       fi
>-
>+      MOZ_THUMB2=1
>+   fi
>+
>+   if test $MOZ_PLATFORM_MAEMO = 6; then
>+      MOZ_THUMB2=1
>    fi

Might make sense to just make a separate block that says:
if test "$MOZ_PLATFORM_MAEMO" -gt 5; then
  MOZ_THUMB2=1
fi

> dnl ========================================================
>+dnl = Enable building the Thumb2 instruction set
>+dnl ========================================================
>+MOZ_ARG_ENABLE_BOOL(thumb2,
>+ [  --enable-thumb2              Enable Thumb2 instruction set],
>+    MOZ_THUMB2=1,)
>+
>+if test -n "$MOZ_THUMB2"; then
>+  CFLAGS="$CFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb"
>+  CXXFLAGS="$CXXFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb"
>+  ASFLAGS="$ASFLAGS -march=armv7-a -mthumb"
>+  AC_SUBST(CLFAGS)
>+  AC_SUBST(CXXFLAGS)
>+  AC_SUBST(ASFLAGS)

These already get AC_SUBSTed later in the file. (Also: CLFAGS?)

Comment 7

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

Attachment: js patch v.2

Comment 8

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

Attachment: nspr patch v.2

Comment 9

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

Attachment: patch v.2

Comment 10

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

Comment on attachment 447030 [details] [diff] [review]
patch v.2

>+   if test "$MOZ_PLATFORM_MAEMO" -ge 5; then

just realized that if we don't want thumb2 builds for the n810 we should change this to -gt

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 447029 [details] [diff] [review]
nspr patch v.2

>diff -r 28c9048d8ae4 -r 5aa90111be10 nsprpub/configure.in
>--- a/nsprpub/configure.in	Tue May 04 15:40:47 2010 -0400
>+++ b/nsprpub/configure.in	Tue May 04 18:39:35 2010 -0400
>+if test -n "$MOZ_THUMB2"; then
>+  case "$target" in
>+    *arm*)

I think you can just do case "$target_cpu" in
  arm)

can't you?

>+      if test "$GNU_CC"; then
>+        CFLAGS="$CFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb"
>+        CXXFLAGS="$CXXFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb"
>+        ASFLAGS="$ASFLAGS -march=armv7-a -mthumb"
>+      else
>+        AC_MSG_ERROR([--enable-thumb2 specified for non-gnu toolchain])
>+      fi
>+    ;;
>+    *)
>+      AC_MSG_ERROR([--enable-thumb2 specified for an arch other than arm])
>+    ;;

I'd probably reword these two errors:
--enable-thumb2 is not supported for non-GNU toolchains
--enable-thumb2 is not supported for non-ARM CPU architectures

r=me with those nits fixed.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 447028 [details] [diff] [review]
js patch v.2

r=me with the same nits as the NSPR patch.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 447030 [details] [diff] [review]
patch v.2

># HG changeset patch
># User Brad Lassey <[e-mail]>
># Date 1273002047 14400
># Node ID 28c9048d8ae48cc89ecda89e163956acb74863a6
># Parent  6ef424688579e4ecd0b7e9c9935262e94522a53c
>[mq]: thumb2-configure-option
>
>diff -r 6ef424688579 -r 28c9048d8ae4 configure.in
>--- a/configure.in	Fri May 21 15:27:24 2010 -0400
>+++ b/configure.in	Tue May 04 15:40:47 2010 -0400
>@@ -5014,6 +5014,7 @@
> MOZ_SPLASHSCREEN=
> MOZ_STORAGE=1
> MOZ_SVG=1
>+MOZ_THUMB2=
> MOZ_TIMELINE=
> MOZ_TOOLKIT_SEARCH=1
> MOZ_UI_LOCALE=en-US
>@@ -6854,6 +6855,17 @@
> 
>    fi
> 
>+   if test "$MOZ_PLATFORM_MAEMO" -ge 5; then
>+     MOZ_THUMB2=1
>+     case "$ac_configure_args" in
>+       *--disable-thumb2*)
>+       ;;
>+       *)
>+         _SUBDIR_CONFIG_ARGS="$_SUBDIR_CONFIG_ARGS --enable-thumb2"
>+       ;;
>+     esac

You don't need to set this way up here, you can just check MOZ_THUMB2 right before we run the sub-configures, and add to ac_configure_args there:
http://mxr.mozilla.org/mozilla-central/source/configure.in#8963

That should make that a bit cleaner.

Comment 14

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

Attachment: nspr patch, nits addressed

carrying r+

Comment 15

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

Attachment: js patch, nits addressed

carrying r+

Comment 16

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

Attachment: patch v.3

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 447132 [details] [diff] [review]
nspr patch, nits addressed

Landed in NSPR CVS:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.274; previous revision: 1.273
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.278; previous revision: 1.277
done

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 447140 [details] [diff] [review]
patch v.3

>+if test -n "$MOZ_THUMB2"; then
>+  case "$target" in

$target_cpu like the other patches.

>+    *arm*)
>+      if test "$GNU_CC"; then
>+        CFLAGS="$CFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb"
>+        CXXFLAGS="$CXXFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb"
>+        ASFLAGS="$ASFLAGS -march=armv7-a -mthumb"
>+      else
>+        AC_MSG_ERROR([--enable-thumb2 specified for non-gnu toolchain])
>+      fi
>+    ;;
>+    *)
>+      AC_MSG_ERROR([--enable-thumb2 specified for an arch other than arm])

Fix the error messages to match the other patches.

>@@ -8977,6 +9007,9 @@
>     if test -n "$USE_ARM_KUSER"; then
>         ac_configure_args="$ac_configure_args --with-arm-kuser"
>     fi
>+    if test -n "$MOZ_THUMB2"; then
>+        ac_configure_args="$ac_configure_args --enable-thumb2"
>+    fi
>     AC_OUTPUT_SUBDIRS(nsprpub)
>     ac_configure_args="$_SUBDIR_CONFIG_ARGS"
> fi

I think you need to add this block to the js/src/configure invocation as well.

r=me with those fixes.

Comment 19

[(not currently active) Ted Mielczarek]

I tagged NSPR CVS with NSPR_HEAD_20100524. rs=me to update the m-c copy.

Comment 20

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

tried pushing that nspr tag, but it caused linux 32 bit builders to burn:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274733715.1274733805.20465.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274737619.1274737724.5911.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274733715.1274733817.20490.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274737620.1274737711.5883.gz

Comment 21

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

pushed http://hg.mozilla.org/mozilla-central/rev/5b6f96b1a706 and http://hg.mozilla.org/mozilla-central/rev/c3af7e83e26f

going to close this out, but still need to get nspr updated