1585990 - Fix multi-value codegen for Ion

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, task, P3)

Description

[Andy Wingo [:wingo]]

I realized we have some interesting implementation questions regarding Ion and multi-value.

For JS, Ion can calculate the number of slots in the function just fine -- it mostly corresponds to the number of locals plus a handful of special-purpose slots.

For Wasm up to now, we took the same approach, while adding on one more slot for use as a block return value. The return value gets set via pushDef() when leaving a basic block and gets set to the last slot in the block.

But with multi-value we might have many results for any given block exit, or loop entry, and we don't know how many when we start compiling the function. So I guess we would need to be able to add on more slots while we're compiling. We could do so at each block entry, to expand the slots if needed; but that's... gross.

Is there a nice way to do this?

Comment 1

[Andy Wingo [:wingo]]

Just for context, the error I get when compiling this:

  (module
    (func $add (export "add") (param i32 i32) (result i32)
      (i32.add
        (block (result i32 i32)
          (local.get 0) (local.get 1)))))

is this:

Thread 1 "js" received signal SIGSEGV, Segmentation fault.
js::jit::MBasicBlock::push (this=<optimized out>, ins=<optimized out>) at /home/wingo/src/mozilla-unified/js/src/jit/MIRGraph.h:201
201	    MOZ_ASSERT(stackPosition_ < nslots());
(gdb) bt
#0  js::jit::MBasicBlock::push (this=<optimized out>, ins=<optimized out>) at /home/wingo/src/mozilla-unified/js/src/jit/MIRGraph.h:201
#1  (anonymous namespace)::FunctionCompiler::pushDefs (this=<optimized out>, defs=...) at /home/wingo/src/mozilla-unified/js/src/wasm/WasmIonCompile.cpp:1195
#2  0x0000555557a8e642 in EmitEnd (f=...) at /home/wingo/src/mozilla-unified/js/src/wasm/WasmIonCompile.cpp:1818
#3  EmitBodyExprs (f=...) at /home/wingo/src/mozilla-unified/js/src/wasm/WasmIonCompile.cpp:3374

Comment 2

[Luke Wagner [:luke]]

Oh right, I remember forgetting about this!

So it looks like, by default, the size of MBasicBlock::slots_ is initialized (by MBasicBlock::init()) to the number of wasm locals, plus, by default, 1, so that it's always possible to push one phi (I'm guessing CompileInfo(unsigned nslots) is only used by wasm, which is why it gets hardcoded). It looks like MBasicBlock::nstack_ can be different for each MBasicBlock, though, so I think a fix could be to pass in a second nstack parameter to CompileInfo() which is computed from the block type (which is always known before creating the MBasicBlock, I think).

Comment 3

[Andy Wingo [:wingo]]

Humm, I think I am missing some of the mechanics; there' no MBasicBlock::nstack_, right? I just see the slot count that comes from the CompileInfo, which is set once for the entire compilation unit.

Also we don't know the precise nstack value eagerly, as there is an N-to-1 relationship between MBasicBlock and wasm blocks; a br_if ends an MBasicBlock, pushing defs corresponding to the branch target, not the current block result type.

So in general, in the current one-pass compiler, we know the maximum number of possible results (nmax) for the current block at any point: it's initially the number of return values of the function; when entering a block, it becomes nmax = max(nmax, block.results().length()); additionally when entering loops, it becomes nmax = max(nmax, block.params.length()). But we don't know nmax for the function as a whole in advance, nor do we know nresults for any given MBasicBlock until it ends.

Comment 4

[Andy Wingo [:wingo]]

Of course after asking for help, I realized that MBasicBlock has a perfectly good ensureHasSlots method that can expand the number of slots :P A patch to use this appears to work just fine. Will attach.

Comment 5

[Andy Wingo [:wingo]]

Attachment: Bug 1585990 - Fix WebAssembly multi-value Ion codegen

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a6ae373e59e
Fix WebAssembly multi-value Ion codegen r=luke

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9a6ae373e59e