1066642 - IonMonkey MIPS: Do not allocate odd FP registers on Loongson CPU-s.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Branislav Rankov [:rankov]]

On Loongson CPU-s the odd FP registers behave differently in fp-32 mode than standard MIPS. 
It is more efficient not to use them than to generate the code to work around the problem.

Comment 1

[Branislav Rankov [:rankov]]

Attachment: Fix-Loongson-fp-patr1.patch

Comment 2

[Branislav Rankov [:rankov]]

Attachment: Fix-Loongson-fp-patr2.patch

Comment 3

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8488657 [details] [diff] [review]
Fix-Loongson-fp-patr2.patch

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

::: js/src/jit/mips/Architecture-mips.h
@@ +421,4 @@
>              return FloatRegister(index * 2, RegType(kind));
> +        return FloatRegister(index, RegType(kind));
> +#else
> +# error "Unsupported ABI"

I feel like having multiple #errors for !defined(USES_O32_ABI) is kind of redundant, at least within one file.  On the other hand, it tells you exactly where you need to make modifications, should you add N32 support.

@@ +523,3 @@
>      return true;
> +#else
> +# error "Unsupported ABI"

Ditto.

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8488655 [details] [diff] [review]
Fix-Loongson-fp-patr1.patch

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

::: js/src/configure.in
@@ +3251,5 @@
> +    AC_MSG_ERROR([Invalid --with-mips-arch-variant value.])
> +fi
> +
> +AC_SUBST(MIPS_ARCH_MIPS32R2)
> +AC_SUBST(MIPS_ARCH_MIPS32R1)

You're not using any of those two.

Also, this strikes me as very wrong to do a build-time thing, when, for instance, aiui, besides the kernel, running debian on loongson uses the standard mipsel packages. It would be much better to go with runtime detection.

Comment 5

[Branislav Rankov [:rankov]]

I have done some investigation. Debian packages work on Loongson because debian builds all packages for MIPS2 target. MIPS2 is the least common denominator for most of the MIPS available today. 

I have made a port for MIPS32r2 and MIPS32r1. It also worked on Loongson3a until support for odd registers was added. This is because of difference between MIPS32r2 and Loongson3a.

I will investigate into making this change dynamic.

Comment 6

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS32-Do-not-allocate-odd-FP-registers-on.patch

Workaround for Loongson3-series CPU-s, depends on GCC built-in architecture macro.

Comment 7

[Wan-Teh Chang]

Comment on attachment 8672270 [details] [diff] [review]
0001-IonMonkey-MIPS32-Do-not-allocate-odd-FP-registers-on.patch

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

Hi Heiher,

I am not familiar with js, so I am not qualified to review
this patch. Sorry!

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8672270 [details] [diff] [review]
0001-IonMonkey-MIPS32-Do-not-allocate-odd-FP-registers-on.patch

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

I need some hint to complete reviewing.

::: js/src/jit/mips32/Architecture-mips32.h
@@ +70,5 @@
>      static Code FromName(const char* name);
>  
>      static const uint32_t Total = 64;
>      static const uint32_t TotalDouble = 16;
> +    static const uint32_t TotalRegisters = 32;

Can you add some comment about what this constant actually means?

@@ +73,5 @@
>      static const uint32_t TotalDouble = 16;
> +    static const uint32_t TotalRegisters = 32;
> +#if defined(_MIPS_ARCH_LOONGSON3A)
> +    static const uint32_t TotalSingle = 16;
> +    static const uint32_t Allocatable = 32;

The number of allocatable registers is |28 == TotalDouble + TotalSingle - 4| where 4 is for f16(double), f18(double), f16(single), and f18(single), isn't it?

>    static const SetType NonAllocatableDoubleMask =
>        ((1ULL << FloatRegisters::f16) |
>         (1ULL << FloatRegisters::f18)) << 32;
>    // f16-single and f17-single alias f16-double ...
>    static const SetType NonAllocatableMask =
>        NonAllocatableDoubleMask |
>        (1ULL << FloatRegisters::f16) |
>        (1ULL << FloatRegisters::f17) |
>        (1ULL << FloatRegisters::f18) |
>        (1ULL << FloatRegisters::f19);

