IonMonkey: (ARM) support reading a js shell 'arm-hwcap' argument in addition to the ARMHWCAP environment variable.

RESOLVED FIXED in mozilla33

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dougc, Assigned: dougc)

Tracking

unspecified
mozilla33
ARM
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
Bug 1026919 comment 9 requests a js shell argument to specify the ARM code generation features. This is currently supported by an environment variable ARMHWCAP and adding support for a js shell argument sounds useful.
(Assignee)

Comment 1

4 years ago
Created attachment 8443303 [details] [diff] [review]
Support reading a js shell 'arm-hwcap' argument in addition to the ARMHWCAP environment variable.
Attachment #8443303 - Flags: review?(mrosenberg)
(Assignee)

Comment 2

4 years ago
Comment on attachment 8443303 [details] [diff] [review]
Support reading a js shell 'arm-hwcap' argument in addition to the ARMHWCAP environment variable.

While it worked ok, I don't like storing the pointer to the string and will rework it to parse the string to the flags when parsing the command line.
Attachment #8443303 - Flags: review?(mrosenberg)
(Assignee)

Comment 3

4 years ago
Created attachment 8444217 [details] [diff] [review]
Support reading a js shell 'arm-hwcap' argument in addition to the ARMHWCAP environment variable.

Refactor out the parsing of the ARMHWCAP override flags, and call it to parse the new --arm-hwcap js shell argument at the time that the js shell arguments are parsed.

The fuzzers have requested this js shell argument to help with ARMv6 fuzzing.

It might also help in enabling ARMv6 JIT compiler code generation for the tbpl ARMv6 Android build.
Attachment #8443303 - Attachment is obsolete: true
Attachment #8444217 - Flags: review?(jdemooij)
Comment on attachment 8444217 [details] [diff] [review]
Support reading a js shell 'arm-hwcap' argument in addition to the ARMHWCAP environment variable.

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

Nice, r=me with nits addressed.

::: js/src/jit/arm/Architecture-arm.cpp
@@ +35,5 @@
>  namespace jit {
>  
> +// The override flags parsed from the ARMHWCAP environment variable or from the
> +// --arm-hwcap js shell argument.
> +uint32_t armHwCapFlags = 0;

Nit: static uint32_t ...

@@ +159,5 @@
>                  flags = aux.a_un.a_val;
>                  isSet = true;
>  #if defined(__ARM_ARCH_7__) || defined (__ARM_ARCH_7A__)
> +                // This should really be detected at runtime, but /proc/*/auxv
> +                // doesn't seem to carry the ISA.  I could look in /proc/cpuinfo

Pre-existing nit: we usually use "We" instead of "I" in comments.

@@ +220,5 @@
>      if (strstr(buf, " neon "))
>          flags |= HWCAP_NEON;
>  
> +    // Not part of the HWCAP flag, but I need to know this, and we're not using
> +    // that bit, so... I'm using it.

Same here. Thanks for capitalizing the comments btw!

::: js/src/jit/arm/Architecture-arm.h
@@ +232,5 @@
>  bool hasVFP();
>  bool has16DP();
>  bool hasIDIV();
>  
> +bool parseARMHwCapFlags(const char *armHwCap);

Nit: if the function is not a class/struct member, its name should be capitalized: ParseARMHwCapFlags

(Some other functions in this file have the same problem.)
Attachment #8444217 - Flags: review?(jdemooij) → review+
Oh you should probably post a --flag=value list the fuzzers can copy/paste.
Comment on attachment 8444217 [details] [diff] [review]
Support reading a js shell 'arm-hwcap' argument in addition to the ARMHWCAP environment variable.

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

Nice, r=me with nits addressed.

::: js/src/jit/arm/Architecture-arm.cpp
@@ +35,5 @@
>  namespace jit {
>  
> +// The override flags parsed from the ARMHWCAP environment variable or from the
> +// --arm-hwcap js shell argument.
> +uint32_t armHwCapFlags = 0;

Nit: static uint32_t ...

@@ +159,5 @@
>                  flags = aux.a_un.a_val;
>                  isSet = true;
>  #if defined(__ARM_ARCH_7__) || defined (__ARM_ARCH_7A__)
> +                // This should really be detected at runtime, but /proc/*/auxv
> +                // doesn't seem to carry the ISA.  I could look in /proc/cpuinfo

Pre-existing nit: we usually use "We" instead of "I" in comments.

@@ +220,5 @@
>      if (strstr(buf, " neon "))
>          flags |= HWCAP_NEON;
>  
> +    // Not part of the HWCAP flag, but I need to know this, and we're not using
> +    // that bit, so... I'm using it.

Same here. Thanks for capitalizing the comments btw!

::: js/src/jit/arm/Architecture-arm.h
@@ +232,5 @@
>  bool hasVFP();
>  bool has16DP();
>  bool hasIDIV();
>  
> +bool parseARMHwCapFlags(const char *armHwCap);

Nit: if the function is not a class/struct member, its name should be capitalized: ParseARMHwCapFlags

(Some other functions in this file have the same problem.)

::: js/src/shell/js.cpp
@@ +6258,5 @@
>          || !op.addIntOption('\0', "available-memory", "SIZE",
>                              "Select GC settings based on available memory (MB)", 0)
> +#if defined(JS_CODEGEN_ARM)
> +        || !op.addStringOption('\0', "arm-hwcap", "[features]",
> +                               "Specify ARM code generation features.")

Maybe add something like: or "help" to list all features.
(Assignee)

Comment 7

4 years ago
Created attachment 8444878 [details] [diff] [review]
Support reading a js shell 'arm-hwcap' argument in addition to the ARMHWCAP environment variable.

Address reviewer feedback, including fixing some of other noted capitalization issues in this file. Carrying forward r+.
Attachment #8444217 - Attachment is obsolete: true
Attachment #8444878 - Flags: review+
(Assignee)

Comment 8

4 years ago
Created attachment 8444952 [details] [diff] [review]
Support reading a js shell 'arm-hwcap' argument in addition to the ARMHWCAP environment variable.

Missed renaming one of the functions. Carry forward r+.
Attachment #8444878 - Attachment is obsolete: true
Attachment #8444952 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5399dc155c3b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Comment 12

4 years ago
I have patches for Beta 31 and Aurora 32 testing. Would you want this feature uplifted?
Flags: needinfo?(jdemooij)
(In reply to Douglas Crosher [:dougc] from comment #12)
> I have patches for Beta 31 and Aurora 32 testing. Would you want this
> feature uplifted?

I think it's fine to let it ride the trains... Unless you think it'd be useful at some point.
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.