[PATCH] D133003: [ELF] Parallelize relocation scanning

[Andrew Ng via Phabricator via llvm-commits]

andrewng added a comment.

> If the `NEEDS_*` change looks good, I'll pre-commit it (without using `std::atomic`) to reduce diff for future updates.

The `NEEDS_*` change LGTM.

This approach definitely looks better and hasn't added too much complexity. Initial testing on Windows is looking good, but I need to do a bit more.



================
Comment at: lld/ELF/Relocations.cpp:1559
+    };
+    if (serial)
+      fn();
----------------
I wonder if it might be worthwhile using the previous code for the serial case? Although, it probably doesn't make a big difference to performance.


================
Comment at: lld/ELF/Symbols.h:318
+  }
+  bool hasFlag(uint16_t bit) const {
+    return flags.load(std::memory_order_relaxed) & bit;
----------------
The argument name implies a single bit but perhaps add an assert, e.g. `assert((bit & (bit - 1)) == 0)`?


================
Comment at: lld/ELF/SyntheticSections.h:546-547
   SmallVector<DynamicReloc, 0> relocs;
+  // Used when parallel relocation scanning adds relocations. The elements
+  // should will be moved into relocs.
+  SmallVector<SmallVector<DynamicReloc, 0>, 0> relocsVec;
----------------
Typo: `should will` -> `will`?

Is it worth adding the same comment to `relocsVec` in `class RelrBaseSection`?


================
Comment at: llvm/lib/Support/Parallel.cpp:21
 llvm::ThreadPoolStrategy llvm::parallel::strategy;
+thread_local int llvm::parallel::threadIndex;
 
----------------
Perhaps `int` -> `unsigned`?


================
Comment at: llvm/lib/Support/Parallel.cpp:53
+        Threads.emplace_back([=] {
+          threadIndex = I;
+          work(S, I);
----------------
Perhaps move this initialisation of `threadIndex` and the one below into `work()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133003