Bug 218954 – [JSC] Add wasm atomics instructions

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

After WebAssembly.Memory+shared is done, I'll quickly do that.

[Yusuke Suzuki]

Created attachment 414545 [details]
Patch

[EWS Watchlist]

This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".

[Keith Miller]

Comment on attachment 414545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414545&action=review

> Source/JavaScriptCore/runtime/OptionsList.h:494
> +    v(Bool, useWebAssemblyThreading, true, Normal, "Allow types from the wasm threading spec.") \

I think this should be "allow instructions from the ..." since I don't think there's any new types in the threading spec?

[Yusuke Suzuki]

Created attachment 414747 [details]
Patch

[Yusuke Suzuki]

Created attachment 414750 [details]
Patch

[Yusuke Suzuki]

Created attachment 414752 [details]
Patch

[Yusuke Suzuki]

Created attachment 414753 [details]
Patch

[Yusuke Suzuki]

Created attachment 414758 [details]
Patch

[Yusuke Suzuki]

Created attachment 414771 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/71663019>

[Yusuke Suzuki]

Created attachment 414785 [details]
Patch

[Yusuke Suzuki]

Created attachment 414795 [details]
Patch

[Filip Pizlo]

Comment on attachment 414795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414795&action=review

Super nice!

> Source/JavaScriptCore/b3/B3Width.h:116
> +    return 1U << static_cast<unsigned>(width);

Slight preference for this being a switch.  It's clearer and less sensitive to the enum being messed with.

> Source/JavaScriptCore/b3/testb3_8.cpp:593
> +            // AtomicXchg can be lowered to "xchg" without "lock", and this is OK since "lock" signal is asserted for "xchg" by default.

Oh really?  I thought that xchg only created a fence by default, but not the lock.

[Yusuke Suzuki]

Comment on attachment 414795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414795&action=review

Thanks! I'll land it once tree is opened.

>> Source/JavaScriptCore/b3/B3Width.h:116
>> +    return 1U << static_cast<unsigned>(width);
> 
> Slight preference for this being a switch.  It's clearer and less sensitive to the enum being messed with.

OK, changed :)

>> Source/JavaScriptCore/b3/testb3_8.cpp:593
>> +            // AtomicXchg can be lowered to "xchg" without "lock", and this is OK since "lock" signal is asserted for "xchg" by default.
> 
> Oh really?  I thought that xchg only created a fence by default, but not the lock.

Yes, for xchg, lock is not necessary since this is by default.
https://stackoverflow.com/questions/3144335/on-a-multicore-x86-is-a-lock-necessary-as-a-prefix-to-xchg/3144416

> 8.1.2.1 Automatic Locking
> The operations on which the processor automatically follows the LOCK semantics are as follows:
> • When executing an XCHG instruction that references memory.
From Intel SDM Volume 3 8.1.2.1.

And asm generated from atomic.exchange (in C++) is just emitting xchg.
https://gcc.godbolt.org/z/5GxE3o
> exchange(std::atomic<unsigned int>, unsigned int):               # @exchange(std::atomic<unsigned int>, unsigned int)
>         movl    %esi, %eax
>         xchgl   %eax, (%rdi)
>         retq

[Yusuke Suzuki]

Created attachment 414818 [details]
Patch

[Yusuke Suzuki]

Created attachment 414826 [details]
Patch

[Yusuke Suzuki]

Created attachment 414827 [details]
Patch

[Yusuke Suzuki]

Created attachment 414828 [details]
Patch

[Yusuke Suzuki]

Created attachment 414829 [details]
Patch

[Yusuke Suzuki]

Created attachment 414830 [details]
Patch

[Yusuke Suzuki]

Created attachment 414851 [details]
Patch

[Yusuke Suzuki]

Created attachment 414858 [details]
Patch

[Yusuke Suzuki]

Created attachment 414867 [details]
Patch

[Yusuke Suzuki]

Created attachment 414911 [details]
Patch

[Yusuke Suzuki]

Committed r270208: <https://trac.webkit.org/changeset/270208>

[Derek Schuff]

Thanks, this is great! I'm not sure where is the best place to say this, but I just wanted you to be aware that in order to use this with emscripten, JSC also needs to implement part of the bulk memory spec (namely the "memory" part; specifically passive segments. the table-related bits are not needed). That's tracked as https://bugs.webkit.org/show_bug.cgi?id=200938

[Yusuke Suzuki]

(In reply to Derek Schuff from comment #26)
> Thanks, this is great! I'm not sure where is the best place to say this, but
> I just wanted you to be aware that in order to use this with emscripten, JSC
> also needs to implement part of the bulk memory spec (namely the "memory"
> part; specifically passive segments. the table-related bits are not needed).
> That's tracked as https://bugs.webkit.org/show_bug.cgi?id=200938

Thank you for letting me know! Currently, we are experimenting on wasm threading things but we are interested in bulk-memory support too.

[Derek Schuff]

Just for more background, I forgot to mention that the reason is that emscripten uses passive segments for the data section initializers when building with threads (this is so that it can instantiate the module for new threads without the initializers running again).