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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

In D153268#4438509 <https://reviews.llvm.org/D153268#4438509>, @JDevlieghere wrote:

> 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.

yes, it would be good to have single implementation. But I do not see the simple way to do it.
Actually there are other cases using similar code. f.e.

  llvm/unittests/DebugInfo/DWARF/DwarfGenerator.h
  D130315. 

Thus, it would be useful if we have kind of DWARFLinkerBase library, containing general code for cloning, creating and emitting DIEs.
That library might be used by DWARFLinker, DWARFLinkerParallel, unittests for DebugInfo, BOLT.
Though, I think there are a lot of details which will make implementing that library difficult task.

Anyway, if we want to have single implementation for it then, probably, the best way would be to think of creating DWARFLinkerBase library.
That library should have base implementations for compile units, for code cloning, creating and emitting DIEs, this library should not depend on AsmPrinter.
What do you think?

> - 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.

If I understood correctly, it looks like the current picture is quite close to that description:

The CompileUnit contains data shared between stages and compile units know the current stage.
There is a separate class doing liveness analysys: DependencyTracker.
This class uses data keeping by CompileUnit(this data is shared context).
The result of liveness analysys is used by other stages that is why compile unit keeps the data.

We need "init" and "reset" functions as we need to erase data keeping by compile unit when we return to previous stages.
i.e. when we return to the previous stage we need to adapt shared context.
Probably instead of "init" and "reset" we need to name methods "sync shared context with the current stage" ?



================
Comment at: llvm/lib/DWARFLinkerParallel/DWARFLinkerCompileUnit.h:262
+  uint64_t getDieOutOffset(uint32_t Idx) {
+    return reinterpret_cast<std::atomic<uint64_t> *>(&OutDieOffsetArray[Idx])
+        ->load();
----------------
JDevlieghere wrote:
> 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?
std::atomic<> does not have copy constructor/operator.  Because of this it could not be put into SmallVector.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153268