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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a subscriber: tejohnson.
aganea added a comment.

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



================
Comment at: lld/COFF/Driver.cpp:1156
+            arg->getValue() + "'");
+    parallel::strategy.ThreadsRequested = threads;
+  }
----------------
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).


================
Comment at: lld/COFF/Driver.cpp:1157
+    parallel::strategy.ThreadsRequested = threads;
+  }
 
----------------
Overwrite `config->thinLTOJobs` here, like the ELF part.


================
Comment at: lld/COFF/Options.td:223
+def threads : P<"threads", "Number of threads. 'all' (default) means all of "
+                           "concurrent threads supported">;
 
----------------
```
"Number of threads. 'all' (default) means all of concurrent threads supported. '1' disables multi-threading."
```
Same comment for the other drivers.


================
Comment at: lld/ELF/Driver.cpp:1047
+    parallel::strategy.ThreadsRequested = threads;
+    config->thinLTOJobs = v;
+  }
----------------
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 


================
Comment at: llvm/lib/Support/Parallel.cpp:23
 
+llvm::ThreadPoolStrategy llvm::parallel::strategy;
+
----------------
MaskRay wrote:
> aganea wrote:
> > `llvm::parallel::Strategy`. LLVM uses first letter uppercase for variables, whereas LLD uses lowercase.
> `llvm::parallel::{seq,par}` are lower cased variables. I think this should not matter.
Please add a comment above signifying that this defaults to using all hardware threads.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76885