1247104 - wasm: parsing for load and store operators

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Dan Gohman [:sunfish]]

The following set of patches implements parsing and some amount of encoding and validation for loads and stores. It's not finished yet though, as finishing this will require refactoring the asm.js code, which is a larger change.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: wasm-reduce-indentation.patch

Just a cleanup patch. next() was getting big and deeply nested, so I moved it out of the class definition.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: wasm-anonymous-namespaces.patch

This makes all the WasmText.cpp-local classes local.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: wasm-load-store.patch

This adds parsing and some encoding and decoding support for load and store. It's not usable yet, as this will require some significant asm.js refactoring, but this patch gets things started.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8717706 [details] [diff] [review]
wasm-reduce-indentation.patch

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

Good idea

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8717707 [details] [diff] [review]
wasm-anonymous-namespaces.patch

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

::: js/src/asmjs/WasmText.cpp
@@ +2227,4 @@
>  /*****************************************************************************/
>  // wasm function body serialization
>  
> +namespace {

Normally we don't unnammed namespace our static functions.  I had even thought maybe it wasn't technically valid (to put a static function inside a namespace, but maybe there is a special case for unnamed?).  So if there isn't a big reason, could you remove this and the one below?

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8717710 [details] [diff] [review]
wasm-load-store.patch

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

Good stuff.

::: js/src/asmjs/Wasm.cpp
@@ +350,5 @@
> +    uint32_t offset, align;
> +    return DecodeExpr(f, ExprType::I32) &&
> +           f.d().readVarU32(&offset) &&
> +           f.d().readVarU32(&align) &&
> +           mozilla::IsPowerOfTwo(align) &&

I'm afraid we need to brake this pretty && chain and report a validation message for !power-of-two alignment.

::: js/src/asmjs/WasmText.cpp
@@ +2095,5 @@
>  }
>  
> +static bool
> +ParseLoadStoreAddress(WasmParseContext& c, WasmAstExpr*& base,
> +                      int32_t& offset, int32_t& align)

The usual SM/Odin style is to always pass outparams by *, not &, since this makes it syntactically evident at the callsite since you pass "&x" instead of "x".

@@ +2116,5 @@
> +          case WasmToken::UnsignedInteger:
> +            offset = val.uint();
> +            break;
> +          default:
> +            return false;

Can you c.ts.generateError() here and a few other places below?  I also forgot to look for this in the parse-constants patch, so perhaps you could fix them up too?

@@ +2132,5 @@
> +          case WasmToken::Index:
> +            align = val.index();
> +            break;
> +          default:
> +            return false;

Here, we fail for WasmToken::UnsignedInteger.  Above, UnsignedInteger is implicitly truncated to uint32.  In ParseConst(), it is assigned to a CheckedInt<uint32_t> and checked for validity.  How about you factor out a TokenStream::matchU32() which does the switch and calls generateError.  That should shorten the code too.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd93d21491ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3b4e84a812f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5469639356d3

Comment 8

[Dan Gohman [:sunfish]]

(In reply to Luke Wagner [:luke] from comment #6)
> Comment on attachment 8717710 [details] [diff] [review]
> wasm-load-store.patch
> 
> Review of attachment 8717710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good stuff.
> 
> ::: js/src/asmjs/Wasm.cpp
> @@ +350,5 @@
> > +    uint32_t offset, align;
> > +    return DecodeExpr(f, ExprType::I32) &&
> > +           f.d().readVarU32(&offset) &&
> > +           f.d().readVarU32(&align) &&
> > +           mozilla::IsPowerOfTwo(align) &&
> 
> I'm afraid we need to brake this pretty && chain and report a validation
> message for !power-of-two alignment.

Arg. I somehow dropped this comment. I'll fix this in a separate patch.

> ::: js/src/asmjs/WasmText.cpp
> @@ +2095,5 @@
> >  }
> >  
> > +static bool
> > +ParseLoadStoreAddress(WasmParseContext& c, WasmAstExpr*& base,
> > +                      int32_t& offset, int32_t& align)
> 
> The usual SM/Odin style is to always pass outparams by *, not &, since this
> makes it syntactically evident at the callsite since you pass "&x" instead
> of "x".

Fixed.

> @@ +2116,5 @@
> > +          case WasmToken::UnsignedInteger:
> > +            offset = val.uint();
> > +            break;
> > +          default:
> > +            return false;
> 
> Can you c.ts.generateError() here and a few other places below?  I also
> forgot to look for this in the parse-constants patch, so perhaps you could
> fix them up too?

Fixed, and fixed the constants too.

> @@ +2132,5 @@
> > +          case WasmToken::Index:
> > +            align = val.index();
> > +            break;
> > +          default:
> > +            return false;
> 
> Here, we fail for WasmToken::UnsignedInteger.  Above, UnsignedInteger is
> implicitly truncated to uint32.  In ParseConst(), it is assigned to a
> CheckedInt<uint32_t> and checked for validity.  How about you factor out a
> TokenStream::matchU32() which does the switch and calls generateError.  That
> should shorten the code too.

The UnsignedInteger case getting truncated was in the patch by accident. I've removed that now. Factoring out a matchU32 is tricky because there aren't that many places that need it -- just consts and load offsets, and they need different things. I think the code is pretty clean now, but of course I'm open to further feedback.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd93d21491ea
https://hg.mozilla.org/mozilla-central/rev/a3b4e84a812f
https://hg.mozilla.org/mozilla-central/rev/5469639356d3