[PATCH] D25452: [LTO] Split the options for ThinLTO jobs and Regular LTO partitions

[Peter Collingbourne via llvm-commits]

pcc added inline comments.


================
Comment at: ELF/Driver.cpp:487
+    error("--lto-partitions: number of threads must be > 0");
+  Config->ThinLtoJobs = getInteger(Args, OPT_thinlto_jobs, 1);
+  if (Config->ThinLtoJobs == 0)
----------------
mehdi_amini wrote:
> pcc wrote:
> > I think this would result in a default parallelism level of 1. Can we avoid passing a ThinBackend if the user did not specify a parallelism level?
> Doesn't it mean that if the user does not specify `--thinlto-jobs` on the command line, ThinLTO won't work? I think it would even lead to the LTO library crashing if the bitcode has been generated with ThinLTO? Is there a test that shows what happen in this case? (I don't see any, but I didn't look closely)
> The default should be something like `std::hardware_concurrency`.
No, if the ThinBackend is not supplied, LTO will create one for you. See: http://llvm-cs.pcc.me.uk/lib/LTO/LTO.cpp#229

I think we will eventually want to replace the `thread::hardware_concurrency()` default with something else that will lead to better utilisation of the hardware. In Chromium at least we've observed that `hardware_concurrency` leads to poor utilisation on machines with a large number of cores (https://bugs.chromium.org/p/chromium/issues/detail?id=645295#c20). So I'd prefer to keep any default out of the clients.

You're right, we should have a test that does not pass a `--thinlto-jobs` flag. @davide can you please add one?


================
Comment at: test/ELF/lto/thinlto.ll:2
 ; Basic ThinLTO tests.
 ; RUN: llvm-as %s -o %t.o
 ; RUN: llvm-as %p/Inputs/thinlto.ll -o %t2.o
----------------
I think you need to use `opt -module-summary` here.


https://reviews.llvm.org/D25452