Bug 218954 - [JSC] Add wasm atomics instructions
Summary: [JSC] Add wasm atomics instructions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 218693
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-15 00:08 PST by Yusuke Suzuki
Modified: 2020-11-30 17:08 PST (History)
10 users (show)

See Also:


Attachments
Patch (75.54 KB, patch)
2020-11-18 23:22 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (316.35 KB, patch)
2020-11-20 18:07 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (343.70 KB, patch)
2020-11-20 19:23 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (339.40 KB, patch)
2020-11-20 22:15 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (340.37 KB, patch)
2020-11-20 22:24 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (347.93 KB, patch)
2020-11-21 00:00 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (348.10 KB, patch)
2020-11-21 15:54 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (363.43 KB, patch)
2020-11-22 01:34 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (515.19 KB, patch)
2020-11-22 22:20 PST, Yusuke Suzuki
fpizlo: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (515.55 KB, patch)
2020-11-23 18:26 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (517.38 KB, patch)
2020-11-24 03:02 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (517.42 KB, patch)
2020-11-24 03:24 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (517.52 KB, patch)
2020-11-24 03:33 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (517.34 KB, patch)
2020-11-24 04:04 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (517.63 KB, patch)
2020-11-24 04:31 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (517.88 KB, patch)
2020-11-24 13:38 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (517.92 KB, patch)
2020-11-24 16:16 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (517.93 KB, patch)
2020-11-25 12:34 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (517.93 KB, patch)
2020-11-26 18:37 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-11-15 00:08:03 PST
After WebAssembly.Memory+shared is done, I'll quickly do that.
Comment 1 Yusuke Suzuki 2020-11-18 23:22:08 PST
Created attachment 414545 [details]
Patch
Comment 2 EWS Watchlist 2020-11-18 23:22:49 PST
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".
Comment 3 Keith Miller 2020-11-19 03:37:10 PST
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?
Comment 4 Yusuke Suzuki 2020-11-20 18:07:12 PST
Created attachment 414747 [details]
Patch
Comment 5 Yusuke Suzuki 2020-11-20 19:23:30 PST
Created attachment 414750 [details]
Patch
Comment 6 Yusuke Suzuki 2020-11-20 22:15:32 PST
Created attachment 414752 [details]
Patch
Comment 7 Yusuke Suzuki 2020-11-20 22:24:12 PST
Created attachment 414753 [details]
Patch
Comment 8 Yusuke Suzuki 2020-11-21 00:00:38 PST
Created attachment 414758 [details]
Patch
Comment 9 Yusuke Suzuki 2020-11-21 15:54:00 PST
Created attachment 414771 [details]
Patch
Comment 10 Radar WebKit Bug Importer 2020-11-22 00:09:13 PST
<rdar://problem/71663019>
Comment 11 Yusuke Suzuki 2020-11-22 01:34:27 PST
Created attachment 414785 [details]
Patch
Comment 12 Yusuke Suzuki 2020-11-22 22:20:47 PST
Created attachment 414795 [details]
Patch
Comment 13 Filip Pizlo 2020-11-23 09:27:33 PST
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.
Comment 14 Yusuke Suzuki 2020-11-23 18:21:48 PST
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
Comment 15 Yusuke Suzuki 2020-11-23 18:26:06 PST
Created attachment 414818 [details]
Patch
Comment 16 Yusuke Suzuki 2020-11-24 03:02:58 PST
Created attachment 414826 [details]
Patch
Comment 17 Yusuke Suzuki 2020-11-24 03:24:50 PST
Created attachment 414827 [details]
Patch
Comment 18 Yusuke Suzuki 2020-11-24 03:33:57 PST
Created attachment 414828 [details]
Patch
Comment 19 Yusuke Suzuki 2020-11-24 04:04:55 PST
Created attachment 414829 [details]
Patch
Comment 20 Yusuke Suzuki 2020-11-24 04:31:18 PST
Created attachment 414830 [details]
Patch
Comment 21 Yusuke Suzuki 2020-11-24 13:38:46 PST
Created attachment 414851 [details]
Patch
Comment 22 Yusuke Suzuki 2020-11-24 16:16:58 PST
Created attachment 414858 [details]
Patch
Comment 23 Yusuke Suzuki 2020-11-25 12:34:43 PST
Created attachment 414867 [details]
Patch
Comment 24 Yusuke Suzuki 2020-11-26 18:37:26 PST
Created attachment 414911 [details]
Patch
Comment 25 Yusuke Suzuki 2020-11-27 16:03:22 PST
Committed r270208: <https://trac.webkit.org/changeset/270208>
Comment 26 Derek Schuff 2020-11-30 16:38:37 PST
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
Comment 27 Yusuke Suzuki 2020-11-30 16:54:39 PST
(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.
Comment 28 Derek Schuff 2020-11-30 17:08:54 PST
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).