1244272 - wasm: Implement a binary operator

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Dan Gohman [:sunfish]]

I'm starting with i32.add to start threading it through all the places that need it.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: wasm-binary-operators.patch

Comment 2

[Dan Gohman [:sunfish]]

Attachment: wasm-all-i32-binops.patch

Building on the previous patch, here are the rest of the i32 binary operators (except comparisons).

Comment 3

[Dan Gohman [:sunfish]]

Attachment: wasm-float-binops.patch

And here are all the float binary opcodes except comparisons.

min/max don't work yet because the implementation needs refactoring to deal with them not being variadic in wasm, and copysign doesn't have Ion support yet.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8713797 [details] [diff] [review]
wasm-binary-operators.patch

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

Excellent.  But don't you miss writing ML ;-) ?

::: js/src/asmjs/WasmText.cpp
@@ +587,5 @@
> +                    return WasmToken(WasmToken::ValueType, ValType::F32, begin, cur_);
> +
> +                switch (*cur_) {
> +                  case 'c':
> +                    if (consume(end_, MOZ_UTF16("const")))

Especially since perf doesn't matter, I probably should've done it the way you've done here, with 'switch (*cur_)' instead of 'switch (*cur_++)' so that way the MOZ_UTF16 strings can hold the full string instead of stripping the first character.  Feel free to do that in this or a later patch or I will in some later patch.

@@ +635,5 @@
> +                        return WasmToken(WasmToken::Const, ValType::I64, begin, cur_);
> +                    break;
> +                }
> +            } else if (consume(end_, MOZ_UTF16("mport"))) {
> +                return WasmToken(WasmToken::Import, begin, cur_);

For consistency with other cases, could you have each then-branch end in return/break so that way we can have a sequence of if{}if{}if{} instead of if{}else if{}else if {}?

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8713827 [details] [diff] [review]
wasm-all-i32-binops.patch

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

weeeeeeeeeee

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8713831 [details] [diff] [review]
wasm-float-binops.patch

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

Oh min/max.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3baa9c161c1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1721ff79abc
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe865d5d95dc

Comment 8

[Dan Gohman [:sunfish]]

> For consistency with other cases, could you have each then-branch end in return/break so that way we can have a sequence of if{}if{}if{} instead of if{}else if{}else if {}?

Done.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3baa9c161c1b
https://hg.mozilla.org/mozilla-central/rev/f1721ff79abc
https://hg.mozilla.org/mozilla-central/rev/fe865d5d95dc