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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/InputFiles.cpp:639
   }
 
+  for (ArrayRef<Elf_Word> entries : selectedGroups)
----------------
peter.smith wrote:
> Out of curiosity, is there a reason this needs to be moved from the previous place? No objections to moving it, just curious.
Moved back. The reason is that I have had a very old patch and just recently fixed some issues and posted it here. The code move was due to playing with some parallelism schemes and was not needed.


================
Comment at: lld/ELF/InputFiles.cpp:902
                                                     StringRef name) {
   if (sec.sh_type == SHT_ARM_ATTRIBUTES && config->emachine == EM_ARM) {
     ARMAttributeParser attributes;
----------------
peter.smith wrote:
> 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.
Thanks for the heads-up. I have to move the attributes code to `ObjFile<ELFT>::parse` as otherwise `in.attributes == nullptr` is racy, though benign, would likely fail thread sanitizer.


================
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));
----------------
peter.smith wrote:
> Will be worth checking `readAndFeatures` as they read and update a global value.
Thx. readAndFeatures is thread-safe.


================
Comment at: lld/include/lld/Common/Memory.h:65
 
+template <typename T>
+inline llvm::SpecificBumpPtrAllocator<T> &
----------------
peter.smith wrote:
> 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.  
> 
> 
Good catch. Yes to all 3 questions. I've added a comment to document the assumption.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130810