[WebAssembly] Make thread-related options consistent

[aheejin]

Summary

There have been three options related to threads and users had to set all three of them separately to get the correct compilation results. This makes sure the relationship between the options makes sense and sets necessary options for users if only part of the necessary options are specified. This does:

• Remove -matomics; this option alone does not enable anything, so removed it to not confuse users.
• -mthread-model posix sets -target-feature +atomics
• -pthread sets both -target-feature +atomics and -mthread-model posix

Also errors out when explicitly given options don't match, such as -pthread is given with -mthread-model single.

[aheejin]

• Small fix

[dschuff]

So this CL has the effect that setting -pthreads will also set -matomics.
Currently as you mentioned we have the problem that we can't make our current logic of "do we lower away the atomics" be controlled by the target features because it's done at pass config time and not codegen time (so there's no access to the per-function subtarget).
Also IIUC on ARM, it's -mthread-model that controls generation of atomic instructions, so it makes sense that -mthread-model does the same thing for Wasm. But it's also true that for wasm we use -m<feature> to enable codegen for a particular wasm feature, so it would be good to make -matomics do the same for consistency.
So, can we just pin those 2 flags together here? i.e. setting one will just cause the other to be set?
(I guess we'd also need consistency check so that if someone does something like -mthread-model=posix -mno-atomics we either throw an error or make it so that one of those always overrides the other).

[dschuff]

Oh I guess another option would just be to pin all 3 flags together here, but since -pthread sets a preprocessor define and may also affect linker behavior, I think it's fine to allow atomic codegen without setting -pthread.

[sbc100]

Yes it makes sense to me to set -mthread-model=posix when -pthread is passed on the commandline.

[sunfish]

That sounds reasonable to me too.

[aheejin]

Sorry nevermind my previous code. There was not hacky and much cleaner way to do everything in the driver layer. (Before I tried to do everything in the cc1 compilation layer :( )

Anyway, moved all logic to the driver layer and did this:

• -matomics means -mthread-model posix
• -mthread-model posix means -matomics
• -pthread means both -matomics and -mthread-model posix

It currently does not check mismatches and crash. If either of -matomics or -mthread-model posix is set it is gonna set both. THe same for -pthread. Not sure which flag is the *canonical* one to trust.

[aheejin]

lib/Driver/ToolChains/WebAssembly.cpp

66 ↗

(On Diff #185895)

This code is not strictly related, but hasFlag is better than hasArg when there are both positive and negative versions of an option exist.

[sunfish]

• -matomics means -mthread-model posix

The others sound reasonable, though this one seems a little surprising -- a user might have -matomics enabled because they're targeting a VM that has atomics, but still not want to use -mthread-model posix because their code doesn't actually using threads.

[sbc100]

lib/Driver/ToolChains/WebAssembly.cpp

66 ↗

(On Diff #185895)

Hmm.. there are currently no other references to OPT_no_pthread in the whole codebase. Maybe better to simply remove the option?

I wouldn't want to commit this as that first use of the option as it might make it hard to remove :)

201 ↗

(On Diff #185895)

We are currently the only platform that overrides this.. i hope it can be removed completely at some point ..

[sbc100]

In D57874#1389981, @sunfish wrote:

• -matomics means -mthread-model posix

The others sound reasonable, though this one seems a little surprising -- a user might have -matomics enabled because they're targeting a VM that has atomics, but still not want to use -mthread-model posix because their code doesn't actually using threads.

If you look at what "-mthread-model posix" actually means in the codebase it basically means "don't lower away atomics ops".

Its only used by WebAssembly and ARMTargetMachine to select the createLowerAtomicPass. Given that these are the only uses it seems like it should be removed or renamed.

[tlively]

lib/Driver/ToolChains/WebAssembly.cpp

66 ↗

(On Diff #185895)

I think commands generally come in pairs to make it possible to override previous settings by appending args to command lines. Counting uses of OPT_no_pthread without including uses of OP_pthread doesn't make much sense.

[tlively]

In D57874#1389953, @aheejin wrote:

• -matomics means -mthread-model posix

Why should this be the case? Atomic instructions are necessary for multithreading, but I wouldn't think multithreading would be necessary for atomic instructions. Certainly atomics are not very useful in a single threaded context, but that doesn't seem like a strong enough reason to implicitly opt in to the rest of multithreading when -matomics is provided by itself.

[tlively]

In D57874#1391116, @tlively wrote:

In D57874#1389953, @aheejin wrote:

• -matomics means -mthread-model posix

Why should this be the case? Atomic instructions are necessary for multithreading, but I wouldn't think multithreading would be necessary for atomic instructions. Certainly atomics are not very useful in a single threaded context, but that doesn't seem like a strong enough reason to implicitly opt in to the rest of multithreading when -matomics is provided by itself.

Sorry, missed previous discussion.

[tlively]

In D57874#1389953, @aheejin wrote:

Anyway, moved all logic to the driver layer and did this:

• -matomics means -mthread-model posix
• -mthread-model posix means -matomics
• -pthread means both -matomics and -mthread-model posix

If you replace "-matomics" with "-mbulk-memory," I plan to duplicate the logic for items 2 and 3 above, but not 1. For bulk memory even more than atomics, there is a legitimate usecase for enabling it even in single threaded contexts (namely getting to use memory.copy and memory.fill). I wonder if consistency with how bulk memory works is a strong enough argument for dropping item 1 for atomics as well.

[tlively]

In D57874#1390014, @sbc100 wrote:

In D57874#1389981, @sunfish wrote:

• -matomics means -mthread-model posix

The others sound reasonable, though this one seems a little surprising -- a user might have -matomics enabled because they're targeting a VM that has atomics, but still not want to use -mthread-model posix because their code doesn't actually using threads.

If you look at what "-mthread-model posix" actually means in the codebase it basically means "don't lower away atomics ops".

Its only used by WebAssembly and ARMTargetMachine to select the createLowerAtomicPass. Given that these are the only uses it seems like it should be removed or renamed.

FWIW, I plan to use the thread-model argument to determine what kind of initialization flags data segments should use as well. Will share a short doc detailing this soon.

[aheejin]

• Fix some bugs
• Make the driver error out when explicitly given options don't match, such as -no-pthread and -matomics are given at the same time

[aheejin]

• Small fix

[aheejin]

lib/Driver/ToolChains/WebAssembly.cpp

66 ↗

(On Diff #185895)

Most true/false or enable/disable options exist in pairs. -no-pthread is defined here. So this ArgList::hasFlag function checks the existence of both the positive option and the negative option at the same time, and if neither exists, takes the default value, which is the third argument. So yes, as @tlively said, it's just a safety measure. Before it only checked the existence of -pthread and didn't care if -no-pthread existed or not.

[aheejin]

In D57874#1389981, @sunfish wrote:

• -matomics means -mthread-model posix

The others sound reasonable, though this one seems a little surprising -- a user might have -matomics enabled because they're targeting a VM that has atomics, but still not want to use -mthread-model posix because their code doesn't actually using threads.

As @sbc100 said, -mthread-model is only used in ARM and wasm backend to determine whether to lower away atomic instructions or not. So that the user gave -matomics means the VM can support atomic instructions, so even if we are not actually using threads, it's fine because the VM can handle them.

[aheejin]

• Initialized ThreadModel vairables with ""

[aheejin]

I only added those test routines in wasm-toolchain.c and not wasm-toolchain.cpp, because they are basically the same.

[tlively]

lib/Driver/ToolChains/WebAssembly.cpp

62 ↗

(On Diff #186065)

I'm not sure about this one, either. What if I want atomics for multithreading but I don't want to link with libpthread?

79 ↗

(On Diff #186065)

Same question here.

50 ↗

(On Diff #186063)

Should this logic use getLastArg or perhaps getLastArgNoClaim to check only that the final requested configuration is consistent rather than checking all intermediate configurations?

[sbc100]

lib/Driver/ToolChains/WebAssembly.cpp

66 ↗

(On Diff #185895)

There no current usage of OPT_no_pthread anywhere in clang. For this reason I think this option is ripe for removal completely.

Therefore I don't think its a good idea to introduce a usage here, as it could make the removal more difficult. Especially as this part of the patch is kind of unrelated.. its kind of an authogonal cleanup .. which I think makes things worse. So I vote to revert this line :)

[sbc100]

lib/Driver/ToolChains/WebAssembly.cpp

50 ↗

(On Diff #186063)

Can you remove all the "clang::driver" namspace qualification here since there is a "using" above?

[aheejin]

In D57874#1391133, @tlively wrote:

In D57874#1389953, @aheejin wrote:

Anyway, moved all logic to the driver layer and did this:

• -matomics means -mthread-model posix
• -mthread-model posix means -matomics
• -pthread means both -matomics and -mthread-model posix

If you replace "-matomics" with "-mbulk-memory," I plan to duplicate the logic for items 2 and 3 above, but not 1. For bulk memory even more than atomics, there is a legitimate usecase for enabling it even in single threaded contexts (namely getting to use memory.copy and memory.fill). I wonder if consistency with how bulk memory works is a strong enough argument for dropping item 1 for atomics as well.

Then now -mthread-model posix means both -matomics and -mbulk-memory, -matomics means -mthread-model posix, -mbulk-memory means -mthread-model posix but NOT -matomics means mbulk-memory and vice versa, right? Oh god it's complicated.... BTW if you are planning to use -mthread-model for turning on and off bulk memory too, why not 1?

[tlively]

In D57874#1391384, @aheejin wrote:

In D57874#1391133, @tlively wrote:

In D57874#1389953, @aheejin wrote:

Anyway, moved all logic to the driver layer and did this:

• -matomics means -mthread-model posix
• -mthread-model posix means -matomics
• -pthread means both -matomics and -mthread-model posix

If you replace "-matomics" with "-mbulk-memory," I plan to duplicate the logic for items 2 and 3 above, but not 1. For bulk memory even more than atomics, there is a legitimate usecase for enabling it even in single threaded contexts (namely getting to use memory.copy and memory.fill). I wonder if consistency with how bulk memory works is a strong enough argument for dropping item 1 for atomics as well.

Then now -mthread-model posix means both -matomics and -mbulk-memory, -matomics means -mthread-model posix, -mbulk-memory means -mthread-model posix but NOT -matomics means mbulk-memory and vice versa, right? Oh god it's complicated.... BTW if you are planning to use -mthread-model for turning on and off bulk memory too, why not 1?

We talked about this offline, but recapping here. We can't have -mbulk-memory imply -thread-model posix because a user might want bulk memory for access to instructions like memory.copy and memory.set (which are just more efficient primitives for memcpy and memset), but NOT want multithreading.

[aheejin]

I had an offline discussion with @tlively and @dschuff and decided to remove -matomics option in the driver. Instead, clang -cc1's -target-feature +atomics will be set if either of -pthread or -mthread-model posix is given. This is to reduce the total number of options and thus the total number of combinations of options. Sorry for frequent changes. Also addressed comments.

[aheejin]

• Replace -matomics with -mthread-model posix in preprocessor test

[aheejin]

In D57874#1389981, @sunfish wrote:

• -matomics means -mthread-model posix

The others sound reasonable, though this one seems a little surprising -- a user might have -matomics enabled because they're targeting a VM that has atomics, but still not want to use -mthread-model posix because their code doesn't actually using threads.

FYI, we don't have -matomics anymore.

lib/Driver/ToolChains/WebAssembly.cpp

62 ↗

(On Diff #186065)

Yeah good catch.

50 ↗

(On Diff #186063)

@sbc100 Yeah good catch.

50 ↗

(On Diff #186063)

@tlively I used getLastArgValue when I get the thread model string above:

ThreadModel = DriverArgs.getLastArgValue(
      clang::driver::options::OPT_mthread_model, "single");

This takes the last occurrence of this argument, and the second argument is the default value when the user didn't specify that option.

Here I used hasArg just to determine whether the user gave it explicitly or not, because we error out only when a user explicitly gives conflicting set of options.

[tlively]

lib/Driver/ToolChains/WebAssembly.cpp

29 ↗

(On Diff #186088)

Shouldn't every use of hasFlag be getLastArgValue instead?

36 ↗

(On Diff #186088)

It doesn't matter whether the user included the -pthread flag if they later included the -no-pthread flag.

42 ↗

(On Diff #186088)

This should be ThreadModel == "single", since the distinction we care about is single-threaded vs no single-threaded. If in the future there are dozens of thread models besides "posix" this logic should still work.

157 ↗

(On Diff #186088)

Same here. Should compare ThreadModel with "single".

[aheejin]

lib/Driver/ToolChains/WebAssembly.cpp

29 ↗

(On Diff #186088)

hasFlag is a convenient way to check everything with one function call. with getLastArgValue we have to call it twice (for example, for -pthread and for -no-pthread), and most importantly when both of them are given, calling getLastArgValue doesn't give you information on which one is the last. hasFlag takes care of that by taking the last one when both of them are given. So -pthread -no-pthread will return false, and -no-pthread -pthread will return true.

The reason I used hasArg below is just to check if the user gave it explicitly or not. And that's the reason I named variables Pthread and HasPthread.

36 ↗

(On Diff #186088)

This HasThreadModel is only used with HasPthread below.

if (HasPthread && HasThreadModel && ThreadModel != "posix")

So this is just to check if this thread model value came from the default value or the user explicitly gave it.

[aheejin]

• Address comments

[tlively]

lib/Driver/ToolChains/WebAssembly.cpp

29 ↗

(On Diff #186088)

I see! I had misunderstood hasFlag. Thanks for explaining it.

[aheejin]

If people have opinions on this final version, please let me know.

cfe/trunk/include/clang/Driver/Options.td

 def mnontrapping_fptoint : Flag<["-"], "mnontrapping-fptoint">, Group<m_wasm_Features_Group>;
 def mno_nontrapping_fptoint : Flag<["-"], "mno-nontrapping-fptoint">, Group<m_wasm_Features_Group>;
 def msign_ext : Flag<["-"], "msign-ext">, Group<m_wasm_Features_Group>;
 def mno_sign_ext : Flag<["-"], "mno-sign-ext">, Group<m_wasm_Features_Group>;
 def mexception_handing : Flag<["-"], "mexception-handling">, Group<m_wasm_Features_Group>;
 def mno_exception_handing : Flag<["-"], "mno-exception-handling">, Group<m_wasm_Features_Group>;
 def mbulk_memory : Flag<["-"], "mbulk-memory">, Group<m_wasm_Features_Group>;
 def mno_bulk_memory : Flag<["-"], "mno-bulk-memory">, Group<m_wasm_Features_Group>;
-def matomics : Flag<["-"], "matomics">, Group<m_wasm_Features_Group>;
-def mno_atomics : Flag<["-"], "mno-atomics">, Group<m_wasm_Features_Group>;
 
 def mamdgpu_debugger_abi : Joined<["-"], "mamdgpu-debugger-abi=">,
   Flags<[HelpHidden]>,
   Group<m_Group>,
   HelpText<"Generate additional code for specified <version> of debugger ABI (AMDGPU only)">,
   MetaVarName<"<version>">;
 
 def mcode_object_v3 : Flag<["-"], "mcode-object-v3">, Group<m_amdgpu_Features_Group>,

cfe/trunk/include/clang/Driver/ToolChain.h

public:
   /// GetExceptionModel - Return the tool chain exception model.
   virtual llvm::ExceptionHandling
   GetExceptionModel(const llvm::opt::ArgList &Args) const;
 
   /// SupportsEmbeddedBitcode - Does this tool chain support embedded bitcode.
   virtual bool SupportsEmbeddedBitcode() const { return false; }
 
   /// getThreadModel() - Which thread model does this target use?
-  virtual std::string getThreadModel() const { return "posix"; }
+  virtual std::string getThreadModel(const llvm::opt::ArgList &) const {
+    return "posix";
+  }
 
   /// isThreadModelSupported() - Does this target support a thread model?
   virtual bool isThreadModelSupported(const StringRef Model) const;
 
   /// ComputeLLVMTriple - Return the LLVM target triple to use, after taking
   /// command line arguments into account.
   virtual std::string
   ComputeLLVMTriple(const llvm::opt::ArgList &Args,

cfe/trunk/lib/Driver/Driver.cpp

void Driver::PrintVersion(const Compilation &C, raw_ostream &OS) const {
   OS << "Target: " << TC.getTripleString() << '\n';
 
   // Print the threading model.
   if (Arg *A = C.getArgs().getLastArg(options::OPT_mthread_model)) {
     // Don't print if the ToolChain would have barfed on it already
     if (TC.isThreadModelSupported(A->getValue()))
       OS << "Thread model: " << A->getValue();
   } else
-    OS << "Thread model: " << TC.getThreadModel();
+    OS << "Thread model: " << TC.getThreadModel(C.getArgs());
   OS << '\n';
 
   // Print out the install directory.
   OS << "InstalledDir: " << InstalledDir << '\n';
 
   // If configuration file was used, print its path.
   if (!ConfigFile.empty())
     OS << "Configuration file: " << ConfigFile << '\n';

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

#endif
   CmdArgs.push_back("-mthread-model");
   if (Arg *A = Args.getLastArg(options::OPT_mthread_model)) {
     if (!TC.isThreadModelSupported(A->getValue()))
       D.Diag(diag::err_drv_invalid_thread_model_for_target)
           << A->getValue() << A->getAsString(Args);
     CmdArgs.push_back(A->getValue());
   }
   else
-    CmdArgs.push_back(Args.MakeArgString(TC.getThreadModel()));
+    CmdArgs.push_back(Args.MakeArgString(TC.getThreadModel(Args)));
 
   Args.AddLastArg(CmdArgs, options::OPT_fveclib);
 
   if (Args.hasFlag(options::OPT_fmerge_all_constants,
                    options::OPT_fno_merge_all_constants, false))
     CmdArgs.push_back("-fmerge-all-constants");
 
   if (Args.hasFlag(options::OPT_fno_delete_null_pointer_checks,

cfe/trunk/lib/Driver/ToolChains/WebAssembly.h

private:
   void
   AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                             llvm::opt::ArgStringList &CC1Args) const override;
   void AddClangCXXStdlibIncludeArgs(
       const llvm::opt::ArgList &DriverArgs,
       llvm::opt::ArgStringList &CC1Args) const override;
   void AddCXXStdlibLibArgs(const llvm::opt::ArgList &Args,
                            llvm::opt::ArgStringList &CmdArgs) const override;
-  std::string getThreadModel() const override;
+  std::string getThreadModel(const llvm::opt::ArgList &) const override;
 
   const char *getDefaultLinker() const override { return "wasm-ld"; }
 
   Tool *buildLinker() const override;
 };
 
 } // end namespace toolchains
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_WEBASSEMBLY_H

cfe/trunk/lib/Driver/ToolChains/WebAssembly.cpp

 #include "llvm/Option/ArgList.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang::driver::toolchains;
 using namespace clang;
 using namespace llvm::opt;
 
+void parseThreadArgs(const Driver &Driver, const ArgList &DriverArgs,
+                     bool &Pthread, StringRef &ThreadModel,
+                     bool CheckForErrors = true) {
+  // Default value for -pthread / -mthread-model options, each being false /
+  // "single".
+  Pthread =
+      DriverArgs.hasFlag(options::OPT_pthread, options::OPT_no_pthread, false);
+  ThreadModel =
+      DriverArgs.getLastArgValue(options::OPT_mthread_model, "single");
+  if (!CheckForErrors)
+    return;
+
+  // Did user explicitly specify -mthread-model / -pthread?
+  bool HasThreadModel = DriverArgs.hasArg(options::OPT_mthread_model);
+  bool HasPthread = Pthread && DriverArgs.hasArg(options::OPT_pthread);
+  // '-pthread' cannot be used with '-mthread-model single'
+  if (HasPthread && HasThreadModel && ThreadModel == "single")
+    Driver.Diag(diag::err_drv_argument_not_allowed_with)
+        << "-pthread" << "-mthread-model single";
+}
+
 wasm::Linker::Linker(const ToolChain &TC)
     : GnuTool("wasm::Linker", "lld", TC) {}
 
 /// Following the conventions in https://wiki.debian.org/Multiarch/Tuples,
 /// we remove the vendor field to form the multiarch triple.
 static std::string getMultiarchTriple(const Driver &D,
                                       const llvm::Triple &TargetTriple,
                                       StringRef SysRoot) {

 bool WebAssembly::HasNativeLLVMSupport() const { return true; }
 
 void WebAssembly::addClangTargetOptions(const ArgList &DriverArgs,
                                         ArgStringList &CC1Args,
                                         Action::OffloadKind) const {
   if (DriverArgs.hasFlag(clang::driver::options::OPT_fuse_init_array,
                          options::OPT_fno_use_init_array, true))
     CC1Args.push_back("-fuse-init-array");
+
+  // Either '-mthread-model posix' or '-pthread' sets '-target-feature
+  // +atomics'. We intentionally didn't create '-matomics' and set the atomics
+  // target feature here depending on the other two options.
+  bool Pthread = false;
+  StringRef ThreadModel = "";
+  parseThreadArgs(getDriver(), DriverArgs, Pthread, ThreadModel);
+  if (Pthread || ThreadModel != "single") {
+    CC1Args.push_back("-target-feature");
+    CC1Args.push_back("+atomics");
+  }
 }
 
 ToolChain::RuntimeLibType WebAssembly::GetDefaultRuntimeLibType() const {
   return ToolChain::RLT_CompilerRT;
 }
 
 ToolChain::CXXStdlibType
 WebAssembly::GetCXXStdlibType(const ArgList &Args) const {
case ToolChain::CST_Libcxx:
     CmdArgs.push_back("-lc++");
     CmdArgs.push_back("-lc++abi");
     break;
   case ToolChain::CST_Libstdcxx:
     llvm_unreachable("invalid stdlib name");
   }
 }
 
-std::string WebAssembly::getThreadModel() const {
-  // The WebAssembly MVP does not yet support threads; for now, use the
-  // "single" threading model, which lowers atomics to non-atomic operations.
-  // When threading support is standardized and implemented in popular engines,
-  // this override should be removed.
-  return "single";
+std::string WebAssembly::getThreadModel(const ArgList &DriverArgs) const {
+  // The WebAssembly MVP does not yet support threads. We set this to "posix"
+  // when '-pthread' is set.
+  bool Pthread = false;
+  StringRef ThreadModel = "";
+  parseThreadArgs(getDriver(), DriverArgs, Pthread, ThreadModel, false);
+  if (Pthread)
+    return "posix";
+  return ThreadModel;
 }
 
 Tool *WebAssembly::buildLinker() const {
   return new tools::wasm::Linker(*this);
 }

cfe/trunk/test/Driver/wasm-toolchain.c

 // RUN: %clang -### -O2 -no-canonical-prefixes -target wasm32-unknown-wasi-musl --sysroot=/foo -fuse-ld=wasm-ld %s 2>&1 | FileCheck -check-prefix=LINK_OPT_KNOWN %s
 // LINK_OPT_KNOWN: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
 // LINK_OPT_KNOWN: wasm-ld{{.*}}" "-L/foo/lib/wasm32-wasi-musl" "crt1.o" "[[temp]]" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
 
 // A basic C compile command-line with known OS.
 
 // RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-wasi-musl --sysroot=/foo %s 2>&1 | FileCheck -check-prefix=COMPILE %s
 // COMPILE: clang{{.*}}" "-cc1" {{.*}} "-internal-isystem" "/foo/include/wasm32-wasi-musl" "-internal-isystem" "/foo/include"
+
+// Thread-related command line tests.
+// - '-mthread-model' sets '-target-feature +matomics'
+// - '-phread' sets both '-target-featuer +atomics' and '-mthread-model posix'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -mthread-model posix 2>&1 | FileCheck -check-prefix=POSIX %s
+// POSIX: clang{{.*}}" "-cc1" {{.*}} "-target-feature" "+atomics"
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -pthread 2>&1 | FileCheck -check-prefix=PTHREAD %s
+// PTHREAD: clang{{.*}}" "-cc1" {{.*}} "-mthread-model" "posix" {{.*}} "-target-feature" "+atomics"
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -pthread -mthread-model single 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR %s
+// THREAD_OPT_ERROR: invalid argument '-pthread' not allowed with '-mthread-model single'

cfe/trunk/test/Preprocessor/wasm-target-features.c

 // RUN:     -target wasm32-unknown-unknown -mbulk-memory \
 // RUN:   | FileCheck %s -check-prefix=BULK-MEMORY
 // RUN: %clang -E -dM %s -o - 2>&1 \
 // RUN:     -target wasm64-unknown-unknown -mbulk-memory \
 // RUN:   | FileCheck %s -check-prefix=BULK-MEMORY
 //
 // BULK-MEMORY:#define __wasm_bulk_memory__ 1{{$}}
 //
+// We don't use -matomics directly and '-mthread-model posix' sets the atomics
+// target feature.
 // RUN: %clang -E -dM %s -o - 2>&1 \
-// RUN:     -target wasm32-unknown-unknown -matomics \
+// RUN:     -target wasm32-unknown-unknown -mthread-model posix \
 // RUN:   | FileCheck %s -check-prefix=ATOMICS
 // RUN: %clang -E -dM %s -o - 2>&1 \
-// RUN:     -target wasm64-unknown-unknown -matomics \
+// RUN:     -target wasm64-unknown-unknown -mthread-model posix \
 // RUN:   | FileCheck %s -check-prefix=ATOMICS
 //
 // ATOMICS:#define __wasm_atomics__ 1{{$}}
 //
 // RUN: %clang -E -dM %s -o - 2>&1 \
 // RUN:     -target wasm32-unknown-unknown -mcpu=mvp \
 // RUN:   | FileCheck %s -check-prefix=MVP
 // RUN: %clang -E -dM %s -o - 2>&1 \