1205166 - IonMonkey: MIPS64: Import Architecture-mips64

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

Bug 1140954, Part 1: Architecture-mips64.

Comment 1

[hev [:hev]]

Attachment: Part 1.1: Import MIPS64 support into Architecture-mips-shared

Comment 2

[hev [:hev]]

Attachment: Part 1.2: Import Architecture-mips64

Comment 3

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8661624 [details] [diff] [review]
Part 1.1: Import MIPS64 support into Architecture-mips-shared

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

Please, re-upload this patch with more context.
And ask rankov for review too.

Comment 4

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8661625 [details] [diff] [review]
Part 1.2: Import Architecture-mips64

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

Can you upload a "git show -C -M" version of this patch?

::: js/src/jit/mips64/Architecture-mips64.h
@@ +40,5 @@
> +static const uint32_t ShadowStackSpace = 0;
> +
> +// MIPS64 have 64 bit floating-point coprocessor. There are 32 double
> +// precision register which can also be used as single precision registers.
> +class FloatRegisters : public BaseFloatRegisters

What is Base?  Did you mean Shared, or MIPSShared?

Comment 5

[hev [:hev]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Comment on attachment 8661624 [details] [diff] [review]
> Part 1.1: Import MIPS64 support into Architecture-mips-shared
> 
> Review of attachment 8661624 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please, re-upload this patch with more context.
> And ask rankov for review too.
OK.

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Comment on attachment 8661625 [details] [diff] [review]
> Part 1.2: Import Architecture-mips64
> 
> Review of attachment 8661625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you upload a "git show -C -M" version of this patch?
OK.
> 
> ::: js/src/jit/mips64/Architecture-mips64.h
> @@ +40,5 @@
> > +static const uint32_t ShadowStackSpace = 0;
> > +
> > +// MIPS64 have 64 bit floating-point coprocessor. There are 32 double
> > +// precision register which can also be used as single precision registers.
> > +class FloatRegisters : public BaseFloatRegisters
> 
> What is Base?  Did you mean Shared, or MIPSShared?
Right, should I move BaseFloatRegisters to FloatRegistersMIPSShared in mips-shared first?

Comment 6

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Heiher [:hev] from comment #5)
> (In reply to Nicolas B. Pierron [:nbp] from comment #4)
> > Comment on attachment 8661625 [details] [diff] [review]
> > Part 1.2: Import Architecture-mips64
> > 
> > Review of attachment 8661625 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: js/src/jit/mips64/Architecture-mips64.h
> > @@ +40,5 @@
> > > +class FloatRegisters : public BaseFloatRegisters
> > 
> > What is Base?  Did you mean Shared, or MIPSShared?
> Right, should I move BaseFloatRegisters to FloatRegistersMIPSShared in
> mips-shared first?

I am fine if you do that in a follow-up patch.

Comment 7

[hev [:hev]]

Attachment: Part 1.1: Import MIPS64 support into Architecture-mips-shared

Generated by "git format-patch -U10", thanks!

Comment 8

[hev [:hev]]

Attachment: Part 1.2: Import Architecture-mips64

Generated by "git format-patch -C -M", Thanks!

Comment 9

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8668316 [details] [diff] [review]
Part 1.1: Import MIPS64 support into Architecture-mips-shared

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

::: js/src/jit/mips-shared/Architecture-mips-shared.h
@@ +99,5 @@
> +        t3 = r15,
> +        ta0 = a4,
> +        ta1 = a5,
> +        ta2 = a6,
> +        ta3 = a7,

Would it be better to move these to the same lines as the original registers?

  a4 = r8, ta0 = a4,

?

Comment 10

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8668317 [details] [diff] [review]
Part 1.2: Import Architecture-mips64

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

Sounds good to me :)

Comment 11

[hev [:hev]]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> Comment on attachment 8668316 [details] [diff] [review]
> Part 1.1: Import MIPS64 support into Architecture-mips-shared
> 
> Review of attachment 8668316 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips-shared/Architecture-mips-shared.h
> @@ +99,5 @@
> > +        t3 = r15,
> > +        ta0 = a4,
> > +        ta1 = a5,
> > +        ta2 = a6,
> > +        ta3 = a7,
> 
> Would it be better to move these to the same lines as the original registers?
> 
>   a4 = r8, ta0 = a4,
> 
> ?

I think assign alias to taX registers might be better than original, because we can clear to known which args/temp register are point to.

Comment 12

[hev [:hev]]

Comment on attachment 8668316 [details] [diff] [review]
Part 1.1: Import MIPS64 support into Architecture-mips-shared

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

::: js/src/jit/mips-shared/Architecture-mips-shared.h
@@ +24,1 @@
>  #define USES_O32_ABI

Shall we clean up USES_XXX_ABI, just use JS_CODEGEN_MIPS32/64?

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa811dbbaeb
https://hg.mozilla.org/integration/mozilla-inbound/rev/80d22f41f35e

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cfa811dbbaeb
https://hg.mozilla.org/mozilla-central/rev/80d22f41f35e

Comment 15

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Heiher [:hev] from comment #12)
> ::: js/src/jit/mips-shared/Architecture-mips-shared.h
> @@ +24,1 @@
> >  #define USES_O32_ABI
> 
> Shall we clean up USES_XXX_ABI, just use JS_CODEGEN_MIPS32/64?

From what I understand we could also have N32 abi one day, so I am not completely sure we want to use the JS_CODEGEN_MIPS32 macro here.

Comment 16

[hev [:hev]]

(In reply to Nicolas B. Pierron [:nbp] from comment #15)
> (In reply to Heiher [:hev] from comment #12)
> > ::: js/src/jit/mips-shared/Architecture-mips-shared.h
> > @@ +24,1 @@
> > >  #define USES_O32_ABI
> > 
> > Shall we clean up USES_XXX_ABI, just use JS_CODEGEN_MIPS32/64?
> 
> From what I understand we could also have N32 abi one day, so I am not
> completely sure we want to use the JS_CODEGEN_MIPS32 macro here.

Yes, right. The N32 abi is a subset of mips64. I think USES_O32_ABI might be unnecessary at least. But now it doesn't matter.