Closed Bug 1416723 Opened 2 years ago Closed Last year

Remove SIMD.js


(Core :: JavaScript Engine, enhancement, P3)




Tracking Status
firefox58 --- wontfix
firefox63 --- fixed


(Reporter: bbouvier, Assigned: bbouvier)


(Blocks 1 open bug)


(Keywords: dev-doc-complete)


(2 files)

Could we remove SIMD.js? The spec has been removed from stage 3 [1], and there's a lot of code to handle it in our code base, as well as in Ionmonkey, etc. It's been Nightly only from the beginning, the jit bits haven't been implemented on ARM32 or 64.

Jukka, I guess this might be useful for testing purposes in asm.js, especially since this is the only way to use it at the moment (it's not implemented in our wasm codebase); is that the case? Otherwise, we could disable it from asm.js too (keeping most of the jit/ implementation code so it's easy to revive when it comes to wasm).

This also sounds like a module decision, so asking for approval from Jason too.

I'd be happy to do the patch removing all but the jit/ implementation bits we could reuse.

Flags: needinfo?(jujjyl)
Flags: needinfo?(jorendorff)
Just to explain why I'm suggesting this now and not before: my hope was we could keep SIMD.js until we had a full implementation of SIMD in wasm, probably under Cretonne. But there's no practical timeline in terms of implementing SIMD in SM and/or cretonne, and there are security issues now, and there are a few costs now: CPU time when running test cases, maintenance for engineers, memory runtime cost in SM (even though this should be very lazy), etc. So it could be the right time.
Is the question here to drop SIMD code from being able to execute outside a well-formed asm.js module? Or both in handwritten JS and in asm.js?

In -O0 and in some other cases that might be depending on the scenario (e.g. using asm.js multithreading), one may be running  in non-asm.js validating mode, so retaining the capability to run SIMD.js API outside asm.js would be useful. Although we do compile the output code with the ecmascript-simd.js polyfill embedded in it, so it might be possible that if the non-asm.js path was dropped that we could keep using the polyfill. The ecmascript-simd.js polyfill is something like 200x slower though, so it is quite difficult to run code on it that relies on SIMD heavily.

We do occassionally get bug reports from Emscripten users about SIMD bugs in the toolchain (most recently when I regressed SIMD compilation last week by accident, there were a couple of people who complained almost immediately), so we do have a feeling that there are developers out there who are actively testing against compiling their code over to SIMD in FF Nightly.

I don't think there is a misunderstanding on their part, but they know it is an unreleased feature, and they are just "bleeding edge" developers poking on the latest in-development features rather than developers who would be shipping to the web in wide. Experience shows that treating these types of enthusiasts well is productive, since some of the bugs we receive give very valuable insight to what is working out and what is not.

As an example, previously when we disrupted developers targeting asm.js multithreading in Nightly, by introducing an environment variable that needed to be set in order for asm.js multithreading to work, that was considered awkward and put off a couple of high profile contributors who mentioned "I guess I'll just come back to this when it's ready". And in that feature, we did not even remove asm.js multithreading support, just put it behind a special mode.

My understanding from talking with Dan Gohman in SF All Hands is that SIMD-in-Wasm would still be planned to be more or less "SIMD.js opcodes + new opcodes for 64-bit int related instructions that we now can comfortably do", so the feature set that will be provided there will be close to what current asm.js gives. Is that still expected to be accurate?

If that is still the case, and if maintaining existing SIMD.js support is not a big overhead at present, I would favor keeping SIMD.js available in Nightly, because that would allow these developers to have an uninterrupted platform to develop against, and they can expect to have something closely similar eventually available in Wasm, so that they can just toggle a -s WASM=1 build switch to achieve that once SIMD-in-Wasm ships.

If on the other hand it is expected that SIMD-in-Wasm will be looking like something completely different, then that might be a totally different scenario.
I fear I will be in merge hell if this removal happens before wasm atomics can land, so if we decide to go ahead with this can we wait until that's done?  Hopefully this week, maybe next week in the worst case.
Depends on: 1377576
The plan so far has been to remove SIMD.js not too long after SIMD.wasm is usable for testing so we don't have a gap during which everything regresses.  The assumption here is that there isn't much maintenance cost in keeping SIMD.js around.

One unresolved question is: will SIMD.wasm come before or after Cretonne is enabled?  If it was to come before (SSE-only), then we would get some value out of continued testing of the Ion backend.  Otherwise, it'll all be new code so the only value is in the continued testing of Emscripten.

One question for Jukka/Dan/Alon: I recall hearing Google engineers have been working on SIMD.wasm support; is that only in the new upstream LLVM backend?  If so, then that presumably isn't being tested against asm.js either and if we assume that is the future way to produce SIMD.wasm from Emscripten, then maybe there isn't much value in testing SIMD.js even for Emscripten.
There is some SIMD work happening in the wasm backend, and none in asm2wasm (none planned there either).

But we do currently test the asm.js SIMD stuff in emscripten, as Jukka said earlier. I'm not sure how important that is.
No objection here.
Flags: needinfo?(jorendorff)
Priority: -- → P3
We discussed this internally and we've decided to go on and remove it everywhere, including in asm.js; we'll just keep MacroAssembler definitions so the wasm baseline compiler can use them at some point in the future. (Note: it might actually be moving implementations from codegen to macroassembler definitions, but the idea remains the same).

(Also clearning ni? from Jukka who answered in comment 2)
Assignee: nobody → bbouvier
Flags: needinfo?(jujjyl)
Alright, this is a big patch, but this is pretty mechanical. I've taken all the SIMD codegen methods and moved them to the masm, in a new file called MacroAssembler-x86-shared-SIMD.cpp.

