[PATCH] D76885: [lld][ELF][WebAssembly] Replace --(no-)threads with --threads={all,1,2,...}

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a subscriber: ikitayama.
aganea added inline comments.


================
Comment at: lld/COFF/Driver.cpp:1147
 
-  lld::threadsEnabled = args.hasFlag(OPT_threads, OPT_threads_no, true);
+  lld::enabledThreads = args.hasFlag(OPT_threads, OPT_threads_no, true) ? 0 : 1;
 
----------------
This is not consistent with the ELF part. The cmd-line flags grammar and the handling should the be similar.


================
Comment at: lld/Common/Threads.cpp:11
 
-bool lld::threadsEnabled = true;
+unsigned lld::enabledThreads = 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`.


================
Comment at: lld/ELF/Driver.cpp:1040
+  if (auto *arg = args.getLastArg(OPT_threads)) {
+    StringRef v(arg->getValue());
+    if (v == "all")
----------------
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


================
Comment at: lld/ELF/SyntheticSections.cpp:2753
-    concurrency = std::min<size_t>(
-        hardware_concurrency().compute_thread_count(), numShards);
 
----------------
 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.


================
Comment at: lld/ELF/SyntheticSections.cpp:3196
   size_t concurrency = 1;
-  if (threadsEnabled)
+  if (enabledThreads)
     concurrency = std::min<size_t>(
----------------
`if (parallel::Strategy.ThreadsRequested != 1)`


================
Comment at: lld/include/lld/Common/Threads.h:67
+// Number of threads. 0 means all of concurrent threads supported.
+extern unsigned enabledThreads;
 
----------------
Remove (see first comment).


================
Comment at: lld/include/lld/Common/Threads.h:71
+  // TODO Make llvm::parallel::par support different numbers of threads.
+  if (enabledThreads != 1)
     for_each(llvm::parallel::par, std::begin(range), std::end(range), fn);
----------------
`if (parallel::Strategy.ThreadsRequested != 1)`


================
Comment at: lld/include/lld/Common/Threads.h:79
                              llvm::function_ref<void(size_t)> fn) {
-  if (threadsEnabled)
+  if (enabledThreads != 1)
     for_each_n(llvm::parallel::par, begin, end, fn);
----------------
Ditto.


================
Comment at: lld/wasm/Driver.cpp:380
+  // for --thinlto-jobs.
+  if (auto *arg = args.getLastArg(OPT_threads)) {
+    StringRef v(arg->getValue());
----------------
Same comment as for the ELF part.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76885