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


================
Comment at: llvm/lib/Support/Parallel.cpp:23
 
+llvm::ThreadPoolStrategy llvm::parallel::strategy;
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76885