Initially I wanted to have the file just checked in but not compiled, but it's actually a bad idea if we want to resurrect it later (because there might be a lot of other API changes that imply that this one will need to be almost rewritten).

I don't expect a line-by-line review from this: it's pretty mechanical and mostly code motion and interface changes. This is a transitional patch that was made to make sure that the methods were properly ported to the MacroAssembler. So I could make sure that all tests would pass on x64 with this patch (i.e. without the SIMD.js removal patch that's about to come).

A look at the headers file will give you an idea of the new interface.
Attachment #8994893 - Flags: review?(lhansen)
Comment on attachment 8994893 [details] [diff] [review]

Review of attachment 8994893 [details] [diff] [review]:

::: js/src/jit/x86-shared/MacroAssembler-x86-shared-SIMD.cpp
@@ +60,5 @@
> +    jump(rejoin);
> +}
> +
> +void
> +MacroAssemblerX86Shared::checkedConvertFloat32x4ToUint32x4(FloatRegister in, FloatRegister out, Register temp, FloatRegister tempF, Label* failed)

nit: wrap
r? nbp for the jit/ directory, luke for the rest.

I've let all the LIR level, regalloc and stack alignment details, so that it's not too hard to re-enable this if/when we add SIMD in wasm back again in Ion.

The SIMD masm methods do live in x86-shared/MacroAssembler-x86-shared{,-SIMD}.cpp, so the wasm baseline compiler can use them when we need it.

187 files changed, 168 insertions(+), 29019 deletions(-)
Attachment #8994895 - Flags: review?(nicolas.b.pierron)
Attachment #8994895 - Flags: review?(luke)
Comment on attachment 8994895 [details] [diff] [review]

Review of attachment 8994895 [details] [diff] [review]:

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +88,5 @@
>          MOZ_ASSERT(graph->argumentSlotCount() == 0);
>          frameDepth_ += gen->wasmMaxStackArgBytes();
> +        static_assert(!SupportsSimd, "we need padding so that local slots are SIMD-aligned and "
> +                                     "the stack must be kept SIMD-aligned too.");

Thanks for adding this comment!

::: js/src/vm/JSContext.cpp
@@ +39,5 @@
>  #include "jit/PcScriptCache.h"
>  #include "js/CharacterEncoding.h"
>  #include "js/Printf.h"
> +#ifdef JS_SIMULATOR_ARM64
> +# include "jit/arm64/vixl/Simulator-vixl.h"

nit: Add the ARM simulator too.
Attachment #8994895 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8994895 [details] [diff] [review]

Review of attachment 8994895 [details] [diff] [review]:

Never has the phrase been spoken more truly (only ever to be eclipsed by the asm.js-removal patch): fire ze misiles.
Attachment #8994895 - Flags: review?(luke) → review+
Comment on attachment 8994893 [details] [diff] [review]

Review of attachment 8994893 [details] [diff] [review]:

Basically a rubber stamp, but looks good to me.

::: js/src/jit/x86-shared/MacroAssembler-x86-shared.h
@@ +14,5 @@
>  #elif defined(JS_CODEGEN_X64)
>  # include "jit/x64/Assembler-x64.h"
>  #endif
> +using mozilla::Maybe;

This seems to cover only four cases in the header and is now exposed outside of any namespace; do we really need this?
Attachment #8994893 - Flags: review?(lhansen) → review+
Pushed by
Move SIMD code generation to masm methods; r=lth
Remove SIMD.js support; r=luke, r=nbp
Thanks for the quick reviews! Try push was green on the first try, let's fire ze misiles.
Backout by
Backed out 2 changesets for failures in  dom/serviceworkers/test/test_serviceworker_interfaces.html on a CLOSED TREE
Backed out 2 changesets (Bug 1416723) for failures in dom/serviceworkers/test/test_serviceworker_interfaces.html on a CLOSED TREE


Failure log:

11:49:14     INFO -  Buffered messages finished
11:49:14    ERROR -  535 INFO TEST-UNEXPECTED-FAIL | dom/serviceworkers/test/test_serviceworker_interfaces.html | false: SIMD should  be defined on the global scope
11:49:14     INFO -      setupSW/window.onmessage@dom/serviceworkers/test/test_serviceworker_interfaces.html:31:9
11:49:14     INFO -      EventHandlerNonNull*setupSW@dom/serviceworkers/test/test_serviceworker_interfaces.html:19:5
11:49:14     INFO -  536 INFO TEST-PASS | dom/serviceworkers/test/test_serviceworker_interfaces.html | true: NetworkInformation should  NOT be defined on the global scope
11:49:14     INFO -  Not taking screenshot here: see the one that was previously logged
11:49:14    ERROR -  537 INFO TEST-UNEXPECTED-FAIL | dom/serviceworkers/test/test_serviceworker_interfaces.html | 1 === 0: The following interface(s) are not enumerated: SIMD
11:49:14     INFO -      setupSW/window.onmessage@dom/serviceworkers/test/test_serviceworker_interfaces.html:31:9
Flags: needinfo?(bbouvier)
Ha hum, a few DOM tests actually check for the presence of the SIMD global object on Nightly in the main global + on workers.
Pushed by
Move SIMD code generation to masm methods; r=lth
Remove SIMD.js support; r=luke, r=nbp
Flags: needinfo?(bbouvier)
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Summary: Consider removing SIMD.js? → Remove SIMD.js
From February 2018, the following pages had a banner at the top of the pages saying that SIMD will be removed soon.
I have now deleted all these pages and their translations.

SIMD still appears in the JS sidebar doc navigation. That will be gone once is deployed to MDN prod.
You need to log in before you can comment on or make changes to this bug.