1213746 - IonMonkey: MIPS64: Import MacroAssembler-mips64

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

Bug 1140954, Part 8: MacroAssembler-mips64.

Comment 2

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS64-Import-MacroAssembler-mips64.patch

Comment 3

[Lars T Hansen [:lth]]

Comment on attachment 8679871 [details] [diff] [review]
0001-IonMonkey-MIPS64-Import-MacroAssembler-mips64.patch

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

Tentative r+ from me, but with some concerns as noted below and I think those need to be addressed before this can land.  Feedback from nbp would be useful.

The sizes of certain instruction sequences seem to be "generally known" and show up as constants; I think this is not good and I've noted it several places but there are many more.  One could name the lengths of these sequences, and then always use an argument to ensureSpace that is the maximum of any of those named sizes (or perhaps there's a better solution, eg, functional abstraction).

I have not reviewed most instruction sequences here - I'm not sure what to do about that, it's a big project and requires MIPS expertise on a level I don't yet have.

::: js/src/jit/mips64/MacroAssembler-mips64.cpp
@@ +697,5 @@
> +    // Second word holds a pointer to the next branch in label's chain.
> +    uint32_t nextInChain = label->used() ? label->offset() : LabelBase::INVALID_OFFSET;
> +
> +    // Make the whole branch continous in the buffer.
> +    m_buffer.ensureSpace(6 * sizeof(uint32_t));

I know this "6" shows up here and there.  I don't know if it's the same "6" everywhere; naming it would be very helpful (and would probably allow us not to require the complicated expression here), eg, .ensureSpace(MIPS64_LONGJUMP_BYTESIZE), i'm not wedded to that naming.

@@ +740,5 @@
> +            return;
> +        }
> +
> +        // Handle long conditional branch
> +        writeInst(invertBranch(code, BOffImm16(7 * sizeof(uint32_t))).encode());

"7"??

@@ +769,5 @@
> +
> +    bool conditional = code.encode() != inst_beq.encode();
> +
> +    // Make the whole branch continous in the buffer.
> +    m_buffer.ensureSpace((conditional ? 7 : 6) * sizeof(uint32_t));

More constants, and more complication.

I assume you're doing this because these instruction sequences cannot be broken across buffer boundaries.  Perhaps a cleaner solution is to always use some reasonable but always large enough value, eg 16 instructions, and not worry about all the complicated side conditions.  (Just my opinion, but the complexity here invites brittleness.)

@@ +1286,5 @@
> +        bind(&negative);
> +        {
> +            ma_move(reg, zero);
> +        }
> +    }

Why do you have these scopes here (and in several of the subsequent methods)?  In many cases they don't have any locals and even when they do I'm not sure there's much point.

::: js/src/jit/mips64/MacroAssembler-mips64.h
@@ +138,5 @@
> +
> +    void ma_cmp_set(Register dst, Register lhs, ImmWord imm, Condition c);
> +    void ma_cmp_set(Register dst, Register lhs, ImmPtr imm, Condition c);
> +
> +    // These fuctions abstract the access to high part of the double precision

"functions"

@@ +139,5 @@
> +    void ma_cmp_set(Register dst, Register lhs, ImmWord imm, Condition c);
> +    void ma_cmp_set(Register dst, Register lhs, ImmPtr imm, Condition c);
> +
> +    // These fuctions abstract the access to high part of the double precision
> +    // float register. It is intended to work on both 32 bit and 64 bit

"They are", not "it is".

