1248859 - OdinMonkey: MIPS: Fix jump related methods for wasm.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1245112#c41
https://bugzilla.mozilla.org/show_bug.cgi?id=1245112#c43
https://bugzilla.mozilla.org/show_bug.cgi?id=1245112#c44
https://bugzilla.mozilla.org/show_bug.cgi?id=1245112#c45

Comment 1

[hev [:hev]]

Attachment: 0001-OdinMonkey-MIPS-Fix-replace-retargetWithOffset.patch

Comment 2

[hev [:hev]]

Attachment: 0002-OdinMonkey-MIPS-Implement-thunkWithPatch-and-re-patc.patch

Comment 3

[Luke Wagner [:luke]]

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?

Comment 4

[hev [:hev]]

(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.

Comment 5

[hev [:hev]]

Oops, max range is +/-128k.

Comment 6

[Tooru Fujisawa [:arai]]

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.

Comment 7

[Pulsebot]

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

Comment 8

[Luke Wagner [:luke]]

(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?

Comment 9

[hev [:hev]]

Attachment: 0003-OdinMonkey-MIPS-Refactor-callWithPatch-via-reative-b.patch

Comment 10

[Luke Wagner [: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?

Comment 11

[Luke Wagner [:luke]]

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?

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54c1711710b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/1911cab911de

Comment 13

[hev [:hev]]

(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.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6287d1e9896
https://hg.mozilla.org/mozilla-central/rev/54c1711710b1
https://hg.mozilla.org/mozilla-central/rev/1911cab911de

Comment 15

[Luke Wagner [:luke]]

(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.

Comment 16

[hev [:hev]]

(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.

Comment 17

[Luke Wagner [:luke]]

Ah, don't worry about too much about the performance of the thunks: they should be very cold.