Page MenuHomePhabricator

[lld][COFF][ELF][WebAssembly] Replace --[no-]threads /threads[:no] with --threads={1,2,...} /threads:{1,2,...}
ClosedPublic

Authored by MaskRay on Mar 26 2020, 1:08 PM.

Details

Summary

--no-threads is a name copied from gold.
gold has --no-thread, --thread-count and several other --thread-count-*.

There are needs to customize the number of threads (running several lld
processes concurrently or customizing the number of LTO threads).
Having a single --threads=N is a straightforward replacement of gold's
--no-threads + --thread-count.

--no-threads is used rarely. So just delete --no-threads instead of
keeping it for compatibility for a while.

If --threads= is specified (ELF,wasm; COFF /threads: is similar),
--thinlto-jobs= defaults to --threads=,
otherwise all available hardware threads are used.

There is currently no way to override a --threads={1,2,...}. It is still
a debate whether we should use --threads=all.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
MaskRay marked 12 inline comments as done.Mar 29 2020, 11:40 PM

Address review comments

MaskRay retitled this revision from [lld][ELF][WebAssembly] Replace --(no-)threads with --threads={all,1,2,...} to [lld][COFF][ELF][WebAssembly] Replace --[no-]threads /threads[:no] with --threads={all,1,2,...} /threads:{all,1,2,...}.Mar 29 2020, 11:42 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay added inline comments.
lld/Common/Threads.cpp
0

I only intended to change the option name, but now it is clear that doing that will cause a future rename churn (enabledThreads -> llvm::parallel::strategy). Let's just finish it with one patch.

lld/ELF/Driver.cpp
1040

get_threadpool_strategy interprets 0 differently from lld (your previous suggestion is that we should error), and "" differently (lld probably should reject an empty string).

If this takes time to get a consensus, we can make this change separately.

lld/ELF/SyntheticSections.cpp
2753–2754

This explained an internal failure I saw. I "fixed" that with 34bdddf9a13cfdbbb5506dc89cf8e781be53105f.

Though I have touched this piece of code several times. I forgot there was a bug after your previous change..

3195–3196

I'll simplify it further.

MaskRay updated this revision to Diff 253501.Mar 29 2020, 11:46 PM

Some adjustment

MaskRay updated this revision to Diff 253502.Mar 29 2020, 11:48 PM

Fix ELF/SyntheticSections.cpp

