Closed Bug 1389471 Opened 2 years ago Closed 2 years ago

Preliminaries for Wasm threads

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: lth, Assigned: lth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Changes to moz.build to define a suitable preprocessor macro (ENABLE_WASM_THREAD_OPS) and a TestingFunction to test for presence of threads.

This is small potatoes but I'm breaking this out as a separate bug since several of the other changes depend on it, and it can land independently really.
See comment 0.
Attachment #8896246 - Flags: review?(bbouvier)
Comment on attachment 8896246 [details] [diff] [review]
bug1389471-wasm-thread-preliminaries.patch

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

Thanks, LGTM.

::: js/src/builtin/TestingFunctions.cpp
@@ +533,5 @@
> +WasmThreadsSupported(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +#ifdef ENABLE_WASM_THREAD_OPS
> +    bool isSupported = true;

This is temporary that it's always true, right? e.g. for mips/arm64/none codegen, there won't be any atomics instructions any time soon, right?

(And I assume that probably other preconditions will apply here, other than just the ifdef)

::: js/src/moz.build
@@ +643,5 @@
> +# In-flight WebAssembly atomic operations, sign extension operations,
> +# and shared memory objects proposal:
> +# https://github.com/WebAssembly/threads
> +if CONFIG['NIGHTLY_BUILD']:
> +    DEFINES['ENABLE_WASM_THREAD_OPS'] = True

(might be ok to group with the above group of if[NIGHTLY], since it seems to be the common case in this file; although wasm thread ops are muuuuuch more likely to land in release sooner than binary data/simd, so it makes sense to start a new `if` group; I'll leave it to your preference)
Attachment #8896246 - Flags: review?(bbouvier) → review+
Excellent point about the always-trueness of the return value...  Hm.  The right fix is perhaps to engage in some ifdeffery on the moz.build level (or similar) so that for ARM64, MIPS, et al we don't risk anything... on the other hand, we MOZ_CRASH for JS atomics on ARM64 and MIPS and could probably do that here too...  Would certainly be cleaner.  Certainly the wasm atomics implementations will crash on those platforms.

At a minimum this should not return true if wasm support is disabled, oy!  Will fix.

Re the new CONFIG block, it was mainly to have a nice point for the comment.  And as you say, this stuff is likely to land soon :)
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3777a6f18c8a
Build config + testing predicate for wasm thread functionality, v2. r=bbouvier
https://hg.mozilla.org/mozilla-central/rev/3777a6f18c8a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.