[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL353761: [WebAssembly] Make thread-related options consistent 
(authored by aheejin, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57874

Files:
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/include/clang/Driver/ToolChain.h
  cfe/trunk/lib/Driver/Driver.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Driver/ToolChains/WebAssembly.cpp
  cfe/trunk/lib/Driver/ToolChains/WebAssembly.h
  cfe/trunk/test/Driver/wasm-toolchain.c
  cfe/trunk/test/Preprocessor/wasm-target-features.c

Index: cfe/trunk/test/Preprocessor/wasm-target-features.c
===
--- cfe/trunk/test/Preprocessor/wasm-target-features.c
+++ cfe/trunk/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{{$}}
Index: cfe/trunk/test/Driver/wasm-toolchain.c
===
--- cfe/trunk/test/Driver/wasm-toolchain.c
+++ cfe/trunk/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'
Index: cfe/trunk/lib/Driver/ToolChains/WebAssembly.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/WebAssembly.cpp
+++ cfe/trunk/lib/Driver/ToolChains/WebAssembly.cpp
@@ -20,6 +20,27 @@
 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 @@
   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")

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin added a comment.

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Thomas Lively via Phabricator via cfe-commits]

tlively added inline comments.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:29
+  Pthread =
+  DriverArgs.hasFlag(options::OPT_pthread, options::OPT_no_pthread, false);
+  ThreadModel =

aheejin wrote:
> tlively wrote:
> > Shouldn't every use of `hasFlag` be `getLastArgValue` instead?
> `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`.
I see! I had misunderstood hasFlag. Thanks for explaining it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin marked 4 inline comments as done.
aheejin added inline comments.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:29
+  Pthread =
+  DriverArgs.hasFlag(options::OPT_pthread, options::OPT_no_pthread, false);
+  ThreadModel =

tlively wrote:
> Shouldn't every use of `hasFlag` be `getLastArgValue` instead?
`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`.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:36
+  // Did user explicitly specify -mthread-model / -pthread?
+  bool HasThreadModel = DriverArgs.hasArg(options::OPT_mthread_model);
+  bool HasPthread = Pthread && DriverArgs.hasArg(options::OPT_pthread);

tlively wrote:
> It doesn't matter whether the user included the -pthread flag if they later 
> included the -no-pthread flag.
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin updated this revision to Diff 186093.
aheejin added a comment.

- Address comments


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874

Files:
  include/clang/Driver/Options.td
  include/clang/Driver/ToolChain.h
  lib/Driver/Driver.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/WebAssembly.cpp
  lib/Driver/ToolChains/WebAssembly.h
  test/Driver/wasm-toolchain.c
  test/Preprocessor/wasm-target-features.c

Index: test/Preprocessor/wasm-target-features.c
===
--- test/Preprocessor/wasm-target-features.c
+++ 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{{$}}
Index: test/Driver/wasm-toolchain.c
===
--- test/Driver/wasm-toolchain.c
+++ 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'
Index: lib/Driver/ToolChains/WebAssembly.h
===
--- lib/Driver/ToolChains/WebAssembly.h
+++ lib/Driver/ToolChains/WebAssembly.h
@@ -64,7 +64,7 @@
   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"; }
 
Index: lib/Driver/ToolChains/WebAssembly.cpp
===
--- lib/Driver/ToolChains/WebAssembly.cpp
+++ lib/Driver/ToolChains/WebAssembly.cpp
@@ -20,6 +20,27 @@
 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 @@
   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
+  // tar

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Thomas Lively via Phabricator via cfe-commits]

tlively added inline comments.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:29
+  Pthread =
+  DriverArgs.hasFlag(options::OPT_pthread, options::OPT_no_pthread, false);
+  ThreadModel =

