[PATCH] D87966: [ThinLTO] Re-order modules for optimal multi-threaded processing

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 13 15:51:07 PDT 2020


tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

In D87966#2328736 <https://reviews.llvm.org/D87966#2328736>, @aganea wrote:

> @tejohnson I added the test in LLD, since the gold tests only run on Linux, which is harder for me to test & debug. The test fails when the following block is removed: `if (BackendProc->getThreadCount() == 1) { ... }`.

Great, thanks! LGTM with request for comment as described below.

> This test also implictly covers the "InProcessThinBackend" codepath with `/opt:lldltojobs=1` which I don't see how to cover explicitly otherwise.

I don't see how this test covers that code path since it isn't using in process backends, but I'm not really sure how to test this case effectively - like the original patch, it is just a performance change on the in process backends case.



================
Comment at: llvm/lib/LTO/LTO.cpp:1469
+  if (RegularLTO.ParallelCodeGenParallelismLevel == 1 ||
+      BackendProc->kind() == ThinBackendProc::WriteIndexesKind) {
+    // Process the modules in the order they were provided.
----------------
aganea wrote:
> tejohnson wrote:
> > Needs comment as to why we are not doing this for WriteIndexesKind
> With the new `ThinBackendProc::getThreadCount()` things are a bit more explicit I think.
The virtual method looks good and cleaner. But I'd like to capture in a comment somewhere that for write indexes thin backend there is a user visible effect if we were to reorder, in that the LinkedObjectsFile written for distributed ThinLTO will contain the objects in a different order, which will in turn affect the final link order assuming that file is used as intended. I.e. we don't just skip it in the thread = 1 case because it is unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87966



More information about the llvm-commits mailing list