[PATCH] D131247: [ELF] Parallelize writes of different OutputSections

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added a comment.

I am not really happy that the patch exposes some implementation details (i.e `TaskGroup` and requires a value of `TaskSize`) that are best to be hidden, but I, honestly, do not have a better idea ready. Maybe there can be a wrapper class that encloses a `TaskGroup` and where `asyncParallelFor` can be moved as a method? Maybe it even could automatically adjust `TaskSize` based on `parallel::detail::MaxTasksPerGroup` and the count of already added tasks?



================
Comment at: lld/ELF/OutputSections.cpp:457-465
+  bool written = false;
   for (SectionCommand *cmd : commands)
-    if (auto *data = dyn_cast<ByteCommand>(cmd))
+    if (auto *data = dyn_cast<ByteCommand>(cmd)) {
+      if (!std::exchange(written, true))
+        parallelFor(0, sections.size(), fn);
       writeInt(buf + data->offset, data->expression().getValue(), data->size);
+    }
----------------
I cannot find a requirement for `BYTE()` commands to override the content of input sections. Isn't the script simply malformed in that case? Can't we add these writings to the pool too?


================
Comment at: lld/ELF/OutputSections.cpp:471
+  // non-deterministic.
+  asyncParallelFor(tg, 128, 0, sections.size(), fn);
 }
----------------
Where does the value of `128` come from? It looks like, with the hardcoded value, large projects will benefit from the parallelization more than small ones. And it also does not take into account the `llvm::parallel::detail::MaxTasksPerGroup`; can it somehow be derived from the common setting?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131247