@@ +199,5 @@
>          uint32_t code = i & 31;
>          uint32_t kind = i >> 5;
>          return FloatRegister(code, RegType(kind));
>      }
>      // This is similar to FromCode except for double registers on O32.

This comment needs update to mention about Loongson.

@@ +202,5 @@
>      }
>      // This is similar to FromCode except for double registers on O32.
>      static FloatRegister FromIndex(uint32_t index, RegType kind) {
>  #if defined(USES_O32_ABI)
> +# if !defined(_MIPS_ARCH_LOONGSON3A)

Does |defined(_MIPS_ARCH_LOONGSON3A)| imply |defined(USES_O32_ABI)| ?
If not, can you tell me why |defined(_MIPS_ARCH_LOONGSON3A)| branch is not needed for other case?

@@ +207,2 @@
>          if (kind == Double)
> +# endif

#ifdef only for |if (...)| line is a little confusing.
can this be separated somehow?

@@ +209,1 @@
>              return FloatRegister(index * 2, RegType(kind));

Looks like |RegType(...)| cast here is redundant.
Can we just remove it?

@@ +209,3 @@
>              return FloatRegister(index * 2, RegType(kind));
>  #endif
>          return FloatRegister(index, RegType(kind));

Here too.

Comment 9

[hev [:hev]]

Comment on attachment 8672270 [details] [diff] [review]
0001-IonMonkey-MIPS32-Do-not-allocate-odd-FP-registers-on.patch

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

Thanks for review, I will update the patch.

::: js/src/jit/mips32/Architecture-mips32.h
@@ +70,5 @@
>      static Code FromName(const char* name);
>  
>      static const uint32_t Total = 64;
>      static const uint32_t TotalDouble = 16;
> +    static const uint32_t TotalRegisters = 32;

OK

@@ +73,5 @@
>      static const uint32_t TotalDouble = 16;
> +    static const uint32_t TotalRegisters = 32;
> +#if defined(_MIPS_ARCH_LOONGSON3A)
> +    static const uint32_t TotalSingle = 16;
> +    static const uint32_t Allocatable = 32;

Right ;)

@@ +199,5 @@
>          uint32_t code = i & 31;
>          uint32_t kind = i >> 5;
>          return FloatRegister(code, RegType(kind));
>      }
>      // This is similar to FromCode except for double registers on O32.

OK

@@ +202,5 @@
>      }
>      // This is similar to FromCode except for double registers on O32.
>      static FloatRegister FromIndex(uint32_t index, RegType kind) {
>  #if defined(USES_O32_ABI)
> +# if !defined(_MIPS_ARCH_LOONGSON3A)

This workaround is not needed for other ABIs.

@@ +207,2 @@
>          if (kind == Double)
> +# endif

OK

@@ +209,1 @@
>              return FloatRegister(index * 2, RegType(kind));

OK

Comment 10

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS32-Do-not-allocate-odd-FP-registers-on.patch

Thanks!

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8674717 [details] [diff] [review]
0001-IonMonkey-MIPS32-Do-not-allocate-odd-FP-registers-on.patch

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

Thank you :D

::: js/src/jit/mips32/Architecture-mips32.h
@@ +53,5 @@
>      static Code FromName(const char* name);
>  
>      static const uint32_t Total = 64;
>      static const uint32_t TotalDouble = 16;
> +    static const uint32_t MaxRegisterId = 32;

as asserting |r.id() < FloatRegisters::MaxRegisterId| etc, maximum id is 31.  "RegisterIdLimit" or something like that would be better :)

Comment 12

[hev [:hev]]

(In reply to Tooru Fujisawa [:arai] from comment #11)
> Comment on attachment 8674717 [details] [diff] [review]
> 0001-IonMonkey-MIPS32-Do-not-allocate-odd-FP-registers-on.patch
> 
> Review of attachment 8674717 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you :D
> 
> ::: js/src/jit/mips32/Architecture-mips32.h
> @@ +53,5 @@
> >      static Code FromName(const char* name);
> >  
> >      static const uint32_t Total = 64;
> >      static const uint32_t TotalDouble = 16;
> > +    static const uint32_t MaxRegisterId = 32;
> 
> as asserting |r.id() < FloatRegisters::MaxRegisterId| etc, maximum id is 31.
> "RegisterIdLimit" or something like that would be better :)

Thanks! ;)

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd07e888b4f5

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dd07e888b4f5