1360248 - Baldr: kill WasmActivation::entrySP_

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Luke Wagner [:luke]]

This is one of the oldest asm.js cheats; instead we can use the existing stack unwinding to return to the entry stub's epilogue which is a first step to what we'll need to do for EH anyway.

Comment 1

[Luke Wagner [:luke]]

Attachment: rm-entry-sp

Comment 2

[Luke Wagner [:luke]]

Oops, hit Enter before comment.  I meant to say: mostly pretty easy; the only challenge is that each entry stub needs to pop a different amount of stack b/c the stack holds stack arguments.  The easiest thing I found was to have the throw stub return to the entry stub (using the return address it pushed when calling the real function).  The only trick is to know whether to propagate success or failure which the patch does using the fp register.

Comment 3

[Luke Wagner [:luke]]

Attachment: hoist-arg-return-code

Actually, lemme split this up a bit so it's easier to see entry stub changes.  This patch is pure code motion.

Comment 4

[Luke Wagner [:luke]]

Attachment: rm-entry-sp

And here's the remaining patch.

Comment 5

[Luke Wagner [:luke]]

Attachment: simplify-frame-iter

Unrelated cleanup patch to change type of 'fp' from void* to wasm::Frame* in frame iterators.

Comment 6

[Luke Wagner [:luke]]

Attachment: simplify-frame-iter

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8862991 [details] [diff] [review]
hoist-arg-return-code

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

Thanks for splitting it up!

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8863000 [details] [diff] [review]
rm-entry-sp

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

Nice and easy, thank you!

::: js/src/jit-test/tests/wasm/profiling.js
@@ +191,5 @@
>      (func $foo (unreachable))
>      (func (export "") (call $foo))
>  )`,
>  WebAssembly.RuntimeError,
> +["", ">", "1,>", "0,1,>", "interstitial,0,1,>", "trap handling,0,1,>", "", ">", ""]);

Just to confirm my understanding:
- the first-to-final entry `>` is because we're getting back in the entry stub;
- the second-to-final entry `` accounts for time spent in the throw stub;
Is that right?

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +1551,5 @@
> +void
> +MacroAssembler::PopStackPtr()
> +{
> +    loadPtr(Address(StackPointer, stackAdjust), StackPointer);
> +    adjustFrame(-stackAdjust);

I think stackAdjust isn't defined here?

::: js/src/wasm/WasmFrameIterator.h
@@ +34,5 @@
>  class Code;
>  class CodeRange;
>  class DebugFrame;
>  class DebugState;
> +class Frame;

We don't need this declaration here, do we?

::: js/src/wasm/WasmStubs.cpp
@@ +339,5 @@
>  
>      // Recover the 'argv' pointer which was saved before aligning the stack.
>      masm.Pop(argv);
>  
> +    // Store the return value in argv[0]

uber-nit: dot at the end of sentence.

@@ +350,5 @@
> +    Label success, join;
> +    masm.branchTestPtr(Assembler::Zero, FramePointer, FramePointer, &success);
> +#ifdef DEBUG
> +    Label ok;
> +    masm.branchPtr(Assembler::Equal, FramePointer, Imm32(FailFP), &ok);

Just to assess my understanding, the FailFP magic value trick won't interfere with the profiling frame iterator because it doesn't use the FP register for starting unwinding, but does use the exitFP for that purpose, correct?

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8863012 [details] [diff] [review]
simplify-frame-iter

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

Seems this is a subset of the next patch, so will cancel review on that one.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8863031 [details] [diff] [review]
simplify-frame-iter

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

Yay to more typing.

Comment 11

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> > +["", ">", "1,>", "0,1,>", "interstitial,0,1,>", "trap handling,0,1,>", "", ">", ""]);
> 
> Just to confirm my understanding:
> - the first-to-final entry `>` is because we're getting back in the entry
> stub;
> - the second-to-final entry `` accounts for time spent in the throw stub;
> Is that right?

Exactly

> > +    masm.branchPtr(Assembler::Equal, FramePointer, Imm32(FailFP), &ok);
> 
> Just to assess my understanding, the FailFP magic value trick won't
> interfere with the profiling frame iterator because it doesn't use the FP
> register for starting unwinding, but does use the exitFP for that purpose,
> correct?

Right, as explained in the comment in the ProfilingFrameIterator ctor, when exitFP is set, it takes precedence.  Moreover, even w/o this special case, pc is Throw and Entry stubs both of which immediately mark the iterator as done().

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/502f442198b6
Baldr: factor out argument/return ABI code (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0d8eb9dc56
Baldr: remove WasmActivation::exitSP_, using unwinding instead (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/20f376c71c88
Baldr: use wasm::Frame* type for fp instead of void* (r=bbouvier)

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out bug 1360248 and bug 1360254 for build bustage in WasmSignalHandlers.cpp, at least on Windows 2012:

Bug 1360248:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0318c8fa15e76f6eaab2df21235b196f64187b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dafdc199ec7ff8c03682048c8b0e624aeda38a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b83af0b372e8473091513f85cde558b4957814be

Bug 1360254:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fede60d2180928e31b9e7b13cd96fbe0dc151121

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3e087957c2c69a32e5a45da0f24db1c9625a5543&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

There were also test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=98098909&repo=mozilla-inbound

Comment 15

[Luke Wagner [:luke]]

D'oh, that's what I get for blindly applying a windows-only change without recompiling on windows.  The second failure was because the AutoNoteSingleThreadedRegion bug 1360263 added was narrower in scope than the one I the local patch I try server'd and then deleted when bug 1360263 landed.  Updated patch broadens the scope again.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0284ba593ded
Baldr: factor out argument/return ABI code (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f13207882d12
Baldr: remove WasmActivation::exitSP_, using unwinding instead (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/44f970fca9ed
Baldr: use wasm::Frame* type for fp instead of void* (r=bbouvier)

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0284ba593ded
https://hg.mozilla.org/mozilla-central/rev/f13207882d12
https://hg.mozilla.org/mozilla-central/rev/44f970fca9ed