Closed
Bug 1416723
Opened 7 years ago
Closed 6 years ago
Remove SIMD.js
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla63
People
(Reporter: bbouvier, Assigned: bbouvier)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(2 files)
(deleted),
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
nbp
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•7 years ago
|
||
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.
Comment 2•7 years ago
|
||
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.
Comment 3•7 years ago
|
||
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
Comment 4•7 years ago
|
||
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.
Comment 5•7 years ago
|
||
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.
Updated•7 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 7•6 years ago
|
||
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)
Assignee | ||
Comment 8•6 years ago
|
||
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)
Assignee | ||
Comment 9•6 years ago
|
||
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
Assignee | ||
Comment 10•6 years ago
|
||
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 11•6 years ago
|
||
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 12•6 years ago
|
||
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 13•6 years ago
|
||
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+
Comment 14•6 years ago
|
||
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
Assignee | ||
Comment 15•6 years ago
|
||
Thanks for the quick reviews! Try push was green on the first try, let's fire ze misiles.
Comment 16•6 years ago
|
||
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
Comment 17•6 years ago
|
||
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)
Assignee | ||
Comment 18•6 years ago
|
||
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
Comment 19•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(bbouvier)
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3d7a2ff6821f
https://hg.mozilla.org/mozilla-central/rev/4534ae540e86
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
Blocks: memshrink-content
Updated•6 years ago
|
Summary: Consider removing SIMD.js? → Remove SIMD.js
Updated•6 years ago
|
Comment 21•6 years ago
|
||
Regarding dev-doc-needed, it'd be good to entirely remove https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD.
Comment 22•6 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•