Closed Bug 1248859 Opened 4 years ago Closed 4 years ago

OdinMonkey: MIPS: Fix jump related methods for wasm.

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: hev, Assigned: hev)

References

Details

Attachments

(3 files)

Comment on attachment 8720140 [details] [diff] [review]
0002-OdinMonkey-MIPS-Implement-thunkWithPatch-and-re-patc.patch

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

::: js/src/jit/mips-shared/Architecture-mips-shared.h
@@ +31,5 @@
>  namespace js {
>  namespace jit {
>  
> +// How far forward/back can a jump go? Provide a generous buffer for thunks.
> +static const uint32_t JumpImmediateRange = UINT32_MAX;

The goal of this immediate is to describe the jump range of a normal (local) masm.jump or masm.call.  Do these really have a UINT32_MAX range?  (Maybe this is the masm.longJumps?)

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +1184,4 @@
>  CodeOffset
>  MacroAssembler::thunkWithPatch()
>  {
> +    addLongJump(nextOffset());

Does this really need to addLongJump() given that the immediate is patched manually via (re)patchThunk?
(In reply to Luke Wagner [:luke] from comment #3)
> Comment on attachment 8720140 [details] [diff] [review]
> 0002-OdinMonkey-MIPS-Implement-thunkWithPatch-and-re-patc.patch
> 
> Review of attachment 8720140 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips-shared/Architecture-mips-shared.h
> @@ +31,5 @@
> >  namespace js {
> >  namespace jit {
> >  
> > +// How far forward/back can a jump go? Provide a generous buffer for thunks.
> > +static const uint32_t JumpImmediateRange = UINT32_MAX;
> 
> The goal of this immediate is to describe the jump range of a normal (local)
> masm.jump or masm.call.  Do these really have a UINT32_MAX range?  (Maybe
> this is the masm.longJumps?)
Yes. Currently callWithPatch is a long jumps. The jump range is full memory address space.

> 
> ::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
> @@ +1184,4 @@
> >  CodeOffset
> >  MacroAssembler::thunkWithPatch()
> >  {
> > +    addLongJump(nextOffset());
> 
> Does this really need to addLongJump() given that the immediate is patched
> manually via (re)patchThunk?
Yes. thunkWithPatch is completely equivalent to callWithPatch now. On MIPS, a simple relative call/jump's max range is 32k. so, thunkWithPatch implement by long jump.

If I understand the code correctly, In wasm, first compare the jump is out of range with JumpImmediateRange, If out of range, jump via thunkWithPatch, else via normal jump (callWithPatch). right? If true, I will try to update callWithPatch via a 32k relative jump instruction.
Flags: needinfo?(luke)
Oops, max range is +/-128k.
Comment on attachment 8720139 [details] [diff] [review]
0001-OdinMonkey-MIPS-Fix-replace-retargetWithOffset.patch

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

Thank you :D

::: js/src/jit/mips-shared/Assembler-mips-shared.cpp
@@ +1340,5 @@
> +            next = inst[1].encode();
> +            inst[1].makeNop();
> +
> +            b = BufferOffset(next);
> +        } while (next != LabelBase::INVALID_OFFSET);

can't this be separated into nextLink or nextJump method like arm/x86-shared?
so it could become easy to apply further change to all archs at once.
Attachment #8720139 - Flags: review?(arai.unmht) → review+
(In reply to Heiher [:hev] from comment #4)
> If I understand the code correctly, In wasm, first compare the jump is out
> of range with JumpImmediateRange, If out of range, jump via thunkWithPatch,
> else via normal jump (callWithPatch). right? If true, I will try to update
> callWithPatch via a 32k relative jump instruction.

To be clear, this thunk is used for both callWithPatch and jump(wasm::JumpTarget), but you're right for the call case.  Both of these are on somewhat hot paths (the jumps are conditional and almost never taken) so my goal with the recent change was for them to be a single instruction.

So the normal MIPS jump range is 32k, huh.  Hrm.  My concern is that if a single function is bigger than 32k than even the thunk may be out of range.  On ARM, the range is 32m (at which point *all* masm.jump()/Label stop working so everything breaks down) so we just assume the whole function is < 32m.  Does MIPS limit individual function sizes to 32k or does it do something fancier for plain intra-function jumps?
Flags: needinfo?(luke)
Comment on attachment 8720300 [details] [diff] [review]
0003-OdinMonkey-MIPS-Refactor-callWithPatch-via-reative-b.patch

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

Awesome!  Are there any other long jumps emitted for asm.js?  If not, could you remove the #ifdef MIPS longJump loop in ModuleGenerator::finishStaticLinkData in WasmGenerator.cpp?
Attachment #8720300 - Flags: review?(luke) → review+
Comment on attachment 8720140 [details] [diff] [review]
0002-OdinMonkey-MIPS-Implement-thunkWithPatch-and-re-patc.patch

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

::: js/src/jit/mips-shared/Architecture-mips-shared.h
@@ +31,5 @@
>  namespace js {
>  namespace jit {
>  
> +// How far forward/back can a jump go? Provide a generous buffer for thunks.
> +static const uint32_t JumpImmediateRange = UINT32_MAX;

The goal of this immediate is to describe the jump range of a normal (local) masm.jump or masm.call.  Do these really have a UINT32_MAX range?  (Maybe this is the masm.longJumps?)

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +1184,4 @@
>  CodeOffset
>  MacroAssembler::thunkWithPatch()
>  {
> +    addLongJump(nextOffset());

Does this really need to addLongJump() given that the immediate is patched manually via (re)patchThunk?
Attachment #8720140 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #10)
> Comment on attachment 8720300 [details] [diff] [review]
> 0003-OdinMonkey-MIPS-Refactor-callWithPatch-via-reative-b.patch
> 
> Review of attachment 8720300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome!  Are there any other long jumps emitted for asm.js?  If not, could
> you remove the #ifdef MIPS longJump loop in
> ModuleGenerator::finishStaticLinkData in WasmGenerator.cpp?

Yes. The thunkWithPatch is required longJump for asm.js.
https://hg.mozilla.org/mozilla-central/rev/b6287d1e9896
https://hg.mozilla.org/mozilla-central/rev/54c1711710b1
https://hg.mozilla.org/mozilla-central/rev/1911cab911de
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(In reply to Heiher [:hev] from comment #13)
> Yes. The thunkWithPatch is required longJump for asm.js.

Is there an alternative strategy to implement thunkWithPatch that doesn't use long-jump?  For example, if you look at the ARM impl, we simply store the 32-bit displacement right after the thunk's jump instruction and then it's easy to patch directly in patchThunk.  I ask b/c it'd be nice to remove the special MIPS cases from static linking.
(In reply to Luke Wagner [:luke] from comment #15)
> (In reply to Heiher [:hev] from comment #13)
> > Yes. The thunkWithPatch is required longJump for asm.js.
> 
> Is there an alternative strategy to implement thunkWithPatch that doesn't
> use long-jump?  For example, if you look at the ARM impl, we simply store
> the 32-bit displacement right after the thunk's jump instruction and then
> it's easy to patch directly in patchThunk.  I ask b/c it'd be nice to remove
> the special MIPS cases from static linking.

I have a way to implement that doesn't use long jump, but i'm not sure whether to affect performance. i'll try.
Ah, don't worry about too much about the performance of the thunks: they should be very cold.
Blocks: 1249513
Duplicate of this bug: 1248397
You need to log in before you can comment on or make changes to this bug.