Stack test cleanup #351
Stack test cleanup #351
Conversation
…atures, and move some tests to stack.wast
ml-proto/test/block.wast
Outdated
| ;; TODO(stack): move this elsewhere | ||
| (module (func $type-break-num-vs-void | ||
| (block (i32.const 66) (br 0)) | ||
| (block (drop (i32.const 66)) (br 0)) |
There was a problem hiding this comment.
It's not clear why this code needs changing, it would have worked with the AST without change. Could I ask why this is problematic for binaryen at present?
There was a problem hiding this comment.
In the AST model, a value must be used or dropped. The 66 constant is not used, therefore it must be dropped.
There was a problem hiding this comment.
At one point at least the AST did support code such as (i32.add (i32.const 1) (br ...)) and (block (i32.const 1) (i32.const 2)) and 'statements' are consistent with an AST.
There was a problem hiding this comment.
As I understand the AST, in your first example the 1 is used by the add. The second example was supported before explicit drop was decided, but after explicit drop the first of the two constants must be dropped.
There was a problem hiding this comment.
It seems ok for the AST apart from some constraint being assumed of 'explicit drop', but this constraint is overly restrictive as this code is valid with 'explicit drop'. I don't know your code, but might it just be a matter of relaxing a constraint, a simply matter to resolve in binaryen? This might not address all these cases, but perhaps a good number of them. If you could point me to the failing check then that might help me understand the challenge?
There was a problem hiding this comment.
It seems ok for the AST apart from some constraint being assumed of 'explicit drop'
Exactly. But explicit drop is what the wasm project decided, for better or for worse.
There was a problem hiding this comment.
As far as I am aware the 'explicit drop' constraints do not make these examples invalid, it's just binaryen, and we seem to agree that some of these are not incompatible with an AST either. I don't think these spec tests should be changed just to follow a more restrictive interpretation being made in binaryen.
The tests that would be incompatible with the binaryen AST for now are those that use an ordering that would demand first or lexical scoped constants. I think binaryen should work with the rest, relax the constraints, and just separate the remainder.
There was a problem hiding this comment.
I think you can just remove this test here, it is superseded by the tests in unwind.wast (#344).
ml-proto/test/block.wast
Outdated
| ;; TODO(stack): move this elsewhere | ||
| (module (func $type-break-num-vs-void | ||
| (block (i32.const 66) (br 0)) | ||
| (block (drop (i32.const 66)) (br 0)) |
There was a problem hiding this comment.
I think you can just remove this test here, it is superseded by the tests in unwind.wast (#344).
| ) | ||
| (func (export "as-br_if-value-cond") (result i32) | ||
| (block i32 (br_if 0 (i32.const 6) (br 0 (i32.const 9))) (i32.const 7)) | ||
| (block i32 (drop (br_if 0 (i32.const 6) (br 0 (i32.const 9)))) (i32.const 7)) |
There was a problem hiding this comment.
He he, most of these are probably also fixed in #345.
| "type mismatch" | ||
| ) | ||
|
|
||
| ;; TODO(stack): move these elsewhere |
There was a problem hiding this comment.
Equivalent tests exist in nop.wast, so you can just delete this.
| "type mismatch" | ||
| ) | ||
|
|
||
| ;; TODO(stack): move these elsewhere |
There was a problem hiding this comment.
Same here, this can just go away.
| ) | ||
| ;) | ||
|
|
||
| ;; TODO(stack): move this elsewhere |
There was a problem hiding this comment.
This one is subsumed by unwind.wast (#344) as well.
| "type mismatch" | ||
| ) | ||
|
|
||
| ;; TODO(stack): move these elsewhere |
| ) | ||
| (func (export "as-block-first-value") (param i32) (result i32) | ||
| (block i32 (br_if 0 (i32.const 10) (get_local 0)) (return (i32.const 11))) | ||
| (block i32 (drop (br_if 0 (i32.const 10) (get_local 0))) (return (i32.const 11))) |
There was a problem hiding this comment.
I don't understand why a drop is required here, since br_if leaves its value on the stack in the false case, and then falls off the end of the block, and the block is annotated as i32.
There was a problem hiding this comment.
The return is also in that block, i.e. with indentation it looks like this:
(block $block0 i32
(br_if $block0
(i32.const 10)
(get_local $0)
)
(return
(i32.const 11)
)
)
|
@rossberg-chromium: ok, I pushed an update addressing your comments. also some fixes i noticed when running the tests. |
|
LGTM |
3 participants
As discussed in #343, this adds
droptobr_ifs where necessary, and moves some stacky tests intostack.wast. With these changes, the "non-stacky" test files don't have stacky stuff, and binaryen can pass them (with just these disabled: stack, nop, typecheck).