Validator yells at stack-based block

[ericvergnaud]

Hi, I'm compiling a x++ like expression, which requires returning the value of a local before it is incremented. The generated wasm looks like this:

(block (result i32)
 (local.get $2)
 (local.set $2
  (i32.add
   (local.get $2)
   (i32.const 1)
  )
 )
)

I believe the block body is valid wat. But it's in a block, and binaryen yells with 2 errors: [wasm-validator error in function fromUTF8] unexpected false: non-final block elements returning a value must be drop()ed (binaryen's autodrop option might help you) and [wasm-validator error in function fromUTF8] none == none: block with value must not have last element that is none

I understand the messages, but I would expect the validator to detect that since there is an i32 on the stack, it's ok for the last instruction (local.set $2) to return none...

Is the current behavior:

1. an issue with the validator ?
2. a binaryen limitation ?
3. a WebAssembly limitation ?

If it's 2., then what are my options for generating the above snippet ? (I know I can simply create a temp variable but that's very ugly, especially since WebAssembly doesn't support scoped locals)

[kripken]

This is "unstructured/stacky" code that binaryen's current text format doesn't support. It only supports the structured form, where all children are nested in their parents (and all block children have type none).

The new parser will fix that eventually, but BinaryenIR is structured, so it will use a temp local. We can optimize those away in some cases, but it is just a minor code size issue (when the code executes in a VM it will in general need a temp register for such code anyhow, to return the value before the increment).

[ericvergnaud]

so you're saying that I have to use a temp local for now, but would be able to drop it with the new parser ?

[kripken]

Yes, exactly.

[ericvergnaud]

Thanks, closing.

[ericvergnaud]

BTW curious question: since binaryen does not support "unstructured/stacky" code, how does it deal with it when it originates from a binary being loaded ?

[kripken]

It adds locals as needed (and often optimizes them away later).

[ericvergnaud]

Reopening because interestingly, I have evidence that despite the validator yelling, the above code actually works as expected... Is that a bug ? Or is it worth digging a bit more into this i.e. adjust the validator's behavior to support "unstructured/stacky" code within blocks ?

[kripken]

Hmm, I can't think of why it would work - it should error, but maybe the validator is missing something. But it's easy to see what's going on by printing the IR to see, is it valid?

[ericvergnaud]

Yes, here is the code being compiled: @ModuleExport function stuff(v: i32): i32 { const a: i32 = v++; const b: i32 = v++; return b; }

Here is the corresponding IR. It runs as desired, but maybe that's not expected... I would be tempted to adjust the validator logic, but I don't know enough about binaryen internals to form a useful opinion.

(module
 (type $0 (func (param i32) (result i32)))
 (memory $0 1)
 (export "stuff" (func $stuff))
 (func $stuff (type $0) (param $0 i32) (result i32)
  (local $a i32)
  (local $b i32)
  (local.set $a
   (block (result i32)
    (local.get $0)
    (local.set $0
     (i32.add
      (local.get $0)
      (i32.const 1)
     )
    )
   )
  )
  (local.set $b
   (block (result i32)
    (local.get $0)
    (local.set $0
     (i32.add
      (local.get $0)
      (i32.const 1)
     )
    )
   )
  )
  (return
   (local.get $b)
  )
 )
)

[kripken]

I don't think it validates:

$ bin/wasm-opt a.wat -all
[wasm-validator error in function stuff] unexpected false: non-final block elements returning a value must be drop()ed (binaryen's autodrop option might help you), on 
(block (result i32)
 (local.get $0)
 (local.set $0
  (i32.add
   (local.get $0)
   (i32.const 1)
  )
 )
)
(on index 0:
0x55b01c6f4a10
), type: i32
[wasm-validator error in function stuff] none == none: block with value must not have last element that is none, on 
(block (result i32)
 (local.get $0)
 (local.set $0
  (i32.add
   (local.get $0)
   (i32.const 1)
  )
 )
)
[wasm-validator error in function stuff] unexpected false: non-final block elements returning a value must be drop()ed (binaryen's autodrop option might help you), on 
(block (result i32)
 (local.get $0)
 (local.set $0
  (i32.add
   (local.get $0)
   (i32.const 1)
  )
 )
)
(on index 0:
0x55b01c6f4b20
), type: i32
[wasm-validator error in function stuff] none == none: block with value must not have last element that is none, on 
(block (result i32)
 (local.get $0)
 (local.set $0
  (i32.add
   (local.get $0)
   (i32.const 1)
  )
 )
)
Fatal: error validating input

It's possible that it happens to get emitted as valid wasm despite not being valid Binaryen IR, but I would not depend on that. In particular anything non-valid will be at very high risk of getting optimized incorrectly.

[kripken]

Indeed, I tried to optimize it and it fails:

$ bin/wasm-opt a.wat -all --no-validation -O1
wasm-opt: Vacuum.cpp:72: wasm::Expression* wasm::Vacuum::optimize(wasm::Expression*, bool, bool): Assertion `!resultUsed || curr->type != Type::none' failed.
Aborted

[ericvergnaud]

Yes, not surprising since I opened this issue because the validator was yelling. I'm just wondering now if it would be worth considering blocks as a special case in the validator, the optimizer and other tools ? After all, it is valid wasm, so in principle it would be an improvement of binaryen... Another idea I came up with would be to implement push/pop commands as follows:

(block (result i32)
 (push
    (local.get $0)
  )
  (local.set $0
  (i32.add
   (local.get $0)
   (i32.const 1)
  )
  (pop i32)
 )
)

but that would require evolving the wat spec

[kripken]

I'm just wondering now if it would be worth considering blocks as a special case in the validator, the optimizer and other tools ?

Unfortunately that would be a pretty massive change in the IR, requiring changes across many passes and utilities. Also it would remove a lot of the current efficiency we have, where if you have a Binary AddInt32 then you just do curr->left|right to get the left and right operands, but if they could be outside of the current node then you need to go to the parent block and look there. There would be benefits too of course, but overall in Binaryen IR we've chosen a different set of tradeoffs.

[tlively]

Closing this since the problem was explained and this is WAI.