1333770 - Wasm baseline: Remove asm.js support from Rabaldr

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Lars T Hansen [:lth]]

A recent change shunts the compilation of asm.js code around the baseline compiler:

  changeset:   329691:b353d014c22b
  user:        Luke Wagner <[e-mail]>
  date:        Tue Jan 17 13:13:51 2017 -0600
  summary:     Bug 1330891 - Baldr: simplify ModuleGenerator (r=bbouvier)

In truth, that was just cleanup from an earlier change that entirely removed the ability of the baseline compiler to compile asm.js code (because the baseline compiler needs to know whether the asm.js code uses simd or atomics and that information is not available with concurrent validation):

  changeset:   328334:b4d14dfa3cf8
  user:        David Major <[e-mail]>
  date:        Fri Jan 06 17:15:48 2017 -0600
  summary:     Bug 1329018 - Part 2: Wasm: Move function validation to 
               helper threads. r=luke

I don't care for that change, because it makes test coverage of the baseline compiler strictly worse, but given that the change has been made (and given that concurrent verification is a real win), we should just remove the asm-js specific code paths in the baseline compiler.  Be sure to clean up code that only the asm.js paths use, such as the VolatileReturnGPR static.

Let's do this for the next release (hence P1).

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1333770-remove-asmjs.patch

Luke, is it a correct assumption that we will not need asm.js support in Rabaldr going forward?

Comment 2

[Luke Wagner [:luke]]

That's a good point about the raw loss of testing coverage for baseline.  A moderately-simple alternative would be to add a JITOptions gate for SIMD.js (just as with asm.js+Atomics) and then have the (Atomics-gate || SIMD-gate) disable baseline+asm.js which could be done upfront at the beginning ModuleGenerator.  If we can get wasm-embenchen into jit-tests as you suggested in the other bug, I think that would end up giving us a big boost in wasm test coverage so the asm.js coverage wouldn't be as valuable.  But to answer your question, I don't think it's necessary for rabaldr to support asm.js.

Comment 3

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #2)

> If we can get wasm-embenchen into jit-tests as you
> suggested in the other bug, I think that would end up giving us a big boost
> in wasm test coverage so the asm.js coverage wouldn't be as valuable.

I like that point of view, I'll make sure that gets done this cycle.

> But to answer your question, I don't think it's necessary for rabaldr to support
> asm.js.

Then I will ask for a review on the patch on this bug :-)

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1333770-remove-asmjs-v2.patch

Removes all asm.js support from Rabaldr, and asserts that we're not compiling asm.js when we enter.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8830791 [details] [diff] [review]
bug1333770-remove-asmjs-v2.patch

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

Mmm, that's nice.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +2471,5 @@
>          const SigWithId& sig = env_.sigs[sigIndex];
>  
>          CalleeDesc callee;
> +        MOZ_ASSERT(sig.id.kind() != SigIdDesc::Kind::None);
> +        MOZ_ASSERT(env_.tables.length() == 1);

super-nit: can you merge the declaration of 'callee' with the initialization below, remove the \n between the def of 'sig' and this MOZ_ASSERT() about sig.id.kind() and lastly add a \n before the MOZ_ASSERT() abut env_.tables?

Comment 6

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/505e39fbc5ed9857ba12c1c12608fc1cca5c43be
Bug 1333770 - wasm baseline, remove dead asm.js support. r=luke

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/505e39fbc5ed

Comment 8

[Lars T Hansen [:lth]]

There are some bytecodes that are only used by code generated from asm.js, which Rabaldr won't see now.  These bytecodes are not marked with "asm.js" in any obvious way in Rabaldr, so they were not removed by the patch on this bug.  I believe the following bytecodes are all implemented:

    TeeGlobal,
    I32Min,
    I32Max,
    I32Neg,
    I32BitNot,
    I32Abs,
    F32TeeStoreF64,
    F64TeeStoreF32,
    I32TeeStore8,
    I32TeeStore16,
    I64TeeStore8,
    I64TeeStore16,
    I64TeeStore32,
    I32TeeStore,
    I64TeeStore,
    F32TeeStore,
    F64TeeStore,
    F64Mod,
    F64Sin,
    F64Cos,
    F64Tan,
    F64Asin,
    F64Acos,
    F64Atan,
    F64Exp,
    F64Log,
    F64Pow,
    F64Atan2,

The question is how many of these we believe might show up in a later edition of WebAssembly.  I might be inclined to just keep these for now, at most comment them out or mark them with comments.  Luke, Benjamin -- opinions?

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Lars T Hansen [:lth] from comment #8)
> There are some bytecodes that are only used by code generated from asm.js,
> which Rabaldr won't see now.  These bytecodes are not marked with "asm.js"
> in any obvious way in Rabaldr, so they were not removed by the patch on this
> bug.  I believe the following bytecodes are all implemented:
> 
>     TeeGlobal,

I think this one would be nice to have in wasm, for symmetry with TeeLocal, but analysis on real-world programs compiled with emscripten should prove this would actually be useful. (That being said, a pattern of set_global/get_global would be nicely compressed, and the Sufficiently Smart Compiler should be able to fold this pattern into the semantic equivalent of a tee_global).

>     I32Min,
>     I32Max,

These don't map to a single CPU instruction; that's equivalent to one comparison and a conditional move (select opcode) which is worse in bytecode size, but closer to what's emitted in assembly.

