1246433 - wasm: parsing and encoding for consts

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Dan Gohman [:sunfish]]

Const nodes for i32, i64, f32, f64, in both decimal and hexadecimal forms.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: wasm-literals.patch

This uses the JS code for parsing decimal floating-point literals. It's guarded by custom tokenizing code so it only sees the strings we want it to see, and I believe it does the right thing for that set of strings.

It also contains a from-scratch hexadecimal floating-point parser. It doesn't use the libc strtod because not all platforms have the strtod_l form for avoiding the global locale, and also, Windows' strtod doesn't appear to have hexadecimal support. I considered other open-source options, such as a recent version of dtoa.c or LLVM's APFloat, however they carry complexity both in their own code and in what would be needed to integrate them into Baldr. The code here is significantly simplified by doing only what we need here and avoiding configuration.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: wasm-literals.patch

Minor code cleanups.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8716777 [details] [diff] [review]
wasm-literals.patch

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

Very nice!

::: js/src/asmjs/Wasm.cpp
@@ +228,5 @@
> +        return f.fail("unable to read f32.const immediate");
> +    if (IsNaN(value)) {
> +        const float jsNaN = (float)JS::GenericNaN();
> +        if (memcmp(&value, &jsNaN, sizeof(value)) != 0)
> +            return f.fail("NYI: NaN literals with custom payloads");

Ahh, this would be fixed by making MConstant not use js::Value right?

::: js/src/asmjs/WasmBinary.h
@@ +405,5 @@
> +            if (i != 0)
> +                byte |= 0x80;
> +            if (!writeU8(byte))
> +                return false;
> +        } while(i != 0);

Perhaps factor out into a private template function called by both writeVarU(32|64)?  (And similarly for readVarU(32|64)?

::: js/src/asmjs/WasmText.cpp
@@ +700,5 @@
>      return false;
>  }
>  
> +static WasmToken LexHexFloatLiteral(const char16_t *begin, const char16_t *end,
> +                                    const char16_t **curp) {

Can you put 'static WasmToken' on its own line as well as the { (on this and all the other static functions in this file?

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccaa937df50d

Comment 5

[Dan Gohman [:sunfish]]

(In reply to Luke Wagner [:luke] from comment #3)
> Comment on attachment 8716777 [details] [diff] [review]
> wasm-literals.patch
> 
> Review of attachment 8716777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very nice!
> 
> ::: js/src/asmjs/Wasm.cpp
> @@ +228,5 @@
> > +        return f.fail("unable to read f32.const immediate");
> > +    if (IsNaN(value)) {
> > +        const float jsNaN = (float)JS::GenericNaN();
> > +        if (memcmp(&value, &jsNaN, sizeof(value)) != 0)
> > +            return f.fail("NYI: NaN literals with custom payloads");
> 
> Ahh, this would be fixed by making MConstant not use js::Value right?

Yes, that's right.

> ::: js/src/asmjs/WasmBinary.h
> @@ +405,5 @@
> > +            if (i != 0)
> > +                byte |= 0x80;
> > +            if (!writeU8(byte))
> > +                return false;
> > +        } while(i != 0);
> 
> Perhaps factor out into a private template function called by both
> writeVarU(32|64)?  (And similarly for readVarU(32|64)?

Done.

> ::: js/src/asmjs/WasmText.cpp
> @@ +700,5 @@
> >      return false;
> >  }
> >  
> > +static WasmToken LexHexFloatLiteral(const char16_t *begin, const char16_t *end,
> > +                                    const char16_t **curp) {
> 
> Can you put 'static WasmToken' on its own line as well as the { (on this and
> all the other static functions in this file?

Done.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94bc06efafd9

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Both patches backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/0596083c0bb2 because it was still busted in a different way after the followup.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45c16dd1f3bd

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45c16dd1f3bd