@@ +224,5 @@
> +    void mov(Register src, Address dest) {
> +        MOZ_CRASH("NYI-IC");
> +    }
> +    void mov(Address src, Register dest) {
> +        MOZ_CRASH("NYI-IC");

Implementations missing here?  If not then perhaps add a better comment.

@@ +260,5 @@
> +        // pc <- [sp]; sp += n
> +        loadPtr(Address(StackPointer, 0), ra);
> +        addPtr(n, StackPointer);
> +        as_jr(ra);
> +        as_nop();

Any reason the add can't be moved into the delay slot?

@@ +302,5 @@
> +    CodeOffsetLabel toggledCall(JitCode* target, bool enabled);
> +
> +    static size_t ToggledCallSize(uint8_t* code) {
> +        // Eight instructions used in: MacroAssemblerMIPS64Compat::toggledCall
> +        return 6 * sizeof(uint32_t);

Missing "." at the end of the comment.  Also, if the comment says "eight" why is the constant here "6"?  That should probably be commented on briefly, if correct, or fixed, if not.

@@ +1125,5 @@
> +
> +    // NOTE: This will use second scratch on MIPS64. Only ARM needs the
> +    // implementation without second scratch.
> +    void store32_NoSecondScratch(Imm32 src, const Address& address) {
> +        store32(src, address);

This is really just a bug waiting to happen.  This masm API is only needed one place in the common code, in GenerateProfilingPrologue, and the _NoSecondScratch qualifier is, as you say, only required for ARM.  The proper fix is at least arguably to expand the #ifdef at that call site so that the _NoSecondScratch API becomes an ARM-specific API, and then there's no need to lie here, or to implement the API on the other platforms.  Anyway, having an API here that does not do what it should do is not appropriate.

Comment 4

[hev [:hev]]

Comment on attachment 8679871 [details] [diff] [review]
0001-IonMonkey-MIPS64-Import-MacroAssembler-mips64.patch

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

Thank you Lars.

::: js/src/jit/mips64/MacroAssembler-mips64.cpp
@@ +697,5 @@
> +    // Second word holds a pointer to the next branch in label's chain.
> +    uint32_t nextInChain = label->used() ? label->offset() : LabelBase::INVALID_OFFSET;
> +
> +    // Make the whole branch continous in the buffer.
> +    m_buffer.ensureSpace(6 * sizeof(uint32_t));

OK

@@ +740,5 @@
> +            return;
> +        }
> +
> +        // Handle long conditional branch
> +        writeInst(invertBranch(code, BOffImm16(7 * sizeof(uint32_t))).encode());

OK, I will comment what's the 7. The '7' is target offset of this branch instruction and based on self. The target point is next instruction of nop at line 749.

@@ +769,5 @@
> +
> +    bool conditional = code.encode() != inst_beq.encode();
> +
> +    // Make the whole branch continous in the buffer.
> +    m_buffer.ensureSpace((conditional ? 7 : 6) * sizeof(uint32_t));

OK, keep it simple stupid. ;)

@@ +1286,5 @@
> +        bind(&negative);
> +        {
> +            ma_move(reg, zero);
> +        }
> +    }

Let me think how to keep it simple.

::: js/src/jit/mips64/MacroAssembler-mips64.h
@@ +138,5 @@
> +
> +    void ma_cmp_set(Register dst, Register lhs, ImmWord imm, Condition c);
> +    void ma_cmp_set(Register dst, Register lhs, ImmPtr imm, Condition c);
> +
> +    // These fuctions abstract the access to high part of the double precision

ok

@@ +139,5 @@
> +    void ma_cmp_set(Register dst, Register lhs, ImmWord imm, Condition c);
> +    void ma_cmp_set(Register dst, Register lhs, ImmPtr imm, Condition c);
> +
> +    // These fuctions abstract the access to high part of the double precision
> +    // float register. It is intended to work on both 32 bit and 64 bit

ok

@@ +224,5 @@
> +    void mov(Register src, Address dest) {
> +        MOZ_CRASH("NYI-IC");
> +    }
> +    void mov(Address src, Register dest) {
> +        MOZ_CRASH("NYI-IC");

Sorry, I don't known. On ARM, these are NYI, too. Nicolas, Could you tell me what's these?

@@ +260,5 @@
> +        // pc <- [sp]; sp += n
> +        loadPtr(Address(StackPointer, 0), ra);
> +        addPtr(n, StackPointer);
> +        as_jr(ra);
> +        as_nop();

Good catch!

@@ +302,5 @@
> +    CodeOffsetLabel toggledCall(JitCode* target, bool enabled);
> +
> +    static size_t ToggledCallSize(uint8_t* code) {
> +        // Eight instructions used in: MacroAssemblerMIPS64Compat::toggledCall
> +        return 6 * sizeof(uint32_t);

Is my mistake. :( At the beginning, The ma_liPatchable is only supported load imm64 by 6 instructions. then Li48 added for load address by 4 instructions.

@@ +1125,5 @@
> +
> +    // NOTE: This will use second scratch on MIPS64. Only ARM needs the
> +    // implementation without second scratch.
> +    void store32_NoSecondScratch(Imm32 src, const Address& address) {
> +        store32(src, address);

You are right, I think that the _NoSecondScratch API becoms an ARM-specific API better than implements it on all platforms.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Heiher [:hev] from comment #4)
> @@ +224,5 @@
> > +    void mov(Register src, Address dest) {
> > +        MOZ_CRASH("NYI-IC");
> > +    }
> > +    void mov(Address src, Register dest) {
> > +        MOZ_CRASH("NYI-IC");
> 
> Sorry, I don't known. On ARM, these are NYI, too. Nicolas, Could you tell me
> what's these?

Hum … That's not a recent addition for sure.  This even pre-dates the release of IonMonkey, and this code is dead since the beginning and got added as part of Bug 707854 at the time when we had no ARM parity.

Let's remove some code!

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8679871 [details] [diff] [review]
0001-IonMonkey-MIPS64-Import-MacroAssembler-mips64.patch

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

(Skimming through the files did not make me notice anything unexpected)

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b585ab78b5

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a1b585ab78b5