857071 - Investigate adding a --force-armv6-ionmonkey config option

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

As of bug 855839 landing, Ionmonkey is enabled on ARMv6 devices, but using dynamic hardware compatibility checks. This means that on tbpl, where we run ARMv6 builds on ARMv7 devices (tegras and pandas), the ARMv6-specific Ionmonkey codepaths are not actually being executed. This means that we're not really testing the stuff that end users will be running, which is less than ideal.

Marty said on IRC (see log in bug 855839 comment 1) that it might be possible to switch to static hardware compatibility checks so that ARMv6 builds use the ARMv6 Ionmonkey codepaths regardless of what device it's running on. This gets us the "test what we're shipping" behaviour, but will have some performance impact.

We should see (1) how much of a performance impact the above has, and if it is unacceptable, (2) investigate adding some sort of mozconfig option to switch to static checking. Then, we could use that mozconfig on tbpl (either on specific trees, like inbound/central but not aurora+) or add a new build type (e.g. android-force-ion) or some combination (e.g. build android-force-ion only on nightly builds) to improve our testing situation.

Marty, can you give an idea of how much of a perf impact it is to switch to static checking?

Comment 1

[Douglas Crosher [:dougc]]

Attachment: Hack patch for testing and discussion.

Here's a hack patch that at least allows the flags to be modified using an environment variable and for some testing to be done using the qemu emulator.  The flags are just a hex number string. The layout of the bit flags differs from the Android/B2G versus other Linux environments which is problematic - this is not a real solution.

Does a real solution need to be based on command line arguments, or would an environment variable or usable too (like IONFLAGS)?

Should each flag be have a separate command line argument?

Would it be better for the flags to reflect the architecture features, or alternatively the capabilities that the compiler needs to identify?

Comment 2

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

(In reply to Douglas Crosher [:dougc] from comment #1)
> Does a real solution need to be based on command line arguments, or would an
> environment variable or usable too (like IONFLAGS)?

In general for android it's much easier to pass along environment variables than command-line arguments, so I would prefer environment variables.

Comment 3

[Douglas Crosher [:dougc]]

Attachment: ARM HWCAP environment variable override.

This patch implements an ARMHWCAP environment variable with a comma or space separated list of features that closely match the Linux /proc/cpuinfo output, plus an armv7 feature.  This overrides the feature detection mechanism when defined.  Not all the features are used, but a good list is noted so that consistent feature lists can be defined in tests.

The patch also tries to address some issues with the current code.

Some examples:
MSM7227a: armv7,swp,half,thumb,fastmult,vfp,edsp,thumbee,neon,vfpv3,tls,vfpv4
MSM7227 : armv6,swp,half,thumb,fastmult,vfp,edsp,java
Raspberry Pi BCM2708: armv6,swp,half,thumb,fastmult,vfp,edsp,java,tls

Comment 4

[Douglas Crosher [:dougc]]

Attachment: ARM HWCAP environment variable override

Fix the patch. Relative to the m-c version now.

Comment 5

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

Just to be clear, if we wanted to force the code to go down ARMv6 codepaths while running on an ARMv7 device, we would take the /proc/cpuinfo feature list from the device and add "armv6" to it? Looking at the cpuinfo for my Nexus 4 it doesn't say "armv7" in the list of features, but if that's present I assume we would have to replace it with armv6.

Comment 6

[Douglas Crosher [:dougc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Just to be clear, if we wanted to force the code to go down ARMv6 codepaths
> while running on an ARMv7 device, we would take the /proc/cpuinfo feature
> list from the device and add "armv6" to it? Looking at the cpuinfo for my
> Nexus 4 it doesn't say "armv7" in the list of features, but if that's
> present I assume we would have to replace it with armv6.

The suggested 'armv6', and 'armv7' features are not part of the Linux
/proc/cpuinfo features.  The compiler needs to differentiate between
these, so we at least need a 'armv7' flag.

Within limits is should be possible to increase the test coverage by
using the flags to change the compiler code generation paths.  Currently
only the 'armv7' and 'vfpv3' features appear to make a difference:

* 'armv7' affect hasMOVWT() and changes some code generation paths in
the ARM compiler.  Not sure if we need 'armv6' but perhaps it would
be a good place holder?

* 'vfpv3' affects hasVFPv3() and changes some code generation paths.

I find that overriding the /proc/cpuinfo detection is necessary when
using the qemu-arm system call emulator to run the jit-tests. Perhaps
it tries to open the hosts /proc/cpuinfo which would likely be
completely inappropriate.  Setting an environment variable is
quicker than hacking on the code.  I'd like to test the ARMv6 support
more too, and this environment variable might help.

Comment 7

[Douglas Crosher [:dougc]]

Attachment: ARM HWCAP environment variable override

Comment 8

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 790568 [details] [diff] [review]
ARM HWCAP environment variable override

Review of attachment 790568 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like a good start, but I suspect the purpose of this is so we can force certain fields to off, rather than always having them on on hardware that supports it.  Each variable should also have a corresponding "Negate" option, which would and that field with ~BITFIELD_VALUE.

Comment 9

[Douglas Crosher [:dougc]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #8)
> Comment on attachment 790568 [details] [diff] [review]
> ARM HWCAP environment variable override
> 
> Review of attachment 790568 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks like a good start, but I suspect the purpose of this is so we can
> force certain fields to off, rather than always having them on on hardware
> that supports it.  Each variable should also have a corresponding "Negate"
> option, which would and that field with ~BITFIELD_VALUE.

Took another look into this and recalled that it is probably necessary to
be able to completely override /proc/cpuinfo because there is, or was, a
need to be able to run the js shell in the qemu user land emulator which
would otherwise read the hosts /proc/cpuinfo which is not appropriate.

An alternative version has been written that just allows features to be
disabled, but it's only of use on real hardware.

The original patch would appear to meet the needs, but do you have
something else in mind that would also work for testing with qemu?

Comment 10

[Douglas Crosher [:dougc]]

Attachment: ARM HWCAP environment variable override

Rebase, and update to work with the ARM simulator added in bug 959597.

Comment 11

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8367036 [details] [diff] [review]
ARM HWCAP environment variable override

Review of attachment 8367036 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm/Architecture-arm.cpp
@@ +39,5 @@
>      if (isSet)
>          return flags;
>  
> +#if defined(DEBUG) || defined(JS_ARM_SIMULATOR)
> +    static const char *env = getenv("ARMHWCAP");

I wouldn't bother only doing this on debug.  the getenv check is fast, and it will probably be useful for perf testing.

@@ +88,5 @@
> +            buf[0] = ' ';
> +            buf[i + 1] = ' ';
> +            buf[i + 2] = '\0';
> +
> +            if (strstr(buf, " vfp "))

Minor nit: it would be nice if we could warn the user if they specify an invalid string in the field.  That way someone doesn't pull their hair out wondering why some assertions aren't triggering when they have ARMHWCAP=vfp3

Comment 12

[Douglas Crosher [:dougc]]

Attachment: Support a HWCAP environment variable override

Address reviewer feedback: enabled on all builds, rather than just DEBUG builds; check that the flags are expected, and print a warning if not.

Carrying forward r+.

Comment 13

[Douglas Crosher [:dougc]]

Attachment: Support a HWCAP environment variable override

Sorry, fix too many closing braces.

Carrying forward r+.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dde9c55ad0a0

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dde9c55ad0a0