Shouldn't every use of `hasFlag` be `getLastArgValue` instead?



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:36
+  // Did user explicitly specify -mthread-model / -pthread?
+  bool HasThreadModel = DriverArgs.hasArg(options::OPT_mthread_model);
+  bool HasPthread = Pthread && DriverArgs.hasArg(options::OPT_pthread);

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



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:42
+  // "posix")
+  if (HasPthread && HasThreadModel && ThreadModel != "posix")
+Driver.Diag(diag::err_drv_argument_not_allowed_with)

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.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:157
+  parseThreadArgs(getDriver(), DriverArgs, Pthread, ThreadModel);
+  if (Pthread || ThreadModel == "posix") {
+CC1Args.push_back("-target-feature");

Same here. Should compare ThreadModel with "single".


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin added a comment.

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.




Comment at: lib/Driver/ToolChains/WebAssembly.cpp:50
+  bool HasNoPthread =
+  !Pthread && DriverArgs.hasArg(clang::driver::options::OPT_no_pthread);
+

sbc100 wrote:
> tlively wrote:
> > Should this logic use `getLastArg` or perhaps `getLastArgNoClaim` to check 
> > only that the final requested configuration is consistent rather than 
> > checking all intermediate configurations?
> Can you remove all the "clang::driver" namspace qualification here since 
> there is  a "using" above?
@sbc100 Yeah good catch.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:50
+  bool HasNoPthread =
+  !Pthread && DriverArgs.hasArg(clang::driver::options::OPT_no_pthread);
+

aheejin wrote:
> sbc100 wrote:
> > tlively wrote:
> > > Should this logic use `getLastArg` or perhaps `getLastArgNoClaim` to 
> > > check only that the final requested configuration is consistent rather 
> > > than checking all intermediate configurations?
> > Can you remove all the "clang::driver" namspace qualification here since 
> > there is  a "using" above?
> @sbc100 Yeah good catch.
@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.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:62
+Driver.Diag(diag::err_drv_argument_not_allowed_with) << "-matomics"
+ << "-no-pthread";
+  // '-mno-atomics' cannot be used with '-mthread-model posix'

tlively wrote:
> I'm not sure about this one, either. What if I want atomics for 
> multithreading but I don't want to link with libpthread?
Yeah good catch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin updated this revision to Diff 186090.
aheejin added a comment.

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874

Files:
  include/clang/Driver/Options.td
  include/clang/Driver/ToolChain.h
  lib/Driver/Driver.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/WebAssembly.cpp
  lib/Driver/ToolChains/WebAssembly.h
  test/Driver/wasm-toolchain.c
  test/Preprocessor/wasm-target-features.c

Index: test/Preprocessor/wasm-target-features.c
===
--- test/Preprocessor/wasm-target-features.c
+++ 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{{$}}
Index: test/Driver/wasm-toolchain.c
===
--- test/Driver/wasm-toolchain.c
+++ 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'
Index: lib/Driver/ToolChains/WebAssembly.h
===
--- lib/Driver/ToolChains/WebAssembly.h
+++ lib/Driver/ToolChains/WebAssembly.h
@@ -64,7 +64,7 @@
   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"; }
 
Index: lib/Driver/ToolChains/WebAssembly.cpp
===
--- lib/Driver/ToolChains/WebAssembly.cpp
+++ lib/Driver/ToolChains/WebAssembly.cpp
@@ -20,6 +20,30 @@
 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);
+  std::string ThreadModelOpt =
+  std::string("-mthread-model ") + ThreadModel.data();
+  // '-pthread' cannot be used with '-mthread-model single' (or anything not
+  // "posix")
+  if (HasPthread && HasThreadModel && ThreadModel != "posix")
+Driver.Diag(diag::err_drv_argument_not_allowed_with)
+<< "-pthread" << ThreadModelOpt;
+}
+
 wasm::Linker::Linker(const ToolChain &TC)
 : GnuTool("wasm::Linker", "lld", TC) {}
 
@@ -123,6 +147,17 @@
   if (DriverArgs.hasFlag(clang::driver::options::OPT_fuse_init_array,
  options::OPT_fno_use_init_array, true))
 CC1Args.push_back("-fuse-init-array");
