Closed Bug 1204422 Opened 4 years ago Closed 4 years ago

IonMonkey: MIPS32: Make more MacroAssembler functions can be shared

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: hev, Assigned: hev)

References

Details

Attachments

(1 file)

Replace arhcitecture specific functions by architecture-inedependent as more as possible. That'll easier to split shared code for MacroAssembler-mips32.
Comment on attachment 8660572 [details] [diff] [review]
0001-IonMonkey-MIPS32-Make-more-MacroAssembler-functions-.patch

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

::: js/src/jit/mips32/MacroAssembler-mips32.cpp
@@ +47,4 @@
>  void
>  MacroAssemblerMIPSCompat::convertInt32ToDouble(const Address& src, FloatRegister dest)
>  {
> +    ma_ls(dest, src);

load single float by ma_ls, that equal to old version.

@@ +54,5 @@
>  void
>  MacroAssemblerMIPSCompat::convertInt32ToDouble(const BaseIndex& src, FloatRegister dest)
>  {
> +    computeScaledAddress(src, ScratchRegister);
> +    convertInt32ToDouble(Address(ScratchRegister, src.offset), dest);

On MIPS64, use SecondScratchReg instead of ScratchRegister to avoid conflict.
I'll factor all ScratchRegister by ScratchRegisterScope in future.

::: js/src/jit/mips32/MacroAssembler-mips32.h
@@ +182,4 @@
>      void ma_b(Register lhs, Register rhs, Label* l, Condition c, JumpKind jumpKind = LongJump);
>      void ma_b(Register lhs, Imm32 imm, Label* l, Condition c, JumpKind jumpKind = LongJump);
>      void ma_b(Register lhs, ImmPtr imm, Label* l, Condition c, JumpKind jumpKind = LongJump) {
> +        ma_b(lhs, ImmWord(uintptr_t(imm.value)), l, c, jumpKind);

The ImmWord is 32-bit on mips32, and 64-bit on mips64. If we want to convert pointer to a Imm value on both platforms, use ImmWord may be better.

@@ +203,4 @@
>      void ma_b(Address addr, ImmGCPtr imm, Label* l, Condition c, JumpKind jumpKind = LongJump);
>      void ma_b(Address addr, Register rhs, Label* l, Condition c, JumpKind jumpKind = LongJump) {
>          MOZ_ASSERT(rhs != ScratchRegister);
> +        ma_load(ScratchRegister, addr, SizeWord);

I want to clean up ma_lw/ma_sw macro functions in future.

@@ +324,4 @@
>      void computeEffectiveAddress(const BaseIndex& address, Register dest) {
>          computeScaledAddress(address, dest);
>          if (address.offset) {
> +            addPtr(Imm32(address.offset), dest);

We can use addPtr/subPtr to compute pointer address on both platforms.

@@ +333,4 @@
>      }
>  
>      void mov(Register src, Register dest) {
> +        as_ori(dest, src, 0);

equal to old version.

@@ +337,3 @@
>      }
>      void mov(ImmWord imm, Register dest) {
> +        ma_li(dest, imm);

I have added ma_li(Label, ImmWord).

@@ +369,4 @@
>      }
>      void retn(Imm32 n) {
>          // pc <- [sp]; sp += n
> +        loadPtr(Address(StackPointer, 0), ra);

loadPtr/storePtr can uses in both platforms.

@@ +670,4 @@
>      }
>      void branchTestPtr(Condition cond, Register lhs, const Imm32 rhs, Label* label) {
> +        ma_li(ScratchRegister, rhs);
> +        branchTestPtr(cond, lhs, ScratchRegister, label);

On MIPS32 and MIPS64 platforms, the pointer are different size.
Comment on attachment 8660572 [details] [diff] [review]
0001-IonMonkey-MIPS32-Make-more-MacroAssembler-functions-.patch

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

Almost looks good.
r+ with minor change in |movePtr(ImmWord imm, Register dest)|.  I'm happy if you could answer following another questions.

::: js/src/jit/mips32/MacroAssembler-mips32.cpp
@@ +274,4 @@
>      // Thread the patch list through the unpatched address word in the
>      // instruction stream.
>      BufferOffset bo = m_buffer.nextOffset();
> +    ma_liPatchable(dest, ImmWord(label->prev()));

https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/js/src/jit/shared/Assembler-shared.h#399
>    int32_t prev() const {

AbsoluteLabel::prev() is int32_t on all arch.  Here ImmWord is used for performance reason on mips64, right?

@@ +943,4 @@
>          // Generate the long jump for calls because return address has to be
>          // the address after the reserved block.
>          addLongJump(nextOffset());
> +        ma_liPatchable(ScratchRegister, ImmWord(label->offset()));

https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/js/src/jit/Label.h?offset=0#36
>     int32_t offset() const {

Here too, LabelBase::offset() is also int32_t.

@@ +1565,4 @@
>  void
>  MacroAssemblerMIPSCompat::movePtr(ImmWord imm, Register dest)
>  {
> +    ma_li(dest, ImmWord(imm.value));

|ma_li(dest, imm)| should be equivalent.

@@ +1791,5 @@
>  
>  void
>  MacroAssemblerMIPSCompat::store32(Register src, const Address& address)
>  {
> +    ma_store(src, address, SizeWord);

This is arch-dependent method, right?  This will be valid only on mips32.

@@ +1798,5 @@
>  void
>  MacroAssemblerMIPSCompat::store32(Imm32 src, const Address& address)
>  {
>      move32(src, SecondScratchReg);
> +    ma_store(SecondScratchReg, address, SizeWord);

Here, too.

@@ +2685,4 @@
>          as_b(BOffImm16(2 * sizeof(uint32_t)));
>      }
>      // No need for nop here. We can safely put next instruction in delay slot.
> +    ma_liPatchable(ScratchRegister, ImmWord(dest));

dest is uint32_t.  This is also for performance on mips64, right?

@@ +2689,4 @@
>      MOZ_ASSERT(nextOffset().getOffset() - bo.getOffset() == 3 * sizeof(uint32_t));
>      as_jr(ScratchRegister);
>      // No need for nop here. We can safely put next instruction in delay slot.
> +    ma_liPatchable(ScratchRegister, ImmWord(dest));

Here too.

@@ +2706,4 @@
>      BufferOffset bo = nextOffset();
>      label->use(bo.getOffset());
>      addLongJump(bo);
> +    ma_liPatchable(ScratchRegister, ImmWord(dest));

and here
Attachment #8660572 - Flags: review?(arai.unmht) → review+
Comment on attachment 8660572 [details] [diff] [review]
0001-IonMonkey-MIPS32-Make-more-MacroAssembler-functions-.patch

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

Thanks for review.

::: js/src/jit/mips32/MacroAssembler-mips32.cpp
@@ +274,4 @@
>      // Thread the patch list through the unpatched address word in the
>      // instruction stream.
>      BufferOffset bo = m_buffer.nextOffset();
> +    ma_liPatchable(dest, ImmWord(label->prev()));

No, that will affect preformance on mips64. :(

The ma_liPatchable must be reserve constant instructions slot, to load larger value patched. on mips32, the pointer size is 32-bit, load a 32-bit immeidate number up to 2 instructions. but on mips64, needs up to 6 instructions to load a 64-bit immediate number.

Why load 32-bit patchable value by ImmWord? on mips32, that's no problem, because equivalent to Imm32. The patchable value should can be extract by ExtractLuiOriValue on mips32, and ExtractLoad64Value on mips64. These two functions can only extract fixed form instructions block, lui/ori for mips32, lui/ori/dsll/ori/dsll/ori for mips64. so, if ExtractLoad64Value try to extract a value that generated by ma_liPatchable(Imm32), that'll be failed.

I'll find a good way to fix, Thank you found the problem. ;)

@@ +1565,4 @@
>  void
>  MacroAssemblerMIPSCompat::movePtr(ImmWord imm, Register dest)
>  {
> +    ma_li(dest, ImmWord(imm.value));

You are right. ;)

@@ +1791,5 @@
>  
>  void
>  MacroAssemblerMIPSCompat::store32(Register src, const Address& address)
>  {
> +    ma_store(src, address, SizeWord);

You are right, I have not found a generic macro instruction to handle it.
ma_store(src, address, SizeDouble) on mips64.
You have SM(e) bustage from this push.
https://hg.mozilla.org/mozilla-central/rev/67d8993c8511
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.