Closed Bug 1280843 Opened 3 years ago Closed 3 years ago

IonMonkey: MIPS: Fix ma_b(Register, T, JumpTarget) for Wasm

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: hev, Assigned: hev)

Details

Attachments

(1 file, 2 obsolete files)

Fix wasm/basic.js fail.
Attachment #8763429 - Flags: review?(hwjeastd07)
Comment on attachment 8763429 [details] [diff] [review]
0001-Bug-1280843-IonMonkey-MIPS-Fix-ma_b-Register-T-JumpT.patch

Handle long jump in Assembler::bind(RepatchLabel*) to bind branch32(..., wasm::JumpTarget).
https://github.com/heiher/gecko-dev/blob/mips-dev/js/src/asmjs/WasmFrameIterator.cpp#L352
Attachment #8763429 - Flags: review?(arai.unmht)
Comment on attachment 8763429 [details] [diff] [review]
0001-Bug-1280843-IonMonkey-MIPS-Fix-ma_b-Register-T-JumpT.patch

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

I'm not capable of reviewing patching things.  forwarding to nbp :)
Attachment #8763429 - Flags: review?(arai.unmht) → review?(nicolas.b.pierron)
Comment on attachment 8763429 [details] [diff] [review]
0001-Bug-1280843-IonMonkey-MIPS-Fix-ma_b-Register-T-JumpT.patch

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

I would highly appreciate if you can give pointers to the MIPS instructions which are being replaced, ideally by using assertions and comments to tells code-readers which function are producing these patterns.

