Page MenuHomePhabricator

[WebAssembly] Remove uses of ThreadModel
ClosedPublic

Authored by tlively on Feb 27 2019, 3:03 PM.

Details

Summary

In the clang UI, replaces -mthread-model posix with -matomics as the
source of truth on threading. In the backend, replaces
-thread-model=posix with the atomics target feature, which is now
collected on the WebAssemblyTargetMachine along with all other used
features. These collected features will also be used to emit the
target features section in the future.

The default configuration for the backend is thread-model=posix and no
atomics, which was previously an invalid configuration. This change
makes the default valid because the thread model is ignored.

A side effect of this change is that objects are never emitted with
passive segments. It will instead be up to the linker to decide
whether sections should be active or passive based on whether atomics
are used in the final link.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

tlively created this revision.Feb 27 2019, 3:03 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 27 2019, 3:03 PM

I think I like this.

We may find a reason down the line to allow passive to be set per-segment, but now keeping it simple and lettings the linker decide makes sense.

llvm/test/CodeGen/WebAssembly/atomic-mem-consistency.ll
1 ↗(On Diff #188634)

What did this start passing? Is it because llc now has the correct default and will lower atomics to non-atomics? I guess there is no longer a way to make this fail in the way that it did before.. which is good?

sbc100 accepted this revision.Feb 27 2019, 3:30 PM
This revision is now accepted and ready to land.Feb 27 2019, 3:30 PM

I certainly like that llc now does the right thing by default!

tlively marked an inline comment as done.Feb 27 2019, 3:44 PM
tlively added inline comments.
llvm/test/CodeGen/WebAssembly/atomic-mem-consistency.ll
1 ↗(On Diff #188634)

Precisely :D

I'll wait and see if @aheejin has any concerns before landing.

aheejin accepted this revision.Feb 27 2019, 4:52 PM

This looks a nice improvement! Thank you.

clang/include/clang/Driver/ToolChain.h
456 ↗(On Diff #188634)

I think now we can delete this overridden method, because the default value for this is "posix". See Dan's commit.

clang/lib/Driver/ToolChains/WebAssembly.cpp
135 ↗(On Diff #188634)

This is nicer! Given that the default values for both options were false, we didn't need to care whether a user explicitly gave it or not after all.

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
150 ↗(On Diff #188634)

Aha, this is the info.

This is still a little confusing to me. -matomic is supposed to be a subtarget flag, stating that the wasm implementation we will run on supports atomic instructions. -mthread-model posix is about the C++ interpretation -- what style implementation of memory model do we want? In the future, -matomic may become enabled by default, when enough wasm engines generally support it. However, -mthread-model single/posix may still be useful to control independently, because even with wasm engines supporting atomic, there are reasons users might still want to compile their apps single-threaded: access to linear memory with no declared max, lower overall code size, or other things.

tlively marked an inline comment as done.Feb 27 2019, 6:22 PM

This is still a little confusing to me. -matomic is supposed to be a subtarget flag, stating that the wasm implementation we will run on supports atomic instructions. -mthread-model posix is about the C++ interpretation -- what style implementation of memory model do we want? In the future, -matomic may become enabled by default, when enough wasm engines generally support it. However, -mthread-model single/posix may still be useful to control independently, because even with wasm engines supporting atomic, there are reasons users might still want to compile their apps single-threaded: access to linear memory with no declared max, lower overall code size, or other things.

The only backend besides us that uses the ThreadModel at all is ARM, so we were questioning its generality. I originally shared your opinion that the available target features should be controlled separately from whether or not the user wants threads, but in reality we get to use the atomics feature if and only if the user opts in to threads. Because of this, ignoring the thread model entirely like most other backends lets us have a much simpler user interface that is not invalid by default.

I expect with this model we would never turn on atomics by default (since we don't want multithreaded by default), and most users would get them by using -pthread. This is very close in UI to native targets. The -matomics option is only there for users who are doing weird threaded things but don't want libpthread for some reason.

clang/include/clang/Driver/ToolChain.h
456 ↗(On Diff #188634)

I did delete the overridden method below in clang/lib/Driver/ToolChains/WebAssembly.cpp. This is restoring the base class implementation to the way it was before your CL added the ArgList argument, since we no longer need it.

aheejin added inline comments.Feb 27 2019, 9:45 PM
clang/include/clang/Driver/ToolChain.h
456 ↗(On Diff #188634)

Oh sorry, I thought this file was WebAssembly.cpp...

This revision was automatically updated to reflect the committed changes.

This is still a little confusing to me. -matomic is supposed to be a subtarget flag, stating that the wasm implementation we will run on supports atomic instructions. -mthread-model posix is about the C++ interpretation -- what style implementation of memory model do we want? In the future, -matomic may become enabled by default, when enough wasm engines generally support it. However, -mthread-model single/posix may still be useful to control independently, because even with wasm engines supporting atomic, there are reasons users might still want to compile their apps single-threaded: access to linear memory with no declared max, lower overall code size, or other things.

" -mthread-model posix is about the C++ interpretation" .. are you sure about that? git grep getThreadModel shows basically no use of this.

Digging a little deeper.. The thread-model thing was introduced here: https://reviews.llvm.org/D4984 and was clearly related to the underlying architecture. Apart from wasm nobody else has ever starting using this is given it any other meaning as far as I can tell. Maybe I'm missing something?

Wasm gives users reasons to want -mthread-model single that other architectures don't, even when -matomics is enabled by default.

When shared memory is used, wasm requires modules to declare a max memory size, which is a burden on applications that want to use dynamic amounts of memory. Wasm is more sensitive to code size than most other architectures. And, interoperating with single-threaded JS is easier from single-threaded wasm (JS has access to SharedArrayBuffer and atomics, but not all JS wants to use that).

Wasm gives users reasons to want -mthread-model single that other architectures don't, even when -matomics is enabled by default.

When shared memory is used, wasm requires modules to declare a max memory size, which is a burden on applications that want to use dynamic amounts of memory. Wasm is more sensitive to code size than most other architectures. And, interoperating with single-threaded JS is easier from single-threaded wasm (JS has access to SharedArrayBuffer and atomics, but not all JS wants to use that).

Yes, but I don't see how that is related to this change. I don't think there is a plan to make -matomics the default, and even if we did users would be free to disable it.

My understanding is that the default for clang and llvm and lld for the foreseeable will be to target single threaded mode, with non-shared-memory. This CL doesn't change that.