1329096 - Wasm baseline: Use NonAssertingLabel for Block/Loop, get rid of the label pool

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P2)

Description

[Lars T Hansen [:lth]]

Currently there is a label pool in the baseline compiler because the labels for Block and Loop don't obey stack discipline (because of nonrecursive traversal of the bytecode).  This label pool is dynamic, so risks OOM, so there is OOM handling and error propagation, it's all a little ugly.

Luke suggests (bug 1316819 comment 3) that we can use NonAssertingLabel instead here, and get rid of the label pool and the OOM handling.

This work will interact slightly with bug 1316814 (control stack revamp), be sure to take that into account.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1329096-remove-label-pool.patch

Something like this?  Passes testing locally, seems reasonable.  (Sits on top of my current patch for bug 1331900.)

Notes:

(1) Discussion going on in bug 1331900 about whether it's reasonable for the iterator's control stack to reinitialize the control item when it switches to the 'else' branch.  In the patch on that bug I hacked around the problem.  Here I remove that functionality entirely since it requires the ControlItem to be assignable, and mine aren't.  Hence asking for feedback only.  If we keep the current reinit functionality for ControlItems then this patch won't work in its current form.

(2) What became a copy before readEnd() in bug 1331900 becomes a Move here to handle the non-copyable Labels.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8827851 [details] [diff] [review]
bug1329096-remove-label-pool.patch

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

Looks great, pretty mechanical except for interactions with bug 1316814 which I think will be decided in that bug (possibly folding in this patch).

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1329096-remove-label-pool-v2.patch

Similar to the patch you just saw but avoids the very dodgy use of the Move() operation and does not make any changes to the iterator, only to the baseline compiler.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8827982 [details] [diff] [review]
bug1329096-remove-label-pool-v2.patch

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

Great!  It'll be interesting to see if this moves the dial on awfy.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +380,1 @@
>                stackSize(0),

Could these be initialized to bogus (UINT32_MAX) values that initControl() asserts are bogus?

@@ +5416,4 @@
>      if (!deadCode_)
>          sync();                    // Simplifies branching out from block
>  
> +    initControl(controlItem());

Could this now be moved to right after readBlock() (to minimize the created-but-not-initialized range of the Control item)?  Also in emitLoop()

@@ +7591,5 @@
>          return false;
>  
>      beginFunction();
>  
> +    initControl(controlItem());

Similarly, could this be moved right under readFunctionStart()?

Comment 5

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #4)
> Comment on attachment 8827982 [details] [diff] [review]
> bug1329096-remove-label-pool-v2.patch
> 
> Review of attachment 8827982 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +5416,4 @@
> >      if (!deadCode_)
> >          sync();                    // Simplifies branching out from block
> >  
> > +    initControl(controlItem());
> 
> Could this now be moved to right after readBlock() (to minimize the
> created-but-not-initialized range of the Control item)?  Also in emitLoop()

I don't think so.  initControl() captures masm.framePushed() and that may be updated by sync(): for now, we have an adjustable stack pointer.

> @@ +7591,5 @@
> >          return false;
> >  
> >      beginFunction();
> >  
> > +    initControl(controlItem());
> 
> Similarly, could this be moved right under readFunctionStart()?

Ditto, masm.framePushed() will be updated (for sure) by beginFunction.

Comment 6

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18121692ab35

Comment 7

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/082be147ff94724672c59bb62ef47d8c086cd191
Bug 1329096 - Wasm baseline, remove the label pool and use NonAssertingLabel instead. r=luke

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/082be147ff94