[PATCH] D130810: [ELF] Parallelize input section initialization

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: lld/ELF/Driver.cpp:2389
+static void initSectionsAndLocalSyms(ELFFileBase *file,
+                                     bool ignoreComdats = false) {
   switch (config->ekind) {
----------------
I don't think any caller has used the ignoreComdats default parameter, is it worth removing the `= false` ?


================
Comment at: lld/ELF/InputFiles.cpp:438
+  }
+
+  ArrayRef<Elf_Shdr> objSections = getELFShdrs<ELFT>();
----------------
Worth a comment along the lines of:
"Handle dependent libraries and selection of ELF Section Groups as these cannot be done in parallel."


================
Comment at: lld/ELF/InputFiles.cpp:639
   }
 
+  for (ArrayRef<Elf_Word> entries : selectedGroups)
----------------
Out of curiosity, is there a reason this needs to be moved from the previous place? No objections to moving it, just curious.


================
Comment at: lld/ELF/InputFiles.cpp:902
                                                     StringRef name) {
   if (sec.sh_type == SHT_ARM_ATTRIBUTES && config->emachine == EM_ARM) {
     ARMAttributeParser attributes;
----------------
IIUC `createInputSection` can be called in parallel now from `initializeSections` will be worth a comment as a warning to what is safe to be done within the function and any functions called from it.

LLD's handling of Arm build attributes is somewhat threadbare at the moment. I expect that Arm will want to send in some patches to improve this in the future. When they do I think this will need to be moved to avoid threading problems. The current implementation gets away with it as I don't think it ever reads from the global state, just writes to it.


================
Comment at: lld/ELF/InputFiles.cpp:919
       if (in.attributes == nullptr) {
         in.attributes = std::make_unique<InputSection>(*this, sec, name);
         return in.attributes.get();
----------------
I'm guessing make_unique is thread safe in this context?


================
Comment at: lld/ELF/InputFiles.cpp:973
     // file's .note.gnu.property section.
     if (name == ".note.gnu.property") {
       this->andFeatures = readAndFeatures<ELFT>(InputSection(*this, sec, name));
----------------
Will be worth checking `readAndFeatures` as they read and update a global value.


================
Comment at: lld/include/lld/Common/Memory.h:65
 
+template <typename T>
+inline llvm::SpecificBumpPtrAllocator<T> &
----------------
Apologies in advance, I'm not too familiar with LLVM in this area:
* As I understand it, we'll be creating a new allocator per thread, and storing it in TLS? 
* I'm assuming that if the underlying threads are destroyed then the allocator and all the memory it points to will be destroyed as well?
* I'm assuming that we're relying on parallelForEach never destroying threads during the lifetime of a link? Is this safe to rely on?

Would be useful to document the assumptions, as we may have to implement a different solution if `parallelForEach` ever changes policy in the future.  




================
Comment at: lld/include/lld/Common/Memory.h:79
+// allocators instead of doing context indirection and pthread_getspecific.
+template <typename T, typename... U> T *makeTL(U &&...args) {
+  return new (getSpecificAllocSingletonThreadLocal<T>().Allocate())
----------------
I suggest `makeThreadLocal` rather than `makeTL` as it isn't obvious from the call site that TL is Thread Local and there are not that many calls to it. Not a strong opinion though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130810