1242949 - wasm: implement Block

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Currently, Block are implemented as:

- u32: number of expressions
- exprs
- DEBUG only debug check point (for sanity)

The number of expressions could become a variable length u32 instead. I think the debug check point is valuable, but let's see.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Implement Block

My first opcode \o/

So this switches the number of sub expressions to be encoded in LEB128, which is a net loss on DeadTrigger2's function bytecode size of 2.31%.

This also found a new class of bug in WasmIonCompile: as everything is an expression in wasm, in some places we can have nullptrs while we couldn't before. This uncovered one of these places; fuzzers will uncover the others as we implement the other opcodes.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. JitSpew_WasmStats

I think this is generally useful to have, so as to experiment on the bytecode's size.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8712189 [details] [diff] [review]
2. JitSpew_WasmStats

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

::: js/src/asmjs/WasmGenerator.cpp
@@ +523,5 @@
>  
> +#ifdef DEBUG
> +    JitSpew(JitSpew_WasmStats, "Total bytecode size for module: %lu", uint64_t(totalBytecodeSize_));
> +    JitSpew(JitSpew_WasmStats, "Biggest bytecode size for module: %lu", uint64_t(biggestBytecodeSize_));
> +    JitSpew(JitSpew_WasmStats, "Total number of functions: %u", funcEntryOffsets_.length());

I've used % PRIu32 at these places, as they are more portable and don't need the implicit conversion.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8712188 [details] [diff] [review]
1. Implement Block

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

::: js/src/asmjs/Wasm.cpp
@@ +212,5 @@
> +        if (!DecodeExpr(f, expected))
> +            return false;
> +    } else if (!CheckType(f, ExprType::Void, expected)) {
> +        return false;
> +    }

What about this control flow:
  if (numExprs) {
    for (uint32_t i = 0; i < numExprs - 1; i++)
        ...
    if (!DecodeExpr(f, expected))
       ...
  } else {
    if (!CheckType(f, ExprType::Void, expected))
       ...
  }
(Relying on LICM to hoist numExpr-1 and putting the CheckType inside the else to avoid the irregular-looking else-if.)

