Closed Bug 1416723 Opened 6 years ago Closed 6 years ago

Remove SIMD.js

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox58 --- wontfix
firefox63 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(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.

[1] https://github.com/tc39/ecmascript_simd
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
Status: NEW → ASSIGNED
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]
1.move-codegen-to-masm.patch

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]
2.remove-simdjs.patch

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]
2.remove-simdjs.patch

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]
1.move-codegen-to-masm.patch

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 bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfaf82051dfd
Move SIMD code generation to masm methods; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2242216d11b
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 shindli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fd93c0985bb
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: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&fromchange=4bf70a81f42f205bd2b0c91da3f511ca39c2f118&selectedJob=190245918

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=190245918&repo=mozilla-inbound&lineNumber=10536

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.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=391d90618dc3f8c8d5f7f93def1f563f1e27b2d0
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d7a2ff6821f
Move SIMD code generation to masm methods; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/4534ae540e86
Remove SIMD.js support; r=luke, r=nbp
Flags: needinfo?(bbouvier)
https://hg.mozilla.org/mozilla-central/rev/3d7a2ff6821f
https://hg.mozilla.org/mozilla-central/rev/4534ae540e86
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Summary: Consider removing SIMD.js? → Remove SIMD.js
Regarding dev-doc-needed, it'd be good to entirely remove https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD.
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.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/SIMD_types

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/abs 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/add 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/addSaturate 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/allTrue 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/and 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/anyTrue 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/check 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/div 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/equal 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/extractLane 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromFloat32x4 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromFloat32x4Bits 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromFloat64x2Bits 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromInt16x8Bits 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromInt32x4 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromInt32x4Bits 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromInt8x16Bits 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromUint16x8Bits 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromUint32x4 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromUint32x4Bits 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromUint8x16Bits 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/greaterThan 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/greaterThanOrEqual 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/lessThan 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/lessThanOrEqual 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/load 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/max 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/maxNum 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/min 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/minNum 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/mul 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/neg 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/not 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/notEqual 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/or 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/reciprocalApproximation 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/reciprocalSqrtApproximation 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/replaceLane 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/select 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/shiftLeftByScalar 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/shiftRightByScalar 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/shuffle 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/splat 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/sqrt
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/store 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/sub 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/subSaturate 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/swizzle 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/toSource 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/toString 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/valueOf 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/xor 

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Bool16x8
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Bool32x4 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Bool64x2 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Bool8x16 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Float32x4 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Float64x2 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Int16x8 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Int32x4 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Int8x16 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint16x8 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint32x4 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8x16

SIMD still appears in the JS sidebar doc navigation. That will be gone once https://github.com/mdn/kumascript/pull/761 is deployed to MDN prod.
Blocks: 1620167
You need to log in before you can comment on or make changes to this bug.