[PATCH] D147493: [ELF] Cap parallel::strategy to 16 threads when --threads= is unspecified

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D147493#4283904 <https://reviews.llvm.org/D147493#4283904>, @andrewng wrote:

> In D147493#4281429 <https://reviews.llvm.org/D147493#4281429>, @MaskRay wrote:
>
>> Good catch. The code should set `config->threadCount`. `config->threadCount` is currently only used for parallel relocation scanning whether an over-sized value still works.
>> I think not setting `config->threadCount` is correct but weird. How about?
>>
>>   } else if (config->threadCount > 16) {
>>     log("set maximum concurrency to 16, specify --threads= to change");
>>     config->threadCount = 16;
>>     parallel::strategy = hardware_concurrency(config->threadCount);
>>   }
>
> Yes, this LGTM for this case, but I think something also needs to be done for the explicit setting of the `--threads` option just above. I think it needs: `config->threadCount = parallel::strategy.compute_thread_count()` after the setting of `parallel::strategy`. IIUC, `compute_thread_count()` will take into account any CPU affinity settings. Having now looked more closely at that code, I do wonder if the default value for `config->thinLTOJobs` should be the same as `config->threadCount`, i.e. take into account any CPU affinity settings.

Thanks. For clarity I reverted the commit and reused the differential for a proper fix.

For in-process ThinLTO, the summary says

> --thinlto-jobs= is unchanged since ThinLTO backend compiles are embarrassingly parallel.

Any measurement is welcome:)  `llvm::heavyweight_hardware_concurrency(config->thinLTOJobs)` is used for this option, along with several other uses across llvm-project.
It will be a good measure whether using `heavyweight_hardware_concurrency` brings improvement over `hardware_concurrency`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147493