Closed Bug 1017539 Opened 5 years ago Closed 5 years ago

Move codegen for some LIR instructions to platform-independent code

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(7 files)

bz noticed LoadSlotV codegen is identical for all platforms. There are a number of other LIR instructions we can share as well.
Attachment #8430732 - Flags: review?(sstangl)
We're using masm.loadUnboxedValue for LoadFixedSlotT, we can also use it for LoadSlotT.
Attachment #8430734 - Flags: review?(sstangl)
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.
Attachment #8430754 - Flags: review?(sstangl)
Especially the ARM and MIPS implementations were way more complicated than necessary.

 11 files changed, 37 insertions(+), 117 deletions(-)
Attachment #8430789 - Flags: review?(sstangl)
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.
Attachment #8430798 - Flags: review?(sstangl)
Attachment #8430798 - Attachment description: rm MImplicitThis/LImplicitThis → Part 5 - rm MImplicitThis/LImplicitThis
Attachment #8430803 - Flags: review?(sstangl)
Also moves storeUnboxedValue to the macro assemblers and eliminates some code duplication with storeElementTyped.
Attachment #8430875 - Flags: review?(sstangl)
Attachment #8430732 - Flags: review?(sstangl) → review+
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.
Attachment #8430734 - Flags: review?(sstangl) → review+
Attachment #8430754 - Flags: review?(sstangl) → review+
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.
Attachment #8430789 - Flags: review?(sstangl) → review+
Comment on attachment 8430798 [details] [diff] [review]
Part 5 - rm MImplicitThis/LImplicitThis

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

\o/
Attachment #8430798 - Flags: review?(sstangl) → review+
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.
Attachment #8430803 - Flags: review?(sstangl) → review+
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.
Attachment #8430875 - Flags: review?(sstangl)
(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()...
(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.
Attachment #8430875 - Flags: review+
You need to log in before you can comment on or make changes to this bug.