::: js/src/asmjs/WasmIonCompile.cpp
@@ +2652,5 @@
>  
>  static bool
>  EmitBlock(FunctionCompiler& f, MaybeType type, MDefinition** def)
>  {
> +    uint32_t numStmt = f.readVarU32();

Preexisting, but how about 'numStmts'?

@@ +2653,5 @@
>  static bool
>  EmitBlock(FunctionCompiler& f, MaybeType type, MDefinition** def)
>  {
> +    uint32_t numStmt = f.readVarU32();
> +    for (uint32_t i = 1; i < numStmt; i++) {

For symmetry DecodeBlock, could you do the same 'i = 0; i < numStmts -1' here too?

::: js/src/asmjs/WasmText.cpp
@@ +180,5 @@
>  
> +class WasmAstBlock : public WasmAstExpr
> +{
> +  public:
> +    typedef WasmAstVector<WasmAstExpr*> ExprVector;

Could you move this typedef to be the line after WasmAstValTypeVector and name it WasmAstExprVector?

@@ +192,5 @@
> +      : WasmAstExpr(Kind),
> +        exprs_(lifo)
> +    {}
> +
> +    ExprVector& exprs() { return exprs_; }

How about making this a const function (returning a const&) and having WasmAstBlock take a WasmAstExprVector&&?  I'm generally trying to use this pure-functional style when building the AST (although for the module and func nodes I build it mutably).

@@ +909,5 @@
> +        if (!EncodeExpr(e, *b.exprs()[i]))
> +            return false;
> +    }
> +
> +    return e.writeDebugCheckPoint();

I'd really rather avoid emitting a debug checkpoint as if this was wasm, even in debug mode.  It seems like this might also introduce some wasm/binary.js testing issues at some point.  Can you remove the debug check point?

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8712189 [details] [diff] [review]
2. JitSpew_WasmStats

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

I kinda wonder how valuable it is to land this logging.  It is nice to be able to easily get it but, then again, it's not super common and it's super-easy to just add the printf's when you need it (usually in conjunction with some other enabling piece of info).

Comment 6

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> My first opcode \o/

I mean to say: great job, you found all the right points!

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #4)
> @@ +2653,5 @@
> >  static bool
> >  EmitBlock(FunctionCompiler& f, MaybeType type, MDefinition** def)
> >  {
> > +    uint32_t numStmt = f.readVarU32();
> > +    for (uint32_t i = 1; i < numStmt; i++) {
> 
> For symmetry DecodeBlock, could you do the same 'i = 0; i < numStmts -1'
> here too?

Thanks for the review! Actually we can't do that: numStmts is an unsigned, and if we have a block with 0 exprs, numStmts - 1 is UINT32_MAX. I've kept it that way.

Also, I'd really want to keep debug check points at least for asm.js, because they allowed me to find a few bugs when implementing the control flow opcodes, and I'm just about to work again on that. So we need to be able to know in FunctionCompiler whether we're in asm.js or wasm mode. Submitting an amended patch for that.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Implement Block

Amended patch, see comment before. This adds DebugOnly<bool> compilingAsmJS to the ThreadView class, so that the FunctionCompiler can conditionally assert the debug check point on whether we're compiling asm.js or not.

About the stats patch, I can keep it local only, but it means having to rebase it all the time, be careful not to push it, etc. It feels like it's not adding too much overhead to runtime and it's debug only anyways. But if you feel strongly about it, let's just forget it.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8712602 [details] [diff] [review]
1. Implement Block

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

::: js/src/asmjs/Wasm.cpp
@@ +208,5 @@
> +    if (!f.d().readVarU32(&numExprs))
> +        return f.fail("unable to read block's number of expressions");
> +
> +    if (numExprs) {
> +        for (uint32_t i = 1; i < numExprs; i++) {

But you can write numExprs - 1, though, since you've checked that numExprs > 0 right above :)

::: js/src/asmjs/WasmGenerator.h
@@ +93,5 @@
>  
>  class ModuleGeneratorThreadView
>  {
>      const ModuleGeneratorData& shared_;
> +    DebugOnly<bool> compilingAsmJS_;

Ok, well I'd also like to avoid this too :)  How about we keep the debug checkpoint for now (for both wasm and asm.js) and remove it later after you finish implementing control flow?  I can see it being useful when making large changes but after that I think we'll be amply covered by tests.

::: js/src/asmjs/WasmIonCompile.cpp
@@ +2640,5 @@
>          // Fine to clobber def, we only want the last use.
>          if (!EmitExprStmt(f, def))
>              return false;
>      }
> +    if (numStmts && !EmitExpr(f, type, def))

Can you use the same control flow pattern as in DecodeBlock?

::: js/src/asmjs/WasmText.cpp
@@ +713,5 @@
> +    WasmAstExprVector exprs(c.lifo);
> +    if (!exprs.reserve(numExprs.integer()))
> +        return nullptr;
> +
> +    for (uint32_t i = 0, e = numExprs.integer(); i < e; i++) {

This isn't high-perf code and numExprs.integer() is just an LICMable load so I'd just do i < numExprs.integer()

::: js/src/jit-test/tests/wasm/basic.js
@@ +127,5 @@
>  
>  assertErrorMessage(() => wasmEvalText('(module (func (local i64)))'), Error, /NYI/);
> +
> +// ----------------------------------------------------------------------------
> +// blocks

In keeping w/ the above style, could you put a \n after this?

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8712189 [details] [diff] [review]
2. JitSpew_WasmStats

I think it would make sense to continue to emit JS console messages when successfully compiling wasm (maybe with some size threshold so we don't spam the console if a page uses lots of small wasm) and that message could include all these stats if you want.

Comment 11

[Lars T Hansen [:lth]]

Comment on attachment 8712189 [details] [diff] [review]
2. JitSpew_WasmStats

Use JS_JITSPEW, not DEBUG, to control spewing please.  Spewing is useful in release.  (Also, isn't the #ifdef unnecessary here?)

+#ifdef DEBUG
+    JitSpew(JitSpew_WasmStats, "Total bytecode size for module: %lu", uint64_t(totalBytecodeSize_));
+    JitSpew(JitSpew_WasmStats, "Biggest bytecode size for module: %lu", uint64_t(biggestBytecodeSize_));
+    JitSpew(JitSpew_WasmStats, "Total number of functions: %u", funcEntryOffsets_.length());
+#endif
+

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/778fb42dc4fe

Comment 13

[Pulsebot]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/778fb42dc4fe
https://hg.mozilla.org/mozilla-central/rev/b86853e8d604

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/778fb42dc4fe
https://hg.mozilla.org/mozilla-central/rev/b86853e8d604