[PATCH] D153268: [DWARFLinkerParallel] Add limited functionality to DWARFLinkerParallel.

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

I don't have all of dsymutil paged in to do a really thorough/meaningful review of all the details. Even if I did I'm sure a bunch of stuff would slip through that would only be caught by real world qualification.

At a high level I think the approach makes sense. I've spent quite a bit of time thinking of alternatives and I cant't think of anything more efficient given the constraints that we have (i.e. not being able to keep all the input DWARF concurrently in memory).

As for the implementation itself:

- I like that you factored out things like the attribute cloning and I wonder if we could hoist this up and make it work with the non-parallel linker as well. I don't think we should go to extreme lengths to unify the two implementations, but even if we were to switch over to the new (parallel) implementation tomorrow, I expect the old one to be around for at least a few more years and I'd hate to have to maintain both.
- I wonder if we could make the stages more explicit. If I'm reading the code right, the data (i.e. the DIE) and the logic (i.e. the live analysis) are shared in the same class and we have to maintain state. I would prefer a design where the two are decoupled and the data moves through the different stages. The way I'm imagining this is to have a class for every stage, possibly with its own shared context, and then have the DIEs move  through them one after the other. It's fine for the DIE to know what stage we're in, but it would avoid things like `init` and `reset` iteration.

I left some inline comments here and there but I definitely need to do another pass.



================
Comment at: llvm/lib/DWARFLinkerParallel/DIEAttributeCloner.h:47
+  ///
+  /// Can not be used asynchroniously.
+  struct AttributesInfo {
----------------



================
Comment at: llvm/lib/DWARFLinkerParallel/DIEAttributeCloner.h:67
+protected:
+  // Clone string attribute.
+  size_t
----------------
Let's make these Doxygen style comments as well. 


================
Comment at: llvm/lib/DWARFLinkerParallel/DWARFLinkerCompileUnit.h:26
+  /// The stages of new compile unit processing.
+  enum class Stages : uint8_t {
+    /// Created, linked with input DWARF file.
----------------
This should be singular: i.e. `Stage`. 


================
Comment at: llvm/lib/DWARFLinkerParallel/DWARFLinkerCompileUnit.h:96
+  /// created at later stages.
+  void eraseForNewIteration();
+
----------------
How about `clear` or `reset`?


================
Comment at: llvm/lib/DWARFLinkerParallel/DWARFLinkerCompileUnit.h:262
+  uint64_t getDieOutOffset(uint32_t Idx) {
+    return reinterpret_cast<std::atomic<uint64_t> *>(&OutDieOffsetArray[Idx])
+        ->load();
----------------
We need the `reinterpret_cast` because the `OutDieOffsetArray` stores `uint64_t`s instead of `std::atomic<uint64_t>`. Why can't we store the latter?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153268