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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


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


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


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


================
Comment at: lld/ELF/SyntheticSections.cpp:3196
   size_t concurrency = 1;
-  if (threadsEnabled)
+  if (enabledThreads)
     concurrency = std::min<size_t>(
----------------
aganea wrote:
> `if (parallel::Strategy.ThreadsRequested != 1)`
I'll simplify it further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76885