+

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin updated this revision to Diff 186088.
aheejin marked 8 inline comments as done.
aheejin added a comment.

I had an offline discussion with @tlively and @dschuff and decided to remove 
`-atomics` option in the driver. Instead, `clang -cc1`'s `-target-feature 
+atomics` will be 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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874

Files:
  include/clang/Driver/Options.td
  include/clang/Driver/ToolChain.h
  lib/Driver/Driver.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/WebAssembly.cpp
  lib/Driver/ToolChains/WebAssembly.h
  test/Driver/wasm-toolchain.c

Index: test/Driver/wasm-toolchain.c
===
--- test/Driver/wasm-toolchain.c
+++ 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'
Index: lib/Driver/ToolChains/WebAssembly.h
===
--- lib/Driver/ToolChains/WebAssembly.h
+++ lib/Driver/ToolChains/WebAssembly.h
@@ -64,7 +64,7 @@
   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"; }
 
Index: lib/Driver/ToolChains/WebAssembly.cpp
===
--- lib/Driver/ToolChains/WebAssembly.cpp
+++ lib/Driver/ToolChains/WebAssembly.cpp
@@ -20,6 +20,30 @@
 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);
+  std::string ThreadModelOpt =
+  std::string("-mthread-model ") + ThreadModel.data();
+  // '-pthread' cannot be used with '-mthread-model single' (or anything not
+  // "posix")
+  if (HasPthread && HasThreadModel && ThreadModel != "posix")
+Driver.Diag(diag::err_drv_argument_not_allowed_with)
+<< "-pthread" << ThreadModelOpt;
+}
+
 wasm::Linker::Linker(const ToolChain &TC)
 : GnuTool("wasm::Linker", "lld", TC) {}
 
@@ -123,6 +147,17 @@
   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 == "posix") {
+CC1Args.push_back("-target-feature");
+CC1Args.push_back("+atomics");
+  }
 }
 
 ToolChain::RuntimeLibType WebAssembly::GetDefaultRuntimeLibType()

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Thomas Lively via Phabricator via cfe-commits]

tlively added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin added a comment.

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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added inline comments.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:50
+  bool HasNoPthread =
+  !Pthread && DriverArgs.hasArg(clang::driver::options::OPT_no_pthread);
+

tlively wrote:
> Should this logic use `getLastArg` or perhaps `getLastArgNoClaim` to check 
> only that the final requested configuration is consistent rather than 
> checking all intermediate configurations?
Can you remove all the "clang::driver" namspace qualification here since there 
is  a "using" above?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added inline comments.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:66
+if (Args.hasFlag(clang::driver::options::OPT_pthread,
+ clang::driver::options::OPT_no_pthread),
+false)

aheejin wrote:
> tlively wrote:
> > sbc100 wrote:
> > > aheejin wrote:
> > > > This code is not strictly related, but `hasFlag` is better than 
> > > > `hasArg` when there are both positive and negative versions of an 
> > > > option exist.
> > > 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 :)
> > 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.
> Most true/false or enable/disable options exist in pairs. `-no-pthread` is 
> defined [[ 
> https://github.com/llvm/llvm-project/blob/91970564191bfc40ea9f2c8d32cc1fb6c314515c/clang/include/clang/Driver/Options.td#L2508
>  | 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.
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 :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Thomas Lively via Phabricator via cfe-commits]

tlively added inline comments.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:50
+  bool HasNoPthread =
+  !Pthread && DriverArgs.hasArg(clang::driver::options::OPT_no_pthread);
+

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



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:62
+Driver.Diag(diag::err_drv_argument_not_allowed_with) << "-matomics"
+ << "-no-pthread";
+  // '-mno-atomics' cannot be used with '-mthread-model posix'

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



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:79
+Driver.Diag(diag::err_drv_argument_not_allowed_with)
+<< "-no-pthread" << ThreadModelOpt;
+}

Same question here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin added a comment.

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin updated this revision to Diff 186068.
aheejin added a comment.

- Initialized ThreadModel vairables with ""


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874

Files:
  include/clang/Driver/ToolChain.h
  lib/Driver/Driver.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/WebAssembly.cpp
  lib/Driver/ToolChains/WebAssembly.h
  test/Driver/wasm-toolchain.c

Index: test/Driver/wasm-toolchain.c
===
--- test/Driver/wasm-toolchain.c
+++ test/Driver/wasm-toolchain.c
@@ -38,3 +38,35 @@
 
 // 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.
+// - '-matomics' sets '-mthread-model posix'
+// - '-mthread-model' sets '-matomics'
+// - '-phread' sets both '-matomics' and '-mthread-model posix'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -matomics 2>&1 | FileCheck -check-prefix=ATOMICS %s
+// ATOMICS: clang{{.*}}" "-cc1" {{.*}} "-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 -matomics -mthread-model single 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR0 %s
+// THREAD_OPT_ERROR0: invalid argument '-matomics' not allowed with '-mthread-model single'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -matomics -no-pthread 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR1 %s
+// THREAD_OPT_ERROR1: invalid argument '-matomics' not allowed with '-no-pthread'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -mno-atomics -mthread-model posix 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR2 %s
+// THREAD_OPT_ERROR2: invalid argument '-mno-atomics' not allowed with '-mthread-model posix'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -mno-atomics -pthread 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR3 %s
+// THREAD_OPT_ERROR3: invalid argument '-mno-atomics' not allowed with '-pthread'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -pthread -mthread-model single 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR4 %s
+// THREAD_OPT_ERROR4: invalid argument '-pthread' not allowed with '-mthread-model single'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -no-pthread -mthread-model posix 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR5 %s
+// THREAD_OPT_ERROR5: invalid argument '-no-pthread' not allowed with '-mthread-model posix'
Index: lib/Driver/ToolChains/WebAssembly.h
===
--- lib/Driver/ToolChains/WebAssembly.h
+++ lib/Driver/ToolChains/WebAssembly.h
@@ -64,7 +64,7 @@
   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"; }
 
Index: lib/Driver/ToolChains/WebAssembly.cpp
===
--- lib/Driver/ToolChains/WebAssembly.cpp
+++ lib/Driver/ToolChains/WebAssembly.cpp
@@ -20,6 +20,65 @@
 using namespace clang;
 using namespace llvm::opt;
 
+void parseThreadArgs(const Driver &Driver, const ArgList &DriverArgs,
+ bool &Atomics, bool &Pthread, StringRef &ThreadModel,
+ bool CheckForErrors = true) {
+  // Default value for -matomics / -pthread / -mthread-model options, each being
+  // false / false / "single".
+  Atomics = DriverArgs.hasFlag(clang::driver::options::OPT_matomics,
+   clang::driver::options::OPT_mno_atomics, false);
+  Pthread = DriverArgs.hasFlag(clang::driver::options::OPT_pthread,
+   clang::driver::options::OPT_no_pthread, false);
+  ThreadModel = DriverArgs.getLastArgValue(
+  clang::driver::options::OPT_mthread_model, "single");
+  if (!CheckForErrors)
+re

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin added a comment.

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin updated this revision to Diff 186065.
aheejin added a comment.

- Small fix


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874

Files:
  include/clang/Driver/ToolChain.h
  lib/Driver/Driver.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/WebAssembly.cpp
  lib/Driver/ToolChains/WebAssembly.h
  test/Driver/wasm-toolchain.c

Index: test/Driver/wasm-toolchain.c
===
--- test/Driver/wasm-toolchain.c
+++ test/Driver/wasm-toolchain.c
@@ -38,3 +38,35 @@
 
 // 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.
+// - '-matomics' sets '-mthread-model posix'
+// - '-mthread-model' sets '-matomics'
+// - '-phread' sets both '-matomics' and '-mthread-model posix'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -matomics 2>&1 | FileCheck -check-prefix=ATOMICS %s
+// ATOMICS: clang{{.*}}" "-cc1" {{.*}} "-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 -matomics -mthread-model single 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR0 %s
+// THREAD_OPT_ERROR0: invalid argument '-matomics' not allowed with '-mthread-model single'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -matomics -no-pthread 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR1 %s
+// THREAD_OPT_ERROR1: invalid argument '-matomics' not allowed with '-no-pthread'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -mno-atomics -mthread-model posix 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR2 %s
+// THREAD_OPT_ERROR2: invalid argument '-mno-atomics' not allowed with '-mthread-model posix'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -mno-atomics -pthread 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR3 %s
+// THREAD_OPT_ERROR3: invalid argument '-mno-atomics' not allowed with '-pthread'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -pthread -mthread-model single 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR4 %s
+// THREAD_OPT_ERROR4: invalid argument '-pthread' not allowed with '-mthread-model single'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -no-pthread -mthread-model posix 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR5 %s
+// THREAD_OPT_ERROR5: invalid argument '-no-pthread' not allowed with '-mthread-model posix'
Index: lib/Driver/ToolChains/WebAssembly.h
===
--- lib/Driver/ToolChains/WebAssembly.h
+++ lib/Driver/ToolChains/WebAssembly.h
@@ -64,7 +64,7 @@
   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"; }
 
Index: lib/Driver/ToolChains/WebAssembly.cpp
===
--- lib/Driver/ToolChains/WebAssembly.cpp
+++ lib/Driver/ToolChains/WebAssembly.cpp
@@ -20,6 +20,65 @@
 using namespace clang;
 using namespace llvm::opt;
 
+void parseThreadArgs(const Driver &Driver, const ArgList &DriverArgs,
+ bool &Atomics, bool &Pthread, StringRef &ThreadModel,
+ bool CheckForErrors = true) {
+  // Default value for -matomics / -pthread / -mthread-model options, each being
+  // false / false / "single".
+  Atomics = DriverArgs.hasFlag(clang::driver::options::OPT_matomics,
+   clang::driver::options::OPT_mno_atomics, false);
+  Pthread = DriverArgs.hasFlag(clang::driver::options::OPT_pthread,
+   clang::driver::options::OPT_no_pthread, false);
+  ThreadModel = DriverArgs.getLastArgValue(
+  clang::driver::options::OPT_mthread_model, "single");
+  if (!CheckForErrors)
+return;
+
+  // Error checking
+
+

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin marked an inline comment as done.
aheejin added inline comments.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:66
+if (Args.hasFlag(clang::driver::options::OPT_pthread,
+ clang::driver::options::OPT_no_pthread),
+false)

tlively wrote:
> sbc100 wrote:
> > aheejin wrote:
> > > This code is not strictly related, but `hasFlag` is better than `hasArg` 
> > > when there are both positive and negative versions of an option exist.
> > 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 :)
> 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.
Most true/false or enable/disable options exist in pairs. `-no-pthread` is 
defined [[ 
https://github.com/llvm/llvm-project/blob/91970564191bfc40ea9f2c8d32cc1fb6c314515c/clang/include/clang/Driver/Options.td#L2508
 | 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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Heejin Ahn via Phabricator via cfe-commits]

aheejin updated this revision to Diff 186063.
aheejin added a comment.

- 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


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874

Files:
  include/clang/Driver/ToolChain.h
  lib/Driver/Driver.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/WebAssembly.cpp
  lib/Driver/ToolChains/WebAssembly.h
  test/Driver/wasm-toolchain.c

Index: test/Driver/wasm-toolchain.c
===
--- test/Driver/wasm-toolchain.c
+++ test/Driver/wasm-toolchain.c
@@ -38,3 +38,35 @@
 
 // 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.
+// - '-matomics' sets '-mthread-model posix'
+// - '-mthread-model' sets '-matomics'
+// - '-phread' sets both '-matomics' and '-mthread-model posix'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -matomics 2>&1 | FileCheck -check-prefix=ATOMICS %s
+// ATOMICS: clang{{.*}}" "-cc1" {{.*}} "-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 -matomics -mthread-model single 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR0 %s
+// THREAD_OPT_ERROR0: invalid argument '-matomics' not allowed with '-mthread-model single'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -matomics -no-pthread 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR1 %s
+// THREAD_OPT_ERROR1: invalid argument '-matomics' not allowed with '-no-pthread'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -mno-atomics -mthread-model posix 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR2 %s
+// THREAD_OPT_ERROR2: invalid argument '-mno-atomics' not allowed with '-mthread-model posix'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -mno-atomics -pthread 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR3 %s
+// THREAD_OPT_ERROR3: invalid argument '-mno-atomics' not allowed with '-pthread'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -pthread -mthread-model single 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR4 %s
+// THREAD_OPT_ERROR4: invalid argument '-pthread' not allowed with '-mthread-model single'
+
+// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s -no-pthread -mthread-model posix 2>&1 | FileCheck -check-prefix=THREAD_OPT_ERROR5 %s
+// THREAD_OPT_ERROR5: invalid argument '-no-pthread' not allowed with '-mthread-model posix'
Index: lib/Driver/ToolChains/WebAssembly.h
===
--- lib/Driver/ToolChains/WebAssembly.h
+++ lib/Driver/ToolChains/WebAssembly.h
@@ -64,7 +64,7 @@
   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"; }
 
Index: lib/Driver/ToolChains/WebAssembly.cpp
===
--- lib/Driver/ToolChains/WebAssembly.cpp
+++ lib/Driver/ToolChains/WebAssembly.cpp
@@ -20,6 +20,65 @@
 using namespace clang;
 using namespace llvm::opt;
 
+void parseThreadArgs(const Driver &Driver, const ArgList &DriverArgs,
+ bool &Atomics, bool &Pthread, StringRef &ThreadModel,
+ bool CheckForErrors = true) {
+  // Default value for -matomics / -pthread / -mthread-model options, each being
+  // false / false / "single".
+  Atomics = DriverArgs.hasFlag(clang::driver::options::OPT_matomics,
+   clang::driver::options::OPT_mno_atomics, false);
+  Pthread = DriverArgs.hasFlag(clang::driver::options::OPT_pthread,
+   clang::driver::options::OPT_no_pthread, false);
+  ThreadModel = DriverArgs.

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Thomas Lively via Phabricator via cfe-commits]

tlively added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Thomas Lively via Phabricator via cfe-commits]

tlively added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Thomas Lively via Phabricator via cfe-commits]

tlively added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Thomas Lively via Phabricator via cfe-commits]

tlively added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Thomas Lively via Phabricator via cfe-commits]

tlively added inline comments.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:66
+if (Args.hasFlag(clang::driver::options::OPT_pthread,
+ clang::driver::options::OPT_no_pthread),
+false)

sbc100 wrote:
> aheejin wrote:
> > This code is not strictly related, but `hasFlag` is better than `hasArg` 
> > when there are both positive and negative versions of an option exist.
> 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 :)
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added inline comments.



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:66
+if (Args.hasFlag(clang::driver::options::OPT_pthread,
+ clang::driver::options::OPT_no_pthread),
+false)

aheejin wrote:
> This code is not strictly related, but `hasFlag` is better than `hasArg` when 
> there are both positive and negative versions of an option exist.
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 :)



Comment at: lib/Driver/ToolChains/WebAssembly.cpp:201
+  if (HasAtomics || HasPthread)
+return "posix";
   return "single";

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

[Dan Gohman via Phabricator via cfe-commits]

sunfish added a comment.

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D57874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits