Bug 218954

Created attachment 414545 [details]
Patch

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 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?

Created attachment 414747 [details]
Patch

Created attachment 414750 [details]
Patch

Created attachment 414752 [details]
Patch

Created attachment 414753 [details]
Patch

Created attachment 414758 [details]
Patch

Created attachment 414771 [details]
Patch

<rdar://problem/71663019>

Created attachment 414785 [details]
Patch

Created attachment 414795 [details]
Patch

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

Created attachment 414818 [details]
Patch

Created attachment 414826 [details]
Patch

Created attachment 414827 [details]
Patch

Created attachment 414828 [details]
Patch

Created attachment 414829 [details]
Patch

Created attachment 414830 [details]
Patch

Created attachment 414851 [details]
Patch

Created attachment 414858 [details]
Patch

Created attachment 414867 [details]
Patch

Created attachment 414911 [details]
Patch

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

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

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

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