>     I32Neg,
>     I32BitNot,
>     I32Abs,

I don't know about these three ones: 2 of them at least seem to have single CPU instruction, but there are ways to obtain them from other instructions (bitnot(x) = xor(x; -1)), but we don't seem to pattern-match these...

>     F32TeeStoreF64,
>     F64TeeStoreF32,
>     I32TeeStore8,
>     I32TeeStore16,
>     I64TeeStore8,
>     I64TeeStore16,
>     I64TeeStore32,
>     I32TeeStore,
>     I64TeeStore,
>     F32TeeStore,
>     F64TeeStore,

All these are just there to reproduce the behavior of JavaScript, which is that a store to a typedarray returns the stored value. Probably in wasm a local variable would be enough for this purpose, with a tee_local.

>     F64Mod,
>     F64Sin,
>     F64Cos,
>     F64Tan,
>     F64Asin,
>     F64Acos,
>     F64Atan,
>     F64Exp,
>     F64Log,
>     F64Pow,
>     F64Atan2,

IIRC, we expect the transcendental functions to be included in a libc variant that's compiled to wasm itself. All these functions are there for calling into the Math object in asm.js, so I don't think we should keep them. (there should be some f32 functions here too, iirc)

> 
> The question is how many of these we believe might show up in a later
> edition of WebAssembly.  I might be inclined to just keep these for now, at
> most comment them out or mark them with comments.  Luke, Benjamin --
> opinions?

Luke has more background on the criteria of choice that make that an instruction be included or not in wasm (bytecode size of an equivalent sequence, does it map to a single CPU instruction on most archs or not, is it a common pattern, the opcode space is limited so does it deserve its own opcode, etc.).

That being said, I'm pretty sure we could get rid of the math and teestore functions, at least. In general, I am more inclined to **not** have full chunks of code commented out, since it's technically dead code we're aware of, that won't get maintained (e.g. if the regalloc algorithm changes, or any manipulated data structure changes) nor tested. If we remove these now, we could just look at the patch that removed them and re-use that in the future, so I'm strongly in favor of removing.

Comment 10

[Luke Wagner [:luke]]

Agreed with Benjamin on removing all of these from the baseline.  I don't anticipate any of these being added to wasm in the near future and I also agree that dead code is best removed eagerly.

Comment 11

[Lars T Hansen [:lth]]

Reopening for additional work, let's keep the P1.

Comment 12

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> (In reply to Lars T Hansen [:lth] from comment #8)

> >     F64Mod,
> >     F64Sin,
> >     F64Cos,
> >     F64Tan,
> >     F64Asin,
> >     F64Acos,
> >     F64Atan,
> >     F64Exp,
> >     F64Log,
> >     F64Pow,
> >     F64Atan2,
> 
> ...
> (there should be some f32 functions here too, iirc)

There are only F64 variants in the current WasmBinaryConstants.h and the list I posted above is the full list of opcodes flagged as "asm.js", apart from atomics and simd and oldcallindirect.

oldcallindirect is implemented by the baseline compiler, and goes away too, I expect.

Comment 13

[Lars T Hansen [:lth]]

Attachment: bug1333770-remove-asmjs-part2.patch

Pretty straightforward, just remove unused code and inline templated functions that are now referenced only once.

I have also closed the bug that is referenced from a comment within the removed code.

Comment 14

[Lars T Hansen [:lth]]

Comment on attachment 8832422 [details] [diff] [review]
bug1333770-remove-asmjs-part2.patch

Removing r?, there's one more thing to do.

Comment 15

[Lars T Hansen [:lth]]

Attachment: bug1333770-remove-asmjs-part2-v2.patch

As the previous patch, but removes additional code and comments that only applied to the now-removed teeStore.  This allows us to close bug 1331264 regarding a possible optimization of that code; the optimization is now easily implemented and is part of the present patch.

Comment 16

[Lars T Hansen [:lth]]

Additionally emitCommonMathCall can be inlined into emitUnaryMathBuiltinCall.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8832435 [details] [diff] [review]
bug1333770-remove-asmjs-part2-v2.patch

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

LGTM, thanks.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +5762,5 @@
>  
>      endCall(baselineCall, stackSpace);
>  
>      // For new style calls, the callee was popped off the compiler's
>      // stack above.

This comment could be deleted now, or at least updated.

@@ +6310,1 @@
>      RegI32 tmp1 = temps >= 1 ? needI32() : invalidI32();

Drive-by remark, but it seems that if for some reason in the future temps > 1 (cause storeTemps decides to use more than 1), then we might need more than 1 register. Could we assert it's exactly 0 or 1, or explicitly fault when it's more than 1? (also, I don't get the `1` suffix in this variable name)

Comment 18

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> Comment on attachment 8832435 [details] [diff] [review]
> bug1333770-remove-asmjs-part2-v2.patch
> 
> Review of attachment 8832435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +6310,1 @@
> >      RegI32 tmp1 = temps >= 1 ? needI32() : invalidI32();
> 
> (also, I don't get the `1` suffix in this variable name)

Probable personal idiosyncracy: there are other places in the code that use the same pattern with multiple temps, and this keeps the naming consistent.  But an alternate explanation could be that there were multiple temps here before.

Either way I think you're right, I'll clean this up...

Comment 19

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de946df35c9758b89625198086c46cf27f96dca2
Bug 1333770 - Wasm baseline, remove remaining asm.js support. r=bbouvier

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/de946df35c97