Closed
Bug 1280843
Opened 8 years ago
Closed 8 years ago
IonMonkey: MIPS: Fix ma_b(Register, T, JumpTarget) for Wasm
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: hev, Assigned: hev)
Details
Attachments
(1 file, 2 obsolete files)
8.22 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
Fix wasm/basic.js fail.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8763429 -
Flags: review?(hwjeastd07)
Assignee | ||
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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 4•8 years ago
|
||
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?
Assignee | ||
Comment 5•8 years ago
|
||
(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
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
(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
Assignee | ||
Comment 9•8 years ago
|
||
Do you think this is ok?
Attachment #8765705 -
Attachment is obsolete: true
Attachment #8767013 -
Flags: review?(nicolas.b.pierron)
Comment 10•8 years ago
|
||
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+
Comment 11•8 years ago
|
||
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
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ebd38bf78b58
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•