Put br_if conditions last.

[sunfishcode]

This implements the second half of WebAssembly/design#489.

Desugaring br_if to if+break was fairly awkward; adding br_if to the kernel
language was straightforward and avoided the awkwardness.

This change happened to expose an aspect of br_if's type checking which
differed from my expectations, so I've taken the liberty of fixing that
too. See the deleted assert_invalid in test/labels.wast and the added
$br_if1 testcase. br_if either sends its value to the destination label
or to its own result.

[ghost]

The prior type check appears to be because br_if returned the empty-values type, the same type that nop returns, and it was in the fall-through position of the block $l which expected a single f32 value.

Your changes to the type checking appear to be consistent with br_if now returning the result of evaluating the expression and is that the case? So when the result expression is omitted it would be equivalent to a nop and return the same as a nop. When the expression is anop` this would be equivalent?

With the change, is there still the duplication noted in #227 ? Perhaps it resolves that issue too.

This might have some interaction with #215

Any opinion on WebAssembly/design#529 while your working in this area?

[sunfishcode]

I don't wish to connect this issue to #215 or #227. Accordingly, I've now removed that part of the change. This PR now just contains the operand order change.

I would also like to keep this issue focused and avoid discussions of WebAssembly/design#529.

[ghost]

@sunfishcode Are you saying you don't understand the type system issues and can not answer these questions? Does it change the existing behaviour as noted in #227 ? Does it change the 'arity' checking as noted in #215 ? Or are you asking for help, do you want other people to check this and give feedback?

This does move the arguments around, and it would look better to me if the label were first, and the v8 encoding has the label depth first, it is an immediate, and having it last is also not consistent with br - would not block the issue on this but still your are moving them and I think we can expect you to champion this change and engage in discussion?

[sunfishcode]

I'm saying I don't want the part of this PR I'm primarily interested in to get lost in tangential discussions.

[ghost]

This does change the behaviour of the issue noted in #227 For example this now passes:

(module
 (func $f1 (result i32)
   (block $l0
     (br_if (block $l1
              (br $l1 (i32.const 1)))
            (i32.const 1) $l0)
     (i32.const 1))))

This seems a good thing to me, but there have been different opinions!

[ghost]

Fwiw is seems to be neutral wrt #215 The following still gives an 'arity mismatch' error:

(module
 (func $f1
   (block $l0
     (br_if (nop) (i32.const 1) $l0))))

[sunfishcode]

So it does; I've now added the testcase.

[rossberg]

That syntax is really odd. The argument should still go after the label, to be consistent with plain br (and calls). So I think this should be BR_IF var expr expr instead.

[rossberg]

Looks good modulo the argument order. It's really unfortunate that the kernel now loses orthogonality and has to duplicate the break logic, but I have no better answer either (other than removing br_if :) ).

[sunfishcode]

@rossberg-chromium Thanks! I added a patch to put the argument after the label.

[rossberg]

lgtm

[sunfishcode]

Thanks!