If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

IonMonkey: Add support for ARMv6

RESOLVED FIXED in mozilla22

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mjrosenb, Unassigned)

Tracking

unspecified
mozilla22
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
There seems to be interest in adding armv6 support.  Support for ARMv6 is mostly subtractive-- turn off features that rely on instructions that don't exist on ARMv6.  Most cases already have alternatives, but a few dont, and those need to be written.
(Reporter)

Comment 1

5 years ago
Created attachment 708528 [details] [diff] [review]
partially tested patch

evidently, I never actually attached this when I filed the bug earlier.  My bad.
Attachment #708528 - Flags: feedback?(Jacob.Bramley)
Comment on attachment 708528 [details] [diff] [review]
partially tested patch

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

Partial review. I'll finish it on Monday.

::: js/src/ion/IonMacroAssembler.cpp
@@ +274,5 @@
>  {
>      JS_ASSERT(input != ScratchFloatReg);
>  #ifdef JS_CPU_ARM
>      Label notSplit;
>      ma_vimm(0.5, ScratchFloatReg);

There's no vmov(imm) in VFPv2. Does this do some magic?

@@ +307,5 @@
> +        as_vcvt(VFPRegister(ScratchFloatReg).uintOverlay(), VFPRegister(input));
> +        // copy the converted value out
> +        as_vxfer(output, InvalidReg, ScratchFloatReg, FloatToCore);
> +        as_vmrs(pc);
> +        ma_mov(Imm32(0), output, NoSetCond, Overflow);

The V (overflow) flag will be set only if 'input' is a NaN. In that case, vcvt should produce (int)0 anyway.

@@ +318,5 @@
> +        // do the check
> +        as_vcmp(ScratchFloatReg, input);
> +        as_vmrs(pc);
> +        ma_bic(Imm32(1), output, NoSetCond, Zero);
> +        bind(&notSplit);

The label name doesn't really make sense in this case.

::: js/src/ion/arm/Architecture-arm.cpp
@@ +9,5 @@
>  namespace ion {
>  
>  bool hasMOVWT()
>  {
> +#ifdef __ARM_ARCH_6__

Since this might one day run on ARMv5, it would be safer to test for ARM_ARCH >= 7. Alternatively, at least make the test pessimistic, and return true _only_ for ARMv7.

@@ +20,3 @@
>  bool hasVFPv3()
>  {
> +#ifdef __ARM_ARCH_6__

Ditto.
Comment on attachment 708528 [details] [diff] [review]
partially tested patch

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

I have some minor comments only. I don't think "r+" makes much sense on a feedback request, but consider it to be approved-with-tweaks.

::: js/src/ion/arm/Assembler-arm.cpp
@@ +647,5 @@
> +        // get the address of the instruction as a raw pointer
> +        char *dataInst = reinterpret_cast<char*>(load);
> +        IsUp_ iu = IsUp_(inst & IsUp);
> +        int32_t offset = inst & 0xfff;
> +        if (iu != IsUp) {

Why not "iu == IsDown"?

@@ +651,5 @@
> +        if (iu != IsUp) {
> +            offset = - offset;
> +        }
> +        if (dest)
> +            *dest = toRD(*load);

The only difference I can see between getPtr32Target and getCF32Target is that the former allows the caller to ask for 'dest' and 'style', and getCF32Target handles branches as well as literal loads. Can one not fall back to the other, to avoid code duplication? That is, getCF32Target could just call getPtr32Target with NULL 'dest' and 'style' arguments.

@@ +653,5 @@
> +        }
> +        if (dest)
> +            *dest = toRD(*load);
> +        if (style)
> +            *style = L_LDR;

Curly brackets please.

@@ +1539,5 @@
> +{
> +    JS_ASSERT(addr->is<InstLDR>());
> +    int32_t offset = addr->encode() & 0xfff;
> +    if ((addr->encode() & IsUp) != IsUp)
> +        offset = -offset;

Curly brackets.

@@ +2332,5 @@
>      const uint32_t *val = getPtr32Target(&iter, &dest, &rs);
>      JS_ASSERT((uint32_t)val == expectedValue.value);
>      reinterpret_cast<MacroAssemblerARM*>(dummy)->ma_movPatchable(Imm32(newValue.value), dest, Always, rs, ptr);
> +    // L_LDR won't cause any instructions to be updated.
> +    if (rs == L_MOVWT) {

Use "rs != L_LDR" to be on the safe side, in case you add other methods later.

@@ +2334,5 @@
>      reinterpret_cast<MacroAssemblerARM*>(dummy)->ma_movPatchable(Imm32(newValue.value), dest, Always, rs, ptr);
> +    // L_LDR won't cause any instructions to be updated.
> +    if (rs == L_MOVWT) {
> +        AutoFlushCache::updateTop(uintptr_t(ptr), 4);
> +        AutoFlushCache::updateTop(uintptr_t(ptr->next()), 4);

This would be faster if you flushed both words at once, since each flush requires a system call. Does AutoFlushCache have some magic to sort that out for you?

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +1210,5 @@
>          double d;
>      } dpun;
>      dpun.d = value;
> +    if (hasVFPv3()) {
> +        if ((dpun.s.lo) == 0) {

Superfluous brackets: "(dpun.s.lo)"
Attachment #708528 - Flags: feedback?(Jacob.Bramley) → feedback+

Comment 4

5 years ago
Try run for a8d58f57798d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a8d58f57798d
Results (out of 75 total builds):
    exception: 1
    success: 69
    warnings: 3
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mrosenberg@mozilla.com-a8d58f57798d
Keywords: checkin-needed
This doesn't have an r+ anywhere that I can see...
Keywords: checkin-needed
(Reporter)

Comment 6

5 years ago
Comment on attachment 708528 [details] [diff] [review]
partially tested patch

ok, I ran this on try, and it seemed to not explode!
Attachment #708528 - Flags: review?
Did you mean to tag someone for review?
(Reporter)

Updated

5 years ago
Attachment #708528 - Flags: review? → review?(Jacob.Bramley)
Comment on attachment 708528 [details] [diff] [review]
partially tested patch

I'm confused. By "checkin-needed", does this mean that someone else will push it? If so, how will the tweaks I listed get fixed?

I my previous review (listed as "feedback") I said "consider it to be approved-with-tweaks".
Blocks: 851450
(In reply to Jacob Bramley [:jbramley] from comment #8)
> Comment on attachment 708528 [details] [diff] [review]
> partially tested patch
> 
> I'm confused. By "checkin-needed", does this mean that someone else will
> push it? If so, how will the tweaks I listed get fixed?

Yes that means that someone else will push it.  I would not assume they are looking at nits before pushing it.

> I my previous review (listed as "feedback") I said "consider it to be
> approved-with-tweaks".

Marty: Is there anything blocking any action based on comment 3 ?
Flags: needinfo?(mrosenberg)
(Reporter)

Comment 10

5 years ago
Created attachment 730583 [details] [diff] [review]
/home/mjrosenb/patches/ARMv6-r2.patch

Nits addressed, sans the curly braces, IM style is to omit them on single line statements.
Attachment #730583 - Flags: review?(Jacob.Bramley)
Flags: needinfo?(mrosenberg)
Comment on attachment 708528 [details] [diff] [review]
partially tested patch

This is the old patch, so I'm marking it as r- to avoid confusion.
Attachment #708528 - Flags: review?(Jacob.Bramley) → review-
Comment on attachment 730583 [details] [diff] [review]
/home/mjrosenb/patches/ARMv6-r2.patch

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

::: js/src/ion/arm/Architecture-arm.cpp
@@ +24,5 @@
>  
> +uint32_t getFlags()
> +{
> +    static bool isSet = false;
> +    static uint32_t flags = 0;

A custom structure (rather than simple flags) would allow you to store version numbers rather than just enumerations of them. That would allow clearer tests for things like "ARMv7 or above", and it's more extensible.

This will do for now, though, so don't worry about fixing it for this patch.

@@ +42,5 @@
> +                // this should really be detected at runtime, but
> +                // /proc/*/auxv doesn't seem to carry the ISA
> +                // I could look in /proc/cpuinfo as well, but
> +                // the chances that it will be different from this
> +                // are low.

Why do you use auxv on Linux, but cpuinfo on Android? Why not use cpuinfo in both cases?

@@ +102,2 @@
>  }
> +bool has16DP()

This is strangely named; to my mind, VFPv3-D32 does have 16 D registers, it just has another 16 as well. The old function (has32DR) made more sense since it obviously excluded VFPv3-D16.

::: js/src/ion/arm/Architecture-arm.h
@@ +208,5 @@
>  };
>  
>  bool hasMOVWT();
>  bool hasVFPv3();
> +static const bool hasVFP() {return true;}

You just added a fancy mechanism for testing that, so you might as well use it.
Attachment #730583 - Flags: review?(Jacob.Bramley) → review+
Backed out for Android & B2G build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee67360ec662
Blocks: 855839
(Reporter)

Comment 14

5 years ago
re-landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f00c41faab97
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d03ef1c2f0
https://hg.mozilla.org/mozilla-central/rev/f00c41faab97
https://hg.mozilla.org/mozilla-central/rev/67d03ef1c2f0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.