::: js/src/jit/mips32/Assembler-mips32.cpp
@@ +331,5 @@
>              MOZ_ASSERT(BOffImm16::IsInRange(offset));
> +            inst[0].setBOffImm16(BOffImm16(offset));
> +        } else if (inst[0].encode() == inst_beq.encode()) {
> +            // Handle long unconditional jump.
> +            addLongJump(BufferOffset(label->offset()));

This is supposed to be called at the time of the creation of the jump, any reason to call it after?
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Comment on attachment 8763429 [details] [diff] [review]
> 0001-Bug-1280843-IonMonkey-MIPS-Fix-ma_b-Register-T-JumpT.patch
> 
> Review of attachment 8763429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would highly appreciate if you can give pointers to the MIPS instructions
> which are being replaced, ideally by using assertions and comments to tells
> code-readers which function are producing these patterns.
> 
> ::: js/src/jit/mips32/Assembler-mips32.cpp
> @@ +331,5 @@
> >              MOZ_ASSERT(BOffImm16::IsInRange(offset));
> > +            inst[0].setBOffImm16(BOffImm16(offset));
> > +        } else if (inst[0].encode() == inst_beq.encode()) {
> > +            // Handle long unconditional jump.
> > +            addLongJump(BufferOffset(label->offset()));
> 
> This is supposed to be called at the time of the creation of the jump, any
> reason to call it after?

Thank you. This is a open long jump that generated by MacroAssemblerMIPSShared::ma_b(..., wasm::JumpTarget, ...). It doesn't called at the time of the creation of the jump.
See: https://github.com/heiher/gecko-dev/blob/98d7a9d9c504bb919595df5af518d70a7b66c3fc/js/src/jit/mips64/MacroAssembler-mips64.cpp#L755
Attachment #8763429 - Attachment is obsolete: true
Attachment #8763429 - Flags: review?(nicolas.b.pierron)
Attachment #8763429 - Flags: review?(hwjeastd07)
Attachment #8765705 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8765705 [details] [diff] [review]
0001-Bug-1280843-IonMonkey-MIPS-Fix-ma_b-Register-T-JumpT.patch

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

Sorry for the huge pile of comments, but this code is really hard to follow.  As branchWithCode can produce branches which are flowing into this ::bind function (which is really badly named for grepping), I suggest this code should be armored to ensure that only unbounded & unused labels & branchWithCode are used as input of this function, plus the jumpWithPatch & backedgeJump that already exists.

PS: I was not aware of wasm hacks, which reconstructs RepatchLabel after the fact in [1] from a collection build with bindLater function calls, and never appeared in my searches as I was only looking under js/src/jit/ directory.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/asmjs/WasmGenerator.cpp#275-278

::: js/src/jit/mips32/Assembler-mips32.cpp
@@ +322,5 @@
> +        // If first instruction is lui, then this is a long jump.
> +        // If second instruction is lui, then this is a loop backedge.
> +        if (inst[0].extractOpcode() == ((uint32_t)op_lui >> OpcodeShift)) {
> +            // For MacroAssemblerMIPSCompat::jumpWithPatch. It's long jump
> +            // and already add to long jumps array.

nit:

// For unconditional branches generated by ma_liPatchable,
// such as under:
//     jumpWithPatch
//     branchWithCode
MOZ_ASSERT(inst[0].extractOpcode() == (uint32_t(op_lui) >> OpcodeShift));
MOZ_ASSERT(inst[1].extractOpcode() == (uint32_t(op_ori) >> OpcodeShift));
MOZ_ASSERT(inst[2].extractOpcode() == (uint32_t(op_dsll) >> OpcodeShift) ||
           inst[2].extractOpcode() == (uint32_t(op_drotr32) >> OpcodeShift);
MOZ_ASSERT_IF(inst[2].extractOpcode() == (uint32_t(op_dsll) >> OpcodeShift) ||
              inst[3].extractOpcode() == …);
…
MOZ_ASSERT_IF(inst[2].extractOpcode() == (uint32_t(op_dsll) >> OpcodeShift) ||
              inst[6].extractOpcode() == (uint32_t(op_jr) >> OpcodeShift));

@@ +324,5 @@
> +        if (inst[0].extractOpcode() == ((uint32_t)op_lui >> OpcodeShift)) {
> +            // For MacroAssemblerMIPSCompat::jumpWithPatch. It's long jump
> +            // and already add to long jumps array.
> +            Assembler::UpdateLuiOriValue(inst, inst->next(), dest.getOffset());
> +        } else if (inst[1].extractOpcode() == ((uint32_t)op_lui >> OpcodeShift) ||

nit: Use C++ cast when possible:
   (uint32_t) …  --->   uint32_t(…)

@@ +325,5 @@
> +            // For MacroAssemblerMIPSCompat::jumpWithPatch. It's long jump
> +            // and already add to long jumps array.
> +            Assembler::UpdateLuiOriValue(inst, inst->next(), dest.getOffset());
> +        } else if (inst[1].extractOpcode() == ((uint32_t)op_lui >> OpcodeShift) ||
> +                   BOffImm16::IsInRange(offset))

This condition can match bound long branches produced by branchWithCode, which have an inverted condition made to skip over the unconditional jump.

Do we have to assert that if the code that we are patching comes from branchWithCode, then this is an unbounded branch, with no uses?

@@ +331,2 @@
>              // Backedges are short jumps when bound, but can become long
>              // when patched.

nit:

// Handle code produced by:
//   backedgeJump

@@ +333,2 @@
>              MOZ_ASSERT(BOffImm16::IsInRange(offset));
> +            inst[0].setBOffImm16(BOffImm16(offset));

nit: MOZ_ASSERT(inst[0]->extractOpcode() == ((uint32_t(op_beq) >> OpcodeShift));  ?

@@ +335,5 @@
> +        } else if (inst[0].encode() == inst_beq.encode()) {
> +            // Handle open long unconditional jumps created by
> +            // MacroAssemblerMIPSShared::ma_b(..., wasm::JumpTarget, ...).
> +            // We need to add it to long jumps array here.
> +            // See MacroAssemblerMIPS::branchWithCode().

This condition also matches  backedgeJump  code.

@@ +339,5 @@
> +            // See MacroAssemblerMIPS::branchWithCode().
> +            addLongJump(BufferOffset(label->offset()));
> +            Assembler::WriteLuiOriInstructions(inst, &inst[1], ScratchRegister, dest.getOffset());
> +            inst[2] = InstReg(op_special, ScratchRegister, zero, zero, ff_jr).encode();
> +            // There is 1 nop after this.

1.  This is only holds if "MacroAssemblerMIPSShared::ma_b(… wasm::JumpTarget target …)" never uses short jumps, and the previous condition does not prevent us from attempting to patch short-jump branches.
I suggest that we remove the JumpKind argument of this function, such that we only generate patchable branches.

2.  Also, assert that the opcode of inst[3] is a nop.  (Note: I do not think this is true when we are patching code emmitted by backedgeJump, as the delay slot is a lui instruiction.)

@@ +350,5 @@
> +            // See MacroAssemblerMIPS::branchWithCode().
> +            addLongJump(BufferOffset(label->offset() + sizeof(void*)));
> +            Assembler::WriteLuiOriInstructions(&inst[1], &inst[2], ScratchRegister, dest.getOffset());
> +            inst[3] = InstReg(op_special, ScratchRegister, zero, zero, ff_jr).encode();
> +            // There is 1 nop after this.

Assert that the opcode of inst[{2,3}] are nops, and that inst[1] is equal to LabelBase::INVALID_OFFSET.
Attachment #8765705 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Comment on attachment 8765705 [details] [diff] [review]
> 0001-Bug-1280843-IonMonkey-MIPS-Fix-ma_b-Register-T-JumpT.patch
> 
> Review of attachment 8765705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the huge pile of comments, but this code is really hard to follow.
> As branchWithCode can produce branches which are flowing into this ::bind
> function (which is really badly named for grepping), I suggest this code
> should be armored to ensure that only unbounded & unused labels &
> branchWithCode are used as input of this function, plus the jumpWithPatch &
> backedgeJump that already exists.
> 
> PS: I was not aware of wasm hacks, which reconstructs RepatchLabel after the
> fact in [1] from a collection build with bindLater function calls, and never
> appeared in my searches as I was only looking under js/src/jit/ directory.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/js/src/asmjs/WasmGenerator.
> cpp#275-278
> 
> ::: js/src/jit/mips32/Assembler-mips32.cpp
> @@ +322,5 @@
> > +        // If first instruction is lui, then this is a long jump.
> > +        // If second instruction is lui, then this is a loop backedge.
> > +        if (inst[0].extractOpcode() == ((uint32_t)op_lui >> OpcodeShift)) {
> > +            // For MacroAssemblerMIPSCompat::jumpWithPatch. It's long jump
> > +            // and already add to long jumps array.
> 
> nit:
> 
> // For unconditional branches generated by ma_liPatchable,
> // such as under:
> //     jumpWithPatch
> //     branchWithCode
> MOZ_ASSERT(inst[0].extractOpcode() == (uint32_t(op_lui) >> OpcodeShift));
> MOZ_ASSERT(inst[1].extractOpcode() == (uint32_t(op_ori) >> OpcodeShift));
> MOZ_ASSERT(inst[2].extractOpcode() == (uint32_t(op_dsll) >> OpcodeShift) ||
>            inst[2].extractOpcode() == (uint32_t(op_drotr32) >> OpcodeShift);
> MOZ_ASSERT_IF(inst[2].extractOpcode() == (uint32_t(op_dsll) >> OpcodeShift)
> ||
>               inst[3].extractOpcode() == …);
> …
> MOZ_ASSERT_IF(inst[2].extractOpcode() == (uint32_t(op_dsll) >> OpcodeShift)
> ||
>               inst[6].extractOpcode() == (uint32_t(op_jr) >> OpcodeShift));
> 
I think we don't need redundancy asserations here, because that alreay exists in UpdateLuiOriValue.

> @@ +324,5 @@
> > +        if (inst[0].extractOpcode() == ((uint32_t)op_lui >> OpcodeShift)) {
> > +            // For MacroAssemblerMIPSCompat::jumpWithPatch. It's long jump
> > +            // and already add to long jumps array.
> > +            Assembler::UpdateLuiOriValue(inst, inst->next(), dest.getOffset());
> > +        } else if (inst[1].extractOpcode() == ((uint32_t)op_lui >> OpcodeShift) ||
> 
> nit: Use C++ cast when possible:
>    (uint32_t) …  --->   uint32_t(…)
okay.

> 
> @@ +325,5 @@
> > +            // For MacroAssemblerMIPSCompat::jumpWithPatch. It's long jump
> > +            // and already add to long jumps array.
> > +            Assembler::UpdateLuiOriValue(inst, inst->next(), dest.getOffset());
> > +        } else if (inst[1].extractOpcode() == ((uint32_t)op_lui >> OpcodeShift) ||
> > +                   BOffImm16::IsInRange(offset))
> 
> This condition can match bound long branches produced by branchWithCode,
> which have an inverted condition made to skip over the unconditional jump.
> 
> Do we have to assert that if the code that we are patching comes from
> branchWithCode, then this is an unbounded branch, with no uses?
> 
Sorry, I don't know why the bound labels bind again here?

> @@ +331,2 @@
> >              // Backedges are short jumps when bound, but can become long
> >              // when patched.
> 
> nit:
> 
> // Handle code produced by:
> //   backedgeJump
> 
> @@ +333,2 @@
> >              MOZ_ASSERT(BOffImm16::IsInRange(offset));
> > +            inst[0].setBOffImm16(BOffImm16(offset));
> 
> nit: MOZ_ASSERT(inst[0]->extractOpcode() == ((uint32_t(op_beq) >>
> OpcodeShift));  ?
Yes, but not only. and op_bne/op_blez/op_bgtz.

> 
> @@ +335,5 @@
> > +        } else if (inst[0].encode() == inst_beq.encode()) {
> > +            // Handle open long unconditional jumps created by
> > +            // MacroAssemblerMIPSShared::ma_b(..., wasm::JumpTarget, ...).
> > +            // We need to add it to long jumps array here.
> > +            // See MacroAssemblerMIPS::branchWithCode().
> 
> This condition also matches  backedgeJump  code.
No, The inst[1] of backedgeJump is lui, so matched by previous condition. also, backedges are short jumps when bound.

> 
> @@ +339,5 @@
> > +            // See MacroAssemblerMIPS::branchWithCode().
> > +            addLongJump(BufferOffset(label->offset()));
> > +            Assembler::WriteLuiOriInstructions(inst, &inst[1], ScratchRegister, dest.getOffset());
> > +            inst[2] = InstReg(op_special, ScratchRegister, zero, zero, ff_jr).encode();
> > +            // There is 1 nop after this.
> 
> 1.  This is only holds if "MacroAssemblerMIPSShared::ma_b(… wasm::JumpTarget
> target …)" never uses short jumps, and the previous condition does not
> prevent us from attempting to patch short-jump branches.
> I suggest that we remove the JumpKind argument of this function, such that
> we only generate patchable branches.
I think always uses patchable branches caused the performance. :(

> 
> 2.  Also, assert that the opcode of inst[3] is a nop.  (Note: I do not think
> this is true when we are patching code emmitted by backedgeJump, as the
> delay slot is a lui instruiction.)
No, I think the backedgeJump only matched by previous condition.

> 
> @@ +350,5 @@
> > +            // See MacroAssemblerMIPS::branchWithCode().
> > +            addLongJump(BufferOffset(label->offset() + sizeof(void*)));
> > +            Assembler::WriteLuiOriInstructions(&inst[1], &inst[2], ScratchRegister, dest.getOffset());
> > +            inst[3] = InstReg(op_special, ScratchRegister, zero, zero, ff_jr).encode();
> > +            // There is 1 nop after this.
> 
> Assert that the opcode of inst[{2,3}] are nops, and that inst[1] is equal to
> LabelBase::INVALID_OFFSET.
inst[{2,3}] are nops here, I think inst[1] is nop too.

https://dxr.mozilla.org/mozilla-central/source/js/src/jit/mips-shared/Assembler-mips-shared.cpp#1565-1582
Do you think this is ok?
Attachment #8765705 - Attachment is obsolete: true
Attachment #8767013 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8767013 [details] [diff] [review]
0001-Bug-1280843-IonMonkey-MIPS-Fix-ma_b-Register-T-JumpT.patch

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

Thanks, with these assertions, I can safely r+ this code without having to dig for hours in the rest of MIPS backends. :)
Attachment #8767013 - Flags: review?(nicolas.b.pierron) → review+
Pushed by r@hev.cc:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd38bf78b58
IonMonkey: MIPS: Fix ma_b(Register, T, JumpTarget) for Wasm. r=nbp
https://hg.mozilla.org/mozilla-central/rev/ebd38bf78b58
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.