[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 inline comments.


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


================
Comment at: llvm/lib/Support/Parallel.cpp:23
 
+llvm::ThreadPoolStrategy llvm::parallel::strategy;
+
----------------
`llvm::parallel::Strategy`. LLVM uses first letter uppercase for variables, whereas LLD uses lowercase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76885