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

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougc, Assigned: dougc)

Tracking

Trunk
mozilla34
ARM
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

5 years ago
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
Assignee

Comment 1

5 years ago
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.
Attachment #8469284 - Flags: review?(nicolas.b.pierron)
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?
Attachment #8469284 - Flags: review?(nicolas.b.pierron) → feedback+
Assignee

Comment 3

5 years ago
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.
Attachment #8469284 - Attachment is obsolete: true
Attachment #8470033 - Flags: review?(nicolas.b.pierron)
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!
Attachment #8470033 - Flags: review?(nicolas.b.pierron) → review+
Thanks for doing this. Moving this out of assembler/ was on my list for bug 1046585, but now I no longer have to :)
Blocks: 1046585
Assignee

Comment 7

5 years ago
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.
Attachment #8470033 - Attachment is obsolete: true
Attachment #8471207 - Flags: review+
Assignee

Comment 9

5 years ago
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
Attachment #8471207 - Attachment is obsolete: true
Attachment #8471629 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 11

5 years ago
Could you please leave this open. I just spotted an unused variable warning that needs fixing.
Keywords: leave-open
Assignee

Comment 13

5 years ago
* 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().
Attachment #8475812 - Flags: review?(jdemooij)
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.
Attachment #8475812 - Flags: review?(jdemooij) → review+
Assignee

Updated

5 years ago
https://hg.mozilla.org/mozilla-central/rev/ff9ae7c8bada
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.