[WebAssembly] Make thread-related options consistent

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

Reviewers: dschuff, sbc100, tlively, sunfish

Subscribers: jgravelle-google, jfb, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D57874

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@353761 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Driver/Options.td b/include/clang/Driver/Options.td
index 9ebb65c2ef23fe13bcc3c957ad9689a18afee492..a6a6e3b5b53c56ff569877917a7eeed50451dcd8 100644 (file)
--- a/include/clang/Driver/Options.td
+++ b/include/clang/Driver/Options.td
@@ -2160,8 +2160,6 @@ def mexception_handing : Flag<["-"], "mexception-handling">, Group<m_wasm_Featur
 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]>,

diff --git a/include/clang/Driver/ToolChain.h b/include/clang/Driver/ToolChain.h
index eec49ec3dde5ab1446b9ef2613c0bcf0538c4d49..e82675d33455ff1dca71f675398bcc2cb827b4b3 100644 (file)
--- a/include/clang/Driver/ToolChain.h
+++ b/include/clang/Driver/ToolChain.h
@@ -453,7 +453,9 @@ public:
   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;

diff --git a/lib/Driver/Driver.cpp b/lib/Driver/Driver.cpp
index eb03e6e87cc02ccca3c5a287aa8a3c1403c03fa0..b405390d44e31e6edaa1e3e47d4db678e670f904 100644 (file)
--- a/lib/Driver/Driver.cpp
+++ b/lib/Driver/Driver.cpp
@@ -1528,7 +1528,7 @@ void Driver::PrintVersion(const Compilation &C, raw_ostream &OS) const {
     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.

diff --git a/lib/Driver/ToolChains/Clang.cpp b/lib/Driver/ToolChains/Clang.cpp
index 3b1ef3a5a8695e3c7cb581ab83be6173dbe7eb26..fc95003e41ba2e4061f89f1855b3abb851e507c9 100644 (file)
--- a/lib/Driver/ToolChains/Clang.cpp
+++ b/lib/Driver/ToolChains/Clang.cpp
@@ -3823,7 +3823,7 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
     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);
 

diff --git a/lib/Driver/ToolChains/WebAssembly.cpp b/lib/Driver/ToolChains/WebAssembly.cpp
index b89e043d88dab43eb20aa673f5ab05cf295b9fe5..f076b528ce4b8e8ff5b95ab1464c09feeda8e351 100644 (file)
--- a/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/lib/Driver/ToolChains/WebAssembly.cpp
@@ -20,6 +20,27 @@ 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) {}
 
@@ -123,6 +144,17 @@ void WebAssembly::addClangTargetOptions(const ArgList &DriverArgs,
   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 {
@@ -180,12 +212,15 @@ void WebAssembly::AddCXXStdlibLibArgs(const llvm::opt::ArgList &Args,
   }
 }
 
-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 {

diff --git a/lib/Driver/ToolChains/WebAssembly.h b/lib/Driver/ToolChains/WebAssembly.h
index 12126ed422d211ae27d1d5ab307661604ce1e947..6ededd84ec6f04fad8c54fe77a477375aa2a3ce0 100644 (file)
--- a/lib/Driver/ToolChains/WebAssembly.h
+++ b/lib/Driver/ToolChains/WebAssembly.h
@@ -64,7 +64,7 @@ private:
       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"; }
 

diff --git a/test/Driver/wasm-toolchain.c b/test/Driver/wasm-toolchain.c
index 39024d11ef6f5566a8fa9e4663dd411563a74247..22867117ffd53a466d1f2e510c268dcf78ddad54 100644 (file)
--- a/test/Driver/wasm-toolchain.c
+++ b/test/Driver/wasm-toolchain.c
@@ -38,3 +38,16 @@
 
 // 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'

diff --git a/test/Preprocessor/wasm-target-features.c b/test/Preprocessor/wasm-target-features.c
index 30bc76abae53e2d1f5c099476b1caedf9058ddcc..3ab22500c1504ff6d9b67407edf150936e31ac40 100644 (file)
--- a/test/Preprocessor/wasm-target-features.c
+++ b/test/Preprocessor/wasm-target-features.c
@@ -52,11 +52,13 @@
 //
 // 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{{$}}