Closed Bug 1205166 Opened 4 years ago Closed 4 years ago

IonMonkey: MIPS64: Import Architecture-mips64

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: hev, Assigned: hev)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1140954, Part 1: Architecture-mips64.
Attachment #8661624 - Flags: review?(nicolas.b.pierron)
Attachment #8661625 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8661624 - Flags: review?(nicolas.b.pierron)
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?
Attachment #8661625 - Flags: review?(nicolas.b.pierron)
(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?
(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.
Generated by "git format-patch -U10", thanks!
Attachment #8661624 - Attachment is obsolete: true
Attachment #8668316 - Flags: review?(nicolas.b.pierron)
Attachment #8668316 - Flags: review?(branislav.rankov)
Generated by "git format-patch -C -M", Thanks!
Attachment #8661625 - Attachment is obsolete: true
Attachment #8668317 - Flags: review?(nicolas.b.pierron)
Attachment #8668317 - Flags: review?(branislav.rankov)
Blocks: 1210322
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,

?
Attachment #8668316 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8668317 [details] [diff] [review]
Part 1.2: Import Architecture-mips64

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

Sounds good to me :)
Attachment #8668317 - Flags: review?(nicolas.b.pierron) → review+
(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 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?
Attachment #8668316 - Flags: feedback?(nicolas.b.pierron)
https://hg.mozilla.org/mozilla-central/rev/cfa811dbbaeb
https://hg.mozilla.org/mozilla-central/rev/80d22f41f35e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(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.
(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.
Attachment #8668316 - Flags: feedback?(nicolas.b.pierron)
You need to log in before you can comment on or make changes to this bug.