1017539 - Move codegen for some LIR instructions to platform-independent code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

bz noticed LoadSlotV codegen is identical for all platforms. There are a number of other LIR instructions we can share as well.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - LoadSlotV

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - LoadSlotT

We're using masm.loadUnboxedValue for LoadFixedSlotT, we can also use it for LoadSlotT.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - LoadElementT cleanup

This patch uses masm.loadUnboxedValue for LoadElementT on x64, so that we can remove CodeGeneratorX64::loadUnboxedValue.

I also changed the x86 codegen to look more like the x64 one.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - LoadElementT

Especially the ARM and MIPS implementations were way more complicated than necessary.

 11 files changed, 37 insertions(+), 117 deletions(-)

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 5 - rm MImplicitThis/LImplicitThis

It's unused. There's JSOP_IMPLICITTHIS now but it's uncommon and if Ion ever has to compile it, it can be just a VM call.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 6 - InterruptCheck

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 7 - StoreSlotT

Also moves storeUnboxedValue to the macro assemblers and eliminates some code duplication with storeElementTyped.

Comment 8

[Sean Stangl [:sstangl]]

Comment on attachment 8430734 [details] [diff] [review]
Part 2 - LoadSlotT

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

::: js/src/jit/CodeGenerator.cpp
@@ +1561,5 @@
> +    Register base = ToRegister(lir->slots());
> +    int32_t offset = lir->mir()->slot() * sizeof(js::Value);
> +    AnyRegister result = ToAnyRegister(lir->output());
> +
> +    masm.loadUnboxedValue(Address(base, offset), lir->mir()->type(), result);

loadUnboxedValue() could easily be moved to the shared CodeGenerator also. Needs the LoadElementT patch to land first, though.

Comment 9

[Sean Stangl [:sstangl]]

Comment on attachment 8430789 [details] [diff] [review]
Part 4 - LoadElementT

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

::: js/src/jit/CodeGenerator.h
@@ +229,5 @@
>      bool visitThrow(LThrow *lir);
>      bool visitTypeOfV(LTypeOfV *lir);
>      bool visitOutOfLineTypeOfV(OutOfLineTypeOfV *ool);
>      bool visitToIdV(LToIdV *lir);
> +    template<typename T>

prefer vertical whitespace before the template<typename T> line. Or just put it on the same line as the function declaration.

Comment 10

[Sean Stangl [:sstangl]]

Comment on attachment 8430798 [details] [diff] [review]
Part 5 - rm MImplicitThis/LImplicitThis

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

\o/

Comment 11

[Sean Stangl [:sstangl]]

Comment on attachment 8430803 [details] [diff] [review]
Part 6 - InterruptCheck

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

::: js/src/jit/CodeGenerator.cpp
@@ +8832,5 @@
> +    if (!ool)
> +        return false;
> +
> +    masm.branch32(Assembler::NotEqual,
> +                  AbsoluteAddress(GetIonContext()->runtime->addressOfInterrupt()), Imm32(0),

If the AbsoluteAddress is declared on its own line and given a name, then the branch32() call need not span multiple lines.

Comment 12

[Sean Stangl [:sstangl]]

Comment on attachment 8430875 [details] [diff] [review]
Part 7 - StoreSlotT

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

::: js/src/jit/mips/CodeGenerator-mips.cpp
@@ -1752,5 @@
>  
>      masm.bind(&done);
>      return true;
>  }
>  

It looks like MacroAssembler-mips.cpp doesn't have storeUnboxedValue() defined for it, but that function is called by shared code.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Sean Stangl [:sstangl] from comment #12)
> It looks like MacroAssembler-mips.cpp doesn't have storeUnboxedValue()
> defined for it, but that function is called by shared code.

That was on purpose. MIPS is a tier-3 platform so I don't think we should spend too much time on it, especially if there's no way to verify our changes (I don't think the simulator landed completely).

I can copy paste some code and hope it works, or define the function and add a MOZ_CRASH()...

Comment 14

[Sean Stangl [:sstangl]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> (In reply to Sean Stangl [:sstangl] from comment #12)
> > It looks like MacroAssembler-mips.cpp doesn't have storeUnboxedValue()
> > defined for it, but that function is called by shared code.
> 
> That was on purpose. MIPS is a tier-3 platform so I don't think we should
> spend too much time on it, especially if there's no way to verify our
> changes (I don't think the simulator landed completely).
> 
> I can copy paste some code and hope it works, or define the function and add
> a MOZ_CRASH()...

Breaking compilation is probably preferable to MOZ_CRASH(), so I'll flip the bits.

Comment 15

[Jan de Mooij [:jandem]]

Thanks for the quick reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/163a0c70a67d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd78e3d6656
https://hg.mozilla.org/integration/mozilla-inbound/rev/b010c950836b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ec1d00485e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb1827f5f61
https://hg.mozilla.org/integration/mozilla-inbound/rev/b99f4cd25bad
https://hg.mozilla.org/integration/mozilla-inbound/rev/c970565582c2

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/163a0c70a67d
https://hg.mozilla.org/mozilla-central/rev/ddd78e3d6656
https://hg.mozilla.org/mozilla-central/rev/b010c950836b
https://hg.mozilla.org/mozilla-central/rev/f7ec1d00485e
https://hg.mozilla.org/mozilla-central/rev/7cb1827f5f61
https://hg.mozilla.org/mozilla-central/rev/b99f4cd25bad
https://hg.mozilla.org/mozilla-central/rev/c970565582c2