1205229 - IonMonkey: MIPS32: Make more CodeGenerator functions can be shared

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

Do that to avoid hard to review split CodeGenerator-mips32.

Comment 1

[hev [:hev]]

Attachment: Part 1: Add jump kind to branch32 and branchPtr with default LongJump

Thanks!

Comment 2

[hev [:hev]]

Attachment: Part 2: Make more CodeGenerator functions can be shared

Thanks!

Comment 3

[Tooru Fujisawa [:arai]]

Comment on attachment 8661712 [details] [diff] [review]
Part 1: Add jump kind to branch32 and branchPtr with default LongJump

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

:D

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8661712 [details] [diff] [review]
Part 1: Add jump kind to branch32 and branchPtr with default LongJump

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

These jump functions are part of the common interface of the MacroAssembler.
Adding default values here prevent merging these functions, while this feature would also benefit other architectures.

Comment 5

[hev [:hev]]

Comment on attachment 8661712 [details] [diff] [review]
Part 1: Add jump kind to branch32 and branchPtr with default LongJump

OK, I understand.

Comment 6

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS32-Make-more-CodeGenerator-functions-c.patch

At this stage I use long jump in bailoutCmp first. If anybody have good idea, please update it.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8661758 [details] [diff] [review]
0001-IonMonkey-MIPS32-Make-more-CodeGenerator-functions-c.patch

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

Please upload patches with more context.

::: js/src/jit/mips32/CodeGenerator-mips32.cpp
@@ +1756,5 @@
>      // Ensure that there is enough space in the buffer for the OsiPoint
>      // patching to occur. Otherwise, we could overwrite the invalidation
>      // epilogue.
> +    size_t existSize = masm.currentOffset() - returnLabel_.offset();
> +    if (Assembler::PatchWrite_NearCallSize() > existSize) {

No other architecture does that!

This literally saves nothing compare to the rest of the allocated code, I don't think this is worth the complexity.

Comment 8

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS32-Make-more-CodeGenerator-functions-c.patch

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8666547 [details] [diff] [review]
0001-IonMonkey-MIPS32-Make-more-CodeGenerator-functions-c.patch

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

::: js/src/jit/mips32/CodeGenerator-mips32.cpp
@@ +135,5 @@
>          emitBranch(ToRegister(comp->left()), ToRegister(comp->right()), cond,
>                     comp->ifTrue(), comp->ifFalse());
>      } else {
> +        masm.load32(ToAddress(comp->right()), ScratchRegister);
> +        emitBranch(ToRegister(comp->left()), ScratchRegister, cond,

This will not work on mips64, if we are comparing object pointers.
You should use comp->cmpMir->compareType() to decide what to do here.

@@ +278,5 @@
>      masm.ma_b(&done, ShortJump);
>  
>      // Check for zero.
>      masm.bind(&equal);
> +    masm.loadConstantFloat32(0.0f, ScratchFloat32Reg);

If you move these similar modification in another patch, I can r+ it.

Comment 10

[hev [:hev]]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> Comment on attachment 8666547 [details] [diff] [review]
> 0001-IonMonkey-MIPS32-Make-more-CodeGenerator-functions-c.patch
> 
> Review of attachment 8666547 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips32/CodeGenerator-mips32.cpp
> @@ +135,5 @@
> >          emitBranch(ToRegister(comp->left()), ToRegister(comp->right()), cond,
> >                     comp->ifTrue(), comp->ifFalse());
> >      } else {
> > +        masm.load32(ToAddress(comp->right()), ScratchRegister);
> > +        emitBranch(ToRegister(comp->left()), ScratchRegister, cond,
> 
> This will not work on mips64, if we are comparing object pointers.
> You should use comp->cmpMir->compareType() to decide what to do here.
That function need update for mips64. thank you! https://github.com/heiher/gecko-dev/blob/mips64-v6.3/js/src/jit/mips64/CodeGenerator-mips64.cpp#L139
> 
> @@ +278,5 @@
> >      masm.ma_b(&done, ShortJump);
> >  
> >      // Check for zero.
> >      masm.bind(&equal);
> > +    masm.loadConstantFloat32(0.0f, ScratchFloat32Reg);
> 
> If you move these similar modification in another patch, I can r+ it.
Got it. ;)

Comment 11

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS32-Make-more-CodeGenerator-functions-c.patch

Thanks!

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8667286 [details] [diff] [review]
0001-IonMonkey-MIPS32-Make-more-CodeGenerator-functions-c.patch

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

Thanks for the link to github.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15671d7b6087

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/15671d7b6087