[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
llvm-commits at lists.llvm.org
Mon Mar 30 07:33:14 PDT 2020
- Previous message: [PATCH] D76885: [lld][COFF][ELF][WebAssembly] Replace --[no-]threads /threads[:no] with --threads={all,1,2,...} /threads:{all,1,2,...}
- Next message: [PATCH] D76885: [lld][COFF][ELF][WebAssembly] Replace --[no-]threads /threads[:no] with --threads={all,1,2,...} /threads:{all,1,2,...}
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
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
- Previous message: [PATCH] D76885: [lld][COFF][ELF][WebAssembly] Replace --[no-]threads /threads[:no] with --threads={all,1,2,...} /threads:{all,1,2,...}
- Next message: [PATCH] D76885: [lld][COFF][ELF][WebAssembly] Replace --[no-]threads /threads[:no] with --threads={all,1,2,...} /threads:{all,1,2,...}
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
More information about the llvm-commits
mailing list