Page MenuHomePhabricator

[WebAssembly] Make thread-related options consistent
ClosedPublic

Authored by aheejin on Feb 6 2019, 7:35 PM.

Details

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.

Diff Detail

Event Timeline

aheejin created this revision.Feb 6 2019, 7:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 7:35 PM
aheejin updated this revision to Diff 185700.Feb 6 2019, 7:36 PM
  • Small fix
sbc100 accepted this revision.Feb 6 2019, 8:10 PM
This revision is now accepted and ready to land.Feb 6 2019, 8:10 PM
dschuff added a comment.EditedFeb 7 2019, 10:44 AM

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

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.

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

That sounds reasonable to me too.

aheejin updated this revision to Diff 185895.Feb 7 2019, 4:54 PM

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 marked an inline comment as done.Feb 7 2019, 4:56 PM
aheejin added inline comments.
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.

aheejin retitled this revision from [WebAssembly] Set '-matomics' when '-pthread' is set to [WebAssembly] Make thread-related options consistent.Feb 7 2019, 4:59 PM
aheejin edited the summary of this revision. (Show Details)
  • -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 added inline comments.Feb 7 2019, 5:17 PM
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 accepted this revision.Feb 7 2019, 5:21 PM
sbc100 added a comment.Feb 7 2019, 5:31 PM
  • -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 added a subscriber: tlively.Feb 8 2019, 1:12 PM
tlively added inline comments.
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.

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

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

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.

  • -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 updated this revision to Diff 186063.Feb 8 2019, 3:58 PM
  • 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 edited the summary of this revision. (Show Details)
aheejin updated this revision to Diff 186065.Feb 8 2019, 4:02 PM
  • Small fix
aheejin marked an inline comment as done.Feb 8 2019, 4:04 PM
aheejin added inline comments.
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 added a comment.EditedFeb 8 2019, 4:06 PM
  • -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 updated this revision to Diff 186068.Feb 8 2019, 4:10 PM
  • Initialized ThreadModel vairables with ""

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

tlively added inline comments.Feb 8 2019, 4:11 PM
lib/Driver/ToolChains/WebAssembly.cpp
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?

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.

sbc100 added inline comments.Feb 8 2019, 4:13 PM
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 added inline comments.Feb 8 2019, 4:15 PM
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 added a comment.EditedFeb 8 2019, 4:20 PM

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?

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 updated this revision to Diff 186088.EditedFeb 8 2019, 5:53 PM
aheejin marked 8 inline comments as done.

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 updated this revision to Diff 186090.Feb 8 2019, 6:02 PM
  • Replace -matomics with -mthread-model posix in preprocessor test
  • -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
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.

62 ↗(On Diff #186065)

Yeah good catch.

tlively added inline comments.Feb 8 2019, 6:04 PM
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 marked 4 inline comments as done.Feb 8 2019, 6:22 PM
aheejin added inline comments.
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 updated this revision to Diff 186093.Feb 8 2019, 6:22 PM
  • Address comments
aheejin edited the summary of this revision. (Show Details)Feb 8 2019, 6:29 PM
tlively added inline comments.Feb 8 2019, 8:21 PM
lib/Driver/ToolChains/WebAssembly.cpp
29 ↗(On Diff #186088)

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

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

sbc100 accepted this revision.Feb 11 2019, 1:41 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 2:47 PM