1050278 - IonMonkey: (ARM) consolidate the mpu feature detection and remove the ARM remnants in js/src/assembler/

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Douglas Crosher [:dougc]]

The ARM backend has some mpu feature detection code in both js/src/assembler/ and js/src/jit/arm/Architecture-arm.cpp, and there is unnecessary duplication. It is proposed that the remnants in js/src/assembler/ be removed and the code be consolidated into Architecture-arm.cpp

Comment 1

[Douglas Crosher [:dougc]]

Attachment: Consolidate the mpu feature detection and remove the ARM remnants in js/src/assembler/

The code removed from js/src/assembler/ was for FPU detection. It has been replaced by the more general code in js/src/jit/arm/

The feature detection has been improved in a few ways:

1. It defaults some key features based on the features used to compile the compiler. The default browser is compiled to exploit ARMv7 and VFP support, and the ARMv6 build is compiled to use VFP support, The code was already doing this, it's just been consolidated. Even if reading the auxv and cpuinfo failed, the feature detection logic will still typically default to VFP to at least enable the use of the JIT.

2. For Linux if reading /proc/self/auxv fails then now falling back to reading /proc/cpuinfo.

3. Try to detect the architecture from the features. The auxv does not have the arch, but the arch can typically be detected from the features, such as vfpv3 or neon which are armv7 specific.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8469284 [details] [diff] [review]
Consolidate the mpu feature detection and remove the ARM remnants in js/src/assembler/

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

::: js/src/jit/arm/Architecture-arm.cpp
@@ +150,2 @@
>  #if WTF_OS_LINUX
>      int fd = open("/proc/self/auxv", O_RDONLY);

This file seems to also be available on B2G, is there any reasons to not use it there.

Note that WTF_OS_LINUX excludes WTF_OS_ANDROID, which is likely to be set when we are compiling for B2G.

@@ +183,5 @@
> +                char  ch = buf[i];
> +                if (!ch)
> +                    break;
> +                else if (ch == '\n')
> +                    buf[i] = 0x20;

I don't think this is needed, as the feature list is already adding extra whitespaces (at least on the 2 phones that I tested with)

Flame:
  Features        : swp half thumb fastmult vfp edsp neon vfpv3 tls vfpv4 idiva idivt $

Unagi:
  Features        : swp half thumb fastmult vfp edsp thumbee neon vfpv3 tls vfpv4 $

on the other hand, it might be good to do the following before the loop:

  const char *featureList = strstr(buf, "Features");
  if (char *featuresEnd = strstr(featureList, "\n"))
      *featuresEnd = '\0';

and replace "strstr(bug, …)" by "strstr(featureList, …)", and avoid iterating over the ~300 bytes for each miss.

@@ +208,5 @@
> +            if (strstr(buf, "ARMv7"))
> +                flags |= HWCAP_ARMv7;
> +#ifdef DEBUG
> +            IonSpew(IonSpew_Codegen, "ARMHWCAP: '%s'\n   flags: 0x%x\n", buf, flags);
> +#endif

IonSpew is defined to be an empty function in Debug builds.
Remove the #ifdef DEBUG

Can we split the spew of the buffer from the spew of the flag, and move the spew of the flag before the return?

Comment 3

[Douglas Crosher [:dougc]]

Attachment: Consolidate the mpu feature detection and remove the ARM remnants in js/src/assembler/

Thank you for the quick feedback.

"/proc/self/auxv" is likely available on Android and B2G, but the Android NDK does not defined the HWCAP flags. It seems very reasonable to simply inline the necessary definitions from the Linux kernel header file, and this has been done. Android and B2g now also read the auxv, with a fall back of reading the cpuinfo features.

The parsing of the features has been refactored into ParseARMCpuFeatures().

Recent kernels have added a HWCAP_VFPD32 flag, and support for reading this has been added.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8470033 [details] [diff] [review]
Consolidate the mpu feature detection and remove the ARM remnants in js/src/assembler/

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

::: js/src/jit/arm/Architecture-arm.cpp
@@ +52,5 @@
> +// Parse the Linux kernel cpuinfo features. This is also used to parse the
> +// override features which has some extensions: 'armv7' and 'hardfp'.
> +uint32_t
> +ParseARMCpuFeatures(const char *features, bool override = false)
> +{

Nice factorization!
This means that every feature that we support this way be added on the command line as well :)

@@ +223,5 @@
> +
> +#endif // JS_ARM_SIMULATOR
> +
> +    // Canonicalize the flags. These rules are also applied to the features
> +    // supplied for simulation.

Sweet!

Comment 5

[Jan de Mooij [:jandem]]

Thanks for doing this. Moving this out of assembler/ was on my list for bug 1046585, but now I no longer have to :)

Comment 6

[Douglas Crosher [:dougc]]

https://tbpl.mozilla.org/?tree=Try&rev=0dd87c49aab8

Comment 7

[Douglas Crosher [:dougc]]

Attachment: Consolidate the mpu feature detection and remove the ARM remnants in js/src/assembler/

The try build failed due to NDK 8d not defining Elf32_auxv_t. This was fixed in NDK 8e, but it is easy to workaround by inlining the definition.

Comment 8

[Douglas Crosher [:dougc]]

https://tbpl.mozilla.org/?tree=Try&rev=9c8dd68653c0

Comment 9

[Douglas Crosher [:dougc]]

Attachment: Consolidate the mpu feature detection and remove the ARM remnants in js/src/assembler/

Sorry, there was another use of Elf32_auxv_t that caused try build failures - fixed. Here's another try run, the builds succeeded but 'B2G ICS Emulator Debug' tests failed. https://tbpl.mozilla.org/?tree=Try&rev=0eb5190b7c70

It looks like infra, so trying again to be sure:
https://tbpl.mozilla.org/?tree=Try&rev=01ed91122521

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Douglas Crosher [:dougc]]

Could you please leave this open. I just spotted an unused variable warning that needs fixing.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Douglas Crosher [:dougc]]

Attachment: Canonicalize the hwcap flags even when overridden, and add a flag for alignment faults.

* The prior patch had intended to canonicalize the flags even when they were obtained from a overriding js shell argument or environment variable. The code expects them in canonical form. Factored out CanonicalizeARMHwCapFlags().

* B2G also defines ANDROID, and B2G and Android define __linux__, so the conditionals can be simplified a little.

* Added a flag for alignment faults. Need this to explore an regexp issue.

* Separated the initialization of the flags, now InitARMFlags(), from GetARMFlags(), and call InitARMFlags() from InitializeIon().

Comment 14

[Douglas Crosher [:dougc]]

https://tbpl.mozilla.org/?tree=Try&rev=d6efcf6258e3

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8475812 [details] [diff] [review]
Canonicalize the hwcap flags even when overridden, and add a flag for alignment faults.

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

Sorry for the delay, nice refactoring.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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