add configure option to enable building for thumb2 instruction set

RESOLVED FIXED

Status

RESOLVED FIXED
9 years ago
11 months ago

People

(Reporter: blassey, Assigned: blassey)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

Created attachment 443418 [details] [diff] [review]
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.)
Attachment #443418 - Flags: review?(ted.mielczarek)
This patch will attempt to build thumb2 on the N810s (which are mistakenly considered maemo 5)
Created attachment 443481 [details] [diff] [review]
js patch
Assignee: nobody → blassey.bugs
Attachment #443481 - Flags: review?(ted.mielczarek)
Created attachment 443482 [details] [diff] [review]
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
Attachment #443482 - Flags: review?(ted.mielczarek)
Comment on attachment 443482 [details] [diff] [review]
nspr patch

># HG changeset patch
># User Brad Lassey <blassey@mozilla.com>
># 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,
Attachment #443482 - Flags: review?(ted.mielczarek) → review-
Comment on attachment 443481 [details] [diff] [review]
js patch

Basically the same comments as the NSPR patch.
Attachment #443481 - Flags: review?(ted.mielczarek) → review-
Comment on attachment 443418 [details] [diff] [review]
patch

># HG changeset patch
># User Brad Lassey <blassey@mozilla.com>
># 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?)
Attachment #443418 - Flags: review?(ted.mielczarek) → review-
Created attachment 447028 [details] [diff] [review]
js patch v.2
Attachment #443481 - Attachment is obsolete: true
Attachment #447028 - Flags: review?(ted.mielczarek)
Created attachment 447029 [details] [diff] [review]
nspr patch v.2
Attachment #443482 - Attachment is obsolete: true
Attachment #447029 - Flags: review?(ted.mielczarek)
Created attachment 447030 [details] [diff] [review]
patch v.2
Attachment #443418 - Attachment is obsolete: true
Attachment #447030 - Flags: review?(ted.mielczarek)
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
Attachment #447029 - Flags: review?(ted.mielczarek) → review+
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 on attachment 447028 [details] [diff] [review]
js patch v.2

r=me with the same nits as the NSPR patch.
Attachment #447028 - Flags: review?(ted.mielczarek) → review+
Attachment #447030 - Flags: review?(ted.mielczarek) → review-
Comment on attachment 447030 [details] [diff] [review]
patch v.2

># HG changeset patch
># User Brad Lassey <blassey@mozilla.com>
># 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.
Created attachment 447132 [details] [diff] [review]
nspr patch, nits addressed

carrying r+
Attachment #447029 - Attachment is obsolete: true
Attachment #447132 - Flags: review+
Created attachment 447134 [details] [diff] [review]
js patch, nits addressed

carrying r+
Attachment #447028 - Attachment is obsolete: true
Attachment #447134 - Flags: review+
Created attachment 447140 [details] [diff] [review]
patch v.3
Attachment #447030 - Attachment is obsolete: true
Attachment #447140 - Flags: review?(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 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.
Attachment #447140 - Flags: review?(ted.mielczarek) → review+
I tagged NSPR CVS with NSPR_HEAD_20100524. rs=me to update the m-c copy.

Updated

9 years ago
Blocks: 567899
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
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 577531
Depends on: 624237

Updated

11 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.