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
aganea added inline comments.
lld/COFF/Driver.cpp
1147–1157

This is not consistent with the ELF part. The cmd-line flags grammar and the handling should the be similar.

lld/Common/Threads.cpp
0

Make this ThreadPoolStrategy parallel::Strategy; and move to llvm/lib/Support/Parallel.[h|cpp], and use in Executor::getDefaultExecutor() when constructing the ThreadPoolExecutor.

Otherwise --threads=N will have no effect on the functions used by lld/Common/Threads.h.

lld/ELF/Driver.cpp
1040

What about doing this instead:

config->thinLTOJobs = args.getLastArgValue(OPT_thinlto_jobs);

if (auto *arg = args.getLastArg(OPT_threads)) {
  auto S = get_threadpool_strategy(arg->getValue());
  if (!S)
    error("--threads: invalid thread count: " + arg->getValue());
  parallel::Strategy = *S;

  // maybe warn if --thinlto-jobs= is provided along with --threads=
  config->thinLTOJobs = arg->getValue();
}

This makes for consistent behavior for any of the thread-count flags: --threads, /opt:lldltojobs=, -flto-jobs=, --thinlto-jobs=, -thinlto-threads=, --plugin-opt=jobs=.

And that way, we could uniformly extend the cmd-line flags grammar, if need be (for example if we want platform-independent takset/NUMA control). See @ikitayama 's issue: http://lists.llvm.org/pipermail/llvm-dev/2020-March/140432.html

lld/ELF/SyntheticSections.cpp
2753–2754

I realized the algorithm below didn't make sense if concurrency is not a power-of-two (including 2^0 when multi-threading is disabled/off).

Could you please rebase on rG42dc667db248e27a44dc245d5c39ce1f8ad26a85 -- reverted a mistake I did.

3195–3196

if (parallel::Strategy.ThreadsRequested != 1)

lld/include/lld/Common/Threads.h
66

Remove (see first comment).

68

if (parallel::Strategy.ThreadsRequested != 1)

75

Ditto.

lld/wasm/Driver.cpp
380

Same comment as for the ELF part.

MaskRay updated this revision to Diff 253500.Mar 29 2020, 11:40 PM
MaskRay marked 12 inline comments as done.

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.