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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D131247#3706866 <https://reviews.llvm.org/D131247#3706866>, @ikudrin wrote:

> 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?

`TaskGroup` is moved outside `detail::`. The idea is that its destructor is a suitable place for the gather place.



================
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);
+    }
----------------
ikudrin wrote:
> 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?

We want to ensure that `BYTE` may overwrite a filler (`fill(start, end - start, filler);`).  I can add a comment.


================
Comment at: lld/ELF/OutputSections.cpp:471
+  // non-deterministic.
+  asyncParallelFor(tg, 128, 0, sections.size(), fn);
 }
----------------
ikudrin wrote:
> 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?
I don't think it can be derived from the common setting. It may not be bad to have a call site specific value like `SmallVector`. If we derived this automatically, tuning the library parameter may be more difficult.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131247