[PATCH] D110018: [lld-macho] Speed up markLive()

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/MarkLive.cpp:97-120
+      // TODO:  we can avoid iterating inputSections multiple times.
+      //
+      // S_ATTR_LIVE_SUPPORT sections are live if they point _to_ a live
+      // section.
+      // Process them in a second pass.
+      if (!skip) {
+        for (size_t i = start; i <= end; ++i) {
----------------
oontvoo wrote:
> int3 wrote:
> > I'm kind of suspicious of this. It's marking sections as live based on the liveness of other sections, which are concurrently being marked as live. Depending on the order of writes, it may incorrectly conclude that a certain live section X is not live, and therefore not mark the sections that X points to as live.
> Good point! (kind of surprised not a lot of tests were failing ...)
> 
Race conditions often don't show up on small inputs :) I would recommend running dead_strip on a large program before and after this change, and checking that the outputs are identical.

I think the easiest way to fix this is to create a mapping of sections to their live support sections (basically reversing the pointer), and then have `addSect` visit the live support sections.


================
Comment at: lld/MachO/MarkLive.cpp:171-172
+
+    if (start < idx || end >= size) {
+      if (idx < size) {
+        start = end = idx;
----------------
oontvoo wrote:
> int3 wrote:
> > i'm kind of confused here. why are we comparing `start` to `idx` -- isn't `start` an index into the `size` array of jobs, whereas `idx` is the index of the `Task` vector of size `poolSize`? seems like we're comparing numbers on different scales...
> > 
> > same thing for `idx` and `size`
> "idx" is the index of the Task vector, yes but it correlates to where its work items are:
> 
> So the division of labour is like this:
> 
> ```
> Task[0] : start=0, end = start + d -1 = d -1 
> Task[1] : start=d, end = d + d - 1
> Task[2]: start = d + d, end = d + d + d -1
> ....
> Task[n]: start = n * d, end = start + d -1.
> ```
right but... what does `start < idx` mean here, and when do we expect it to be true? shouldn't it never be true, since `n >= n * d` for all nonnegative `n` and `d`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110018