[PATCH] D58742: [WebAssembly] Remove uses of ThreadModel

[Thomas Lively via Phabricator via cfe-commits]

tlively marked an inline comment as done.
tlively added a comment.

In D58742#1413077 <https://reviews.llvm.org/D58742#1413077>, @sunfish wrote:

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



================
Comment at: clang/include/clang/Driver/ToolChain.h:456
   /// getThreadModel() - Which thread model does this target use?
-  virtual std::string getThreadModel(const llvm::opt::ArgList &) const {
-    return "posix";
-  }
+  virtual std::string getThreadModel() const { return "posix"; }
 
----------------
aheejin wrote:
> I think now we can delete this overridden method, because the default value for this is "posix". See Dan's [[ https://github.com/llvm/llvm-project/commit/1384ee936e46816f348bc3756704aeaff92e86fe | commit ]].
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58742/new/

https://reviews.llvm.org/D58742