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

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

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

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

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

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

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

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

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

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

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

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?

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

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

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

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:

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

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

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

tlively added a comment. In D57874#1391384 , @aheejin wrote: > In D57874#1391133 , @tlively wrote: > > > In D57874#1389953 , @aheejin wrote: > > > > > Anyway, moved all

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

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`

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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)

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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