grimar added inline comments.Mar 30 2020, 2:04 AM
lld/test/ELF/threads.test
1 ↗(On Diff #253502)

llvm-mc /dev/null -o %t.o -filetype=obj -triple=xxx?

(the same for the wasm test)

12 ↗(On Diff #253502)

Perhaps use FileCheck -DVAL=xxx to make checks explicit?

aganea added inline comments.Mar 30 2020, 7:06 AM
lld/ELF/Driver.cpp
1040

In that case, I would modify get_threadpool_strategy. This function was only added recently, and I'm not fond of =0, we can error out in that case in get_threadpool_strategy, to match what was discussed so far. I would prefer matching behavior for all thread flags across LLVM. But yes, we can do after if you prefer.

llvm/lib/Support/Parallel.cpp
23

llvm::parallel::Strategy. LLVM uses first letter uppercase for variables, whereas LLD uses lowercase.

MaskRay updated this revision to Diff 253614.Mar 30 2020, 9:19 AM
MaskRay marked 4 inline comments as done.

Address comments

lld/ELF/Driver.cpp
1040

If we delete "" and "0" cases from get_threadpool_strategy, then there will be no way to return the default value. This probably means get_threadpool_strategy does not meed lld's needs without an API change. We can defer the change.

llvm/lib/Support/Parallel.cpp
23

llvm::parallel::{seq,par} are lower cased variables. I think this should not matter.

aganea added inline comments.Mar 30 2020, 9:52 AM
llvm/lib/Support/Parallel.cpp
142

Do you think you can move this inside the ThreadPoolExecutor class, like ThreadPoolExecutor::Deleter?

MaskRay updated this revision to Diff 253638.Mar 30 2020, 10:48 AM

Move a function local Creator to ThreadPoolExecutor::Creator

MaskRay marked an inline comment as done.Mar 30 2020, 10:48 AM

Thanks for the quick changes! Just a few more things and I think it should be good to go.

lld/COFF/Driver.cpp
1156

nit: use parallel::strategy = hardware_concurrency(threads); to ensure any future change to the default strategy does not change this behavior (same comment for other places below).

1157

Overwrite config->thinLTOJobs here, like the ELF part.

lld/COFF/Options.td
223
"Number of threads. 'all' (default) means all of concurrent threads supported. '1' disables multi-threading."

Same comment for the other drivers.

lld/ELF/Driver.cpp
1047

In absolute terms, do we really need two options for setting multi-threading? (-threads= for general-purpose code, and -thinlto-jobs= for ThinLTO threads)
We can address this later, but it might complicate the setup for users. @tejohnson

llvm/lib/Support/Parallel.cpp
23

Please add a comment above signifying that this defaults to using all hardware threads.

MaskRay updated this revision to Diff 253698.Mar 30 2020, 2:03 PM
MaskRay marked 7 inline comments as done.

Address review comments

lld/ELF/Driver.cpp
1047

I had the same question:) I think even if people start to migrate to --threads, -thinlto-jobs= may be useful for debugging ThinLTO issues. --thinlto-jobs=1 can be used together with --save-temps, without slowding down other parallel loops. Whether this is worth a separate option needs more thoughts.

MaskRay edited the summary of this revision. (Show Details)Mar 30 2020, 2:09 PM
aganea accepted this revision.Mar 30 2020, 2:42 PM

LGTM, thanks! Maybe wait a bit in case there are other comments.

Some todos for the record:

  • This line: https://github.com/llvm/llvm-project/blob/master/lld/lib/ReaderWriter/MachO/LayoutPass.cpp#L464 - should use the parallelSort() API, but could be done later.
  • parallel::strategy.ThreadsRequested == 1 is a bit verbose and error prone, it could be an API like parallel::strategy.useThreads() - this could be done in a subsequent commit.
  • Is there value for the code in lld/Common/Threads.h? It could be replaced with direct calls to Parallel.h APIs instead? (assuming they handle the "no multi-threading" case)
  • Same function for handling --threads= in all three drivers and get_threadpool_strategy().
This revision is now accepted and ready to land.Mar 30 2020, 2:42 PM
rnk accepted this revision.Mar 30 2020, 3:41 PM

lgtm

Thanks @aganea for reviewing. I didn't review thoroughly, but I approve of the direction.

MaskRay updated this revision to Diff 253732.Mar 30 2020, 3:58 PM

Thanks @aganea for reviewing and a list of TODO items in https://reviews.llvm.org/D76885#1950989 , and @rnk.

Add .Cm 1 disables multi-threading to docs/ld.lld.1
Fix a comment --thinlto-jobs= -> /opt:lldltojobs= in COFF/Driver.cpp

Will wait a bit and see if there are other comments.

ruiu added a comment.Mar 30 2020, 9:06 PM

Do we need all? It's the default behavior, so if you want to specify --threads=all, you can just delete it altogether.

Are you going to make the same change for the other ports? I think wasm-ld mimics GNU linker's options, and I don't think we want to deviate too much from it, even if it simplifies the command line options.

MaskRay added a comment.EditedMar 30 2020, 10:35 PM

Do we need all? It's the default behavior, so if you want to specify --threads=all, you can just delete it altogether.

--threads=all is a way to override a previous --threads=3. We just need a way to reset the behavior to a previous state. It is just for completeness.

Are you going to make the same change for the other ports? I think wasm-ld mimics GNU linker's options, and I don't think we want to deviate too much from it, even if it simplifies the command line options.

--threads and --no-threads are gold-only options. They have currently mostly debug options. GNU ld does not have it. gold actually has more options but they are probably overkill:

--thread-count COUNT
       Number of threads to use

--thread-count-initial COUNT
       Number of threads to use in initial pass

--thread-count-middle COUNT Number of threads to use in middle pass

--thread-count-final COUNT
       Number of threads to use in final pass

For wasm-ld, @sbc100 mentioned that he can change his habit to adapt the new --threads=

ruiu added a comment.Mar 30 2020, 11:07 PM

Instead of --threads=all, can you use --threads? --threads has several advantages over --threads=all:

  • it is consistent with other command such as make, which takes -j for "all" and -jN for N threads
  • it is compatible with gold
  • "all" isn't technically very accurate. A more accurate option name would be something like --threads=same-as-cpu-threads, but that's fringe. By eliminating an argument, we can avoid naming it.

If we are doing this, I'd like to see this to be added to the ELF version.

MaskRay added a comment.EditedMar 31 2020, 8:20 AM

Instead of --threads=all, can you use --threads? --threads has several advantages over --threads=all:

  • it is consistent with other command such as make, which takes -j for "all" and -jN for N threads
  • it is compatible with gold
  • "all" isn't technically very accurate. A more accurate option name would be something like --threads=same-as-cpu-threads, but that's fringe. By eliminating an argument, we can avoid naming it.

If we are doing this, I'd like to see this to be added to the ELF version.

Nobody uses --threads. I will delete --threads=all. Let's wait for other opinion whether we need an options restoring the default behavior.

One argument for --threads=all is that --thinlto-jobs=all is supported, but this point will be moot if we are going to revisit it.

MaskRay updated this revision to Diff 253898.Mar 31 2020, 8:29 AM
MaskRay retitled this revision from [lld][COFF][ELF][WebAssembly] Replace --[no-]threads /threads[:no] with --threads={all,1,2,...} /threads:{all,1,2,...} to [lld][COFF][ELF][WebAssembly] Replace --[no-]threads /threads[:no] with --threads={1,2,...} /threads:{1,2,...}.

Delete --threads=all

MaskRay updated this revision to Diff 253900.Mar 31 2020, 8:45 AM
MaskRay edited the summary of this revision. (Show Details)

Update description

This revision was automatically updated to reflect the committed changes.

Instead of --threads=all, can you use --threads? --threads has several advantages over --threads=all:

  • it is consistent with other command such as make, which takes -j for "all" and -jN for N threads

I don't disagree with your arguments but from the man page it looks like make -j without any args means there is no upper limit at all:

j [jobs], --jobs[=jobs]
            Specifies  the  number of jobs (commands) to run simultaneously.  If there is more than one -j option, the last one
            is effective.  If the -j option is given without an argument, make will not limit the number of jobs that  can  run
            simultaneously.

Unless I'm misreading. I guess that is why we very rarely see it in the wild.

  • it is compatible with gold
  • "all" isn't technically very accurate. A more accurate option name would be something like --threads=same-as-cpu-threads, but that's fringe. By eliminating an argument, we can avoid naming it.

If we are doing this, I'd like to see this to be added to the ELF version.

Just a quick question to bounce on what Rui said: why not make this flag -j instead of --threads? Everyone is indeed used to -j. Is there an argument for it being --threads?

@aganea make/ninja/parallel spawn processes (jobs). For an ld, its has a single job. --threads= might be a bit implementation specific but I cannot think of a better term expression parallelism within a job...

There is another very weak argument. Because j is a prefix of just-symbols, GNU style getopt_long treats -j as an abbreviation of --just-symbols (which is implemented along with -rpath). If we add -j to lld, it can do different things from GNU ld. This makes a short option -j less favorable...

It looks like this change broke our buildbot: http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/6358

Could you please recommit this with a fix, or revert this?

broadwaylamb added inline comments.Mar 31 2020, 1:23 PM
lld/test/ELF/threads.s
1

This bot builds a cross-toolchain that can only produce binaries for ARM, so this test fails because it only supports x86_64. Maybe you can mark it as unsupported?

MaskRay marked 2 inline comments as done.Mar 31 2020, 3:25 PM
MaskRay added inline comments.
lld/test/ELF/threads.s
1
broadwaylamb added inline comments.Mar 31 2020, 4:09 PM
lld/test/ELF/threads.s
1

The failure is gone, thanks!

This comment was removed by smeenai.

For people who were using -Wl,--no-threads to disable parallelism in (some parts of) their build system, what's the way forward if they want to support both older and newer LLD versions?

(I suppose feature flag detection is one way, but that's not really a thing on some build systems, including Bazel, I believe.)

MaskRay marked an inline comment as done.Apr 1 2020, 12:33 PM

For people who were using -Wl,--no-threads to disable parallelism in (some parts of) their build system, what's the way forward if they want to support both older and newer LLD versions?

(I suppose feature flag detection is one way, but that's not really a thing on some build systems, including Bazel, I believe.)

-Wl,--no-threads should be very rare. https://github.com/halide/Halide/blob/master/src/runtime/hexagon_remote/Makefile and libsodium_1.0.18-1/dist-build/wasm32-wasi.sh are the only open source cases I can find. You probably should make the change while bumping the version of lld.

phosek added a subscriber: phosek.EditedApr 1 2020, 4:08 PM

Instead of --threads=all, can you use --threads? --threads has several advantages over --threads=all:

  • it is consistent with other command such as make, which takes -j for "all" and -jN for N threads
  • it is compatible with gold
  • "all" isn't technically very accurate. A more accurate option name would be something like --threads=same-as-cpu-threads, but that's fringe. By eliminating an argument, we can avoid naming it.

If we are doing this, I'd like to see this to be added to the ELF version.

Nobody uses --threads. I will delete --threads=all. Let's wait for other opinion whether we need an options restoring the default behavior.

I disagree, we've been using --threads for some time (there are 5 different instances of this flag just in the Fuchsia build, see for example https://fuchsia.googlesource.com/fuchsia/+/refs/heads/master/build/config/fuchsia/BUILD.gn#48) and this change broke our build.

One argument for --threads=all is that --thinlto-jobs=all is supported, but this point will be moot if we are going to revisit it.

phosek added a comment.Apr 1 2020, 4:21 PM

I think the removal of --thread could have been handled more gracefully, e.g. print a warning but retain the existing functionality, and later follow up with the removal. Right now our only options are to either disable threading until we roll the new toolchain and slow down the build in the meantime, or disable the canary bots and loose coverage for upstream changes, neither of which is great.

sbc100 added a comment.Apr 1 2020, 4:24 PM

I think the removal of --thread could have been handled more gracefully, e.g. print a warning but retain the existing functionality, and later follow up with the removal. Right now our only options are to either disable threading until we roll the new toolchain and slow down the build in the meantime, or disable the canary bots and loose coverage for upstream changes, neither of which is great.

I tend to agree. I think the assumption was that this was only used by developers to debug (at least that was my only use for it). I think you have proved that assumption wrong.

I think the removal of --thread could have been handled more gracefully, e.g. print a warning but retain the existing functionality, and later follow up with the removal. Right now our only options are to either disable threading until we roll the new toolchain and slow down the build in the meantime, or disable the canary bots and loose coverage for upstream changes, neither of which is great.

--threads was the default so you can just delete it from your BUILD.gn There is no functional difference before or after this change.

dmajor added a subscriber: dmajor.Jul 2 2020, 8:42 AM

This commit breaks the build when -DLLVM_ENABLE_THREADS=OFF is used. Could you please take a look?

Undefined symbols for architecture x86_64:
  "llvm::parallel::strategy", referenced from:
      readConfigs(llvm::opt::InputArgList&) in liblldELF.a(Driver.cpp.o)
      lld::coff::LinkerDriver::link(llvm::ArrayRef<char const*>) in liblldCOFF.a(Driver.cpp.o)
      lld::wasm::(anonymous namespace)::LinkerDriver::link(llvm::ArrayRef<char const*>) in liblldWasm.a(Driver.cpp.o)
      lld::unlinkAsync(llvm::StringRef) in liblldCommon.a(Filesystem.cpp.o)
      lld::elf::MergeNoTailSection::finalizeContents() in liblldELF.a(SyntheticSections.cpp.o)
      createSymbols(llvm::ArrayRef<std::__1::vector<lld::elf::GdbIndexSection::NameAttrEntry, std::__1::allocator<lld::elf::GdbIndexSection::NameAttrEntry> > >, std::__1::vector<lld::elf::GdbIndexSection::GdbChunk, std::__1::allocator<lld::elf::GdbIndexSection::GdbChunk> > const&) in liblldELF.a(SyntheticSections.cpp.o)
      void lld::elf::doIcf<llvm::object::ELFType<(llvm::support::endianness)1, false> >() in liblldELF.a(ICF.cpp.o)
      ...
dmajor added a comment.Jul 8 2020, 9:33 AM

I filed https://bugs.llvm.org/show_bug.cgi?id=46641 to make sure it doesn't get lost.

--no-threads is used rarely. So just delete --no-threads instead of keeping it for compatibility for a while.

I encountered a frustrating issue with seemingly-unrelated symptoms that eventually traced back to this change.

The wasi-sdk project was building libcxx, and its build system had set CMAKE_EXE_LINKER_FLAGS="-Wl,--no-threads" (I don't know why, wasi-sdk is an unfamiliar project to me, I just needed to build it for something else). When cmake was doing its usual "is XYZ supported" tests, the --no-threads option caused the checks to fail, so cmake believed that for example LIBCXX_SUPPORTS_FNO_EXCEPTIONS_FLAG was false. But the cmake was overall still successful. So we got an apparently successful libcxx build but it didn't have the settings that were requested. This manifested as weird issues in a faraway place later. It took a lot of time to track down.

It's not clear to me why the --no-threads made the "is XYZ supported" checks fail but the actual build still succeeded. I'm pretty worried though about introducing this kind of silent behavior change. Do you think other projects may be susceptible to such a problem? Should the lack of --no-threads compatibility be reconsidered for the 11 release branch?

--no-threads is used rarely. So just delete --no-threads instead of keeping it for compatibility for a while.

I encountered a frustrating issue with seemingly-unrelated symptoms that eventually traced back to this change.

The wasi-sdk project was building libcxx, and its build system had set CMAKE_EXE_LINKER_FLAGS="-Wl,--no-threads" (I don't know why, wasi-sdk is an unfamiliar project to me, I just needed to build it for something else). When cmake was doing its usual "is XYZ supported" tests, the --no-threads option caused the checks to fail, so cmake believed that for example LIBCXX_SUPPORTS_FNO_EXCEPTIONS_FLAG was false. But the cmake was overall still successful. So we got an apparently successful libcxx build but it didn't have the settings that were requested. This manifested as weird issues in a faraway place later. It took a lot of time to track down.

It's not clear to me why the --no-threads made the "is XYZ supported" checks fail but the actual build still succeeded. I'm pretty worried though about introducing this kind of silent behavior change. Do you think other projects may be susceptible to such a problem? Should the lack of --no-threads compatibility be reconsidered for the 11 release branch?

Regardless of what we decide to do here.. we should remove that flag from wasi-sdk build system. I have no idea why it was added, perhaps there were some bugs related to threading early on in the development of wasm-ld and this just never got removed.

--no-threads is used rarely. So just delete --no-threads instead of keeping it for compatibility for a while.

I encountered a frustrating issue with seemingly-unrelated symptoms that eventually traced back to this change.

The wasi-sdk project was building libcxx, and its build system had set CMAKE_EXE_LINKER_FLAGS="-Wl,--no-threads" (I don't know why, wasi-sdk is an unfamiliar project to me, I just needed to build it for something else). When cmake was doing its usual "is XYZ supported" tests, the --no-threads option caused the checks to fail, so cmake believed that for example LIBCXX_SUPPORTS_FNO_EXCEPTIONS_FLAG was false. But the cmake was overall still successful. So we got an apparently successful libcxx build but it didn't have the settings that were requested. This manifested as weird issues in a faraway place later. It took a lot of time to track down.

It's not clear to me why the --no-threads made the "is XYZ supported" checks fail but the actual build still succeeded. I'm pretty worried though about introducing this kind of silent behavior change. Do you think other projects may be susceptible to such a problem? Should the lack of --no-threads compatibility be reconsidered for the 11 release branch?

Sam is the code owner of the WebAssembly backend. He makes the decision whether --no-threads needs to be supported.

Personally I recognize that the removal of the option caused some friction but (at least for COFF/ELF) I don't think the use cases are critical enough to be supported.

--no-threads is used rarely. So just delete --no-threads instead of keeping it for compatibility for a while.

I encountered a frustrating issue with seemingly-unrelated symptoms that eventually traced back to this change.

The wasi-sdk project was building libcxx, and its build system had set CMAKE_EXE_LINKER_FLAGS="-Wl,--no-threads" (I don't know why, wasi-sdk is an unfamiliar project to me, I just needed to build it for something else). When cmake was doing its usual "is XYZ supported" tests, the --no-threads option caused the checks to fail, so cmake believed that for example LIBCXX_SUPPORTS_FNO_EXCEPTIONS_FLAG was false. But the cmake was overall still successful. So we got an apparently successful libcxx build but it didn't have the settings that were requested. This manifested as weird issues in a faraway place later. It took a lot of time to track down.

It's not clear to me why the --no-threads made the "is XYZ supported" checks fail but the actual build still succeeded. I'm pretty worried though about introducing this kind of silent behavior change. Do you think other projects may be susceptible to such a problem? Should the lack of --no-threads compatibility be reconsidered for the 11 release branch?

Sam is the code owner of the WebAssembly backend. He makes the decision whether --no-threads needs to be supported.

Personally I recognize that the removal of the option caused some friction but (at least for COFF/ELF) I don't think the use cases are critical enough to be supported.

I'm OK with fixing this downstream in this case I think. There should be very few users of this flag with wasm-ld but also I wouldn't want the flag names for the same feature to be different between different lld backends.

In terms of the way this problem manifested, It makes sense that the cmake feature detection would fail because it wouldn't be able to link any of the test programs it was building. I imagine the overall project still built because there are no executables in this case, only libraries, so there is no link step in the build, only llvm-ar. For most projects this wouldn't be and issue and they would see a clear link failure.

I've now opened https://github.com/WebAssembly/wasi-sdk/pull/151/ to remove the --no-threads from wasi-sdk, which was a workaround for an old bug that has long been fixed.

dmajor added a comment.Aug 6 2020, 9:52 AM

I've now opened https://github.com/WebAssembly/wasi-sdk/pull/151/ to remove the --no-threads from wasi-sdk, which was a workaround for an old bug that has long been fixed.

With that PR now merged, I've confirmed that the latest wasi-sdk tree no longer hits the issue I was seeing. Thanks everyone.