836486 - IonMonkey: Add support for ARMv6

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marty Rosenberg [:mjrosenb]]

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.

Comment 1

[Marty Rosenberg [:mjrosenb]]

Attachment: partially tested patch

evidently, I never actually attached this when I filed the bug earlier.  My bad.

Comment 2

[Jacob Bramley [:jbramley]]

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 3

[Jacob Bramley [:jbramley]]

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)"

Comment 4

[Mozilla RelEng Bot]

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/[e-mail]-a8d58f57798d

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This doesn't have an r+ anywhere that I can see...

Comment 6

[Marty Rosenberg [:mjrosenb]]

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

ok, I ran this on try, and it seemed to not explode!

Comment 7

[Till Schneidereit [:till]]

Did you mean to tag someone for review?

Comment 8

[Jacob Bramley [:jbramley]]

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".

Comment 9

[Nicolas B. Pierron [:nbp]]

(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 ?

Comment 10

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mjrosenb/patches/ARMv6-r2.patch

Nits addressed, sans the curly braces, IM style is to omit them on single line statements.

Comment 11

[Jacob Bramley [:jbramley]]

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.

Comment 12

[Jacob Bramley [:jbramley]]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android & B2G build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee67360ec662

Comment 14

[Marty Rosenberg [:mjrosenb]]

re-landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f00c41faab97
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d03ef1c2f0

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f00c41faab97
https://hg.mozilla.org/mozilla-central/rev/67d03ef1c2f0