1229821 - IonMonkey: MIPS: Fix merge macro assemblers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

Fix merge macroassember for mips in Bug 1181612.

Comment 1

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS-Fix-merge-macro-assemblers.patch

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8695093 [details] [diff] [review]
0001-IonMonkey-MIPS-Fix-merge-macro-assemblers.patch

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

Thank you!

::: js/src/jit/mips-shared/Assembler-mips-shared.cpp
@@ +96,5 @@
> +    for (size_t i = 0; i < other.numLongJumps(); i++) {
> +        size_t off = other.longJumps_[i];
> +        addLongJump(BufferOffset(size() + off));
> +    }
> +    return m_buffer.appendBuffer(other.m_buffer);

Appending the buffer at the end means we don't need to remember the former size, that's neat, nice.

::: js/src/jit/mips-shared/Assembler-mips-shared.h
@@ +650,5 @@
> +        if (this->oom())
> +            return false;
> +
> +        for (Slice* cur = other.head; cur != nullptr; cur = cur->getNext()) {
> +            this->putBytes(cur->length(), &cur->instructions[0]);

Could this just be:

this->putBytes(cur->length(), &cur->instructions);

by symmetry with the memcpy call in the method above?

::: js/src/jit/mips64/MacroAssembler-mips64.cpp
@@ +272,5 @@
>      ma_move(rd, rs);
>  }
>  
>  void
> +MacroAssemblerMIPS64::ma_li(Register dest, CodeOffset* label)

Could this method go inside MacroAssembler-mips-shared instead? (could be done in a follow-up bug, I suppose)

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb400a5c4de

Comment 4

[hev [:hev]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> ::: js/src/jit/mips-shared/Assembler-mips-shared.h
> @@ +650,5 @@
> > +        if (this->oom())
> > +            return false;
> > +
> > +        for (Slice* cur = other.head; cur != nullptr; cur = cur->getNext()) {
> > +            this->putBytes(cur->length(), &cur->instructions[0]);
> 
> Could this just be:
> 
> this->putBytes(cur->length(), &cur->instructions);
> 
> by symmetry with the memcpy call in the method above?
OK

> 
> ::: js/src/jit/mips64/MacroAssembler-mips64.cpp
> @@ +272,5 @@
> >      ma_move(rd, rs);
> >  }
> >  
> >  void
> > +MacroAssemblerMIPS64::ma_li(Register dest, CodeOffset* label)
> 
> Could this method go inside MacroAssembler-mips-shared instead? (could be
> done in a follow-up bug, I suppose)
If move ma_li(Register, CodeOffset*) to mips-shared, then need to move ma_li(Register, ImmWord) as pure virtual function, too. I think make ma_li as a virtual function to share just 3-line code is unworthy. What do you think?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Agreed. Thank you.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2cb400a5c4de