Closed Bug 857071 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: kats, Assigned: dougc)

References

Details

Attachments

(1 file, 6 obsolete files)

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?
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?
(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.
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
Attachment #736110 - Attachment is obsolete: true
Fix the patch. Relative to the m-c version now.
Attachment #736236 - Attachment is obsolete: true
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #736255 - Attachment is obsolete: true
Attachment #790568 - Flags: review?
Attachment #790568 - Flags: review? → review?(mrosenberg)
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.
Attachment #790568 - Flags: review?(mrosenberg)
(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?
Flags: needinfo?(mrosenberg)
Rebase, and update to work with the ARM simulator added in bug 959597.
Assignee: nobody → dtc-moz
Attachment #790568 - Attachment is obsolete: true
Attachment #8367036 - Flags: review?(mrosenberg)
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
Attachment #8367036 - Flags: review?(mrosenberg) → review+
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+.
Attachment #8367036 - Attachment is obsolete: true
Attachment #8369032 - Flags: review+
Flags: needinfo?(mrosenberg)
Sorry, fix too many closing braces.

Carrying forward r+.
Attachment #8369032 - Attachment is obsolete: true
Attachment #8369041 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dde9c55ad0a0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.