Closed Bug 1028008 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

Product:
Core
Component:
JavaScript Engine: JIT
Platform:
ARM
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: dougc, Assigned: dougc)

References

Details

Attachments

(1 file, 3 obsolete files)

Support reading a js shell 'arm-hwcap' argument in addition to the ARMHWCAP environment variable.
37.21 KB, patch
dougc
: review+
Details | Diff | Splinter Review 
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.
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)
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.
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+
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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.

Attachment

General

Creator:
Created:
Updated:
Size: