[ORC] Merge MaterializationResponsibility notifyEmitted and addDepend… #79952
Conversation
…encies. Removes the MaterializationResponsibility::addDependencies and addDependenciesForAll methods, and transfers dependency registration to the notifyEmitted operation. The new dependency registration allows dependencies to be specified for arbitrary subsets of the MaterializationResponsibility's symbols (rather than just single symbols or all symbols) via an array of SymbolDependenceGroups (pairs of symbol sets and corresponding dependencies for that set). This patch aims to both improve emission performance and simplify dependence tracking. By eliminating some states (e.g. symbols having registered dependencies but not yet being resolved or emitted) we make some errors impossible by construction, and reduce the number of error cases that we need to check. NonOwningSymbolStringPtrs are used for dependence tracking under the session lock, which should reduce ref-counting operations, and intra-emit dependencies are resolved outside the session lock, which should provide better performance when JITing concurrently (since some dependence tracking can happen in parallel). The Orc C API is updated to account for this change, with the LLVMOrcMaterializationResponsibilityNotifyEmitted API being modified and the LLVMOrcMaterializationResponsibilityAddDependencies and LLVMOrcMaterializationResponsibilityAddDependenciesForAll operations being removed.
lhames
requested review from
vchuravy and
weliveindetail
and removed request for
vchuravy
|
@vchuravy This is the patch that changes the LLVM C API for dependence handling, in case it's of interest. |
|
Posting mostly for visibility. The patch is fairly large -- reviews very welcome, but not expected. |
|
Looks like I need to fix some test cases on Linux and Windows. |
There was a problem hiding this comment.
Thanks for the heads-up. As you see I mostly read the comments and found cosmetic issues. For a thorough review, the patch is a bit large indeed.
Just to double-check: It remains the object layer that is responsible for calculating the registering the dependencies right? Does the patch make an observable difference for existing use-cases, like LLJIT? My impression is that it doesn't, but it adds the option to tweak dependencies on symbol-level.
| void ExecutionSession::propagateExtraEmitDeps( | ||
| std::deque<JITDylib::EmissionDepUnit *> Worklist, EDUInfosMap &EDUInfos) { | ||
|
|
||
| // Iterate to a fixed-poient to propagate extra-emit dependencies through the |
There was a problem hiding this comment.
Found this typo by accident, I guess it's fixed-point?
| } | ||
| #endif // NDEBUG | ||
|
|
||
| // 3. Use the DegGroups DepGroups array to build a graph of dependencies |
There was a problem hiding this comment.
One more typo, DegGroups?
| @@ -7,8 +7,13 @@ | |||
| # symbols: _baz depends on _foo indirectly via the local symbol _bar. We expect | |||
| # _baz to depend on _foo, and _foo on _external_func. | |||
There was a problem hiding this comment.
What about updating this comment with what we expect simplification to do? It wasn't immediately obvious to me, but one of the comments in simplifyDepGroups() was useful. Maybe something like "propagate dependencies to symbols outside this unit"?
|
|
||
| // Verify that trying to resolve Foo fails. | ||
| EXPECT_THAT_ERROR(FooR->notifyResolved({{Foo, FooSym}}), Failed()) | ||
| EXPECT_THAT_ERROR(FooR->notifyResolved({{Foo, FooSym}}), Succeeded()) | ||
| << "Expected resolution for \"Foo\" to fail."; |
There was a problem hiding this comment.
Missed to update the message here? to succeed
| /// elements of DepGroups must be non-overlapping (no symbol should appear in | ||
| /// more than one of hte symbol sets), but do not have to be exhaustive. Any | ||
| /// symbol in this MaterializationResponsibility object that is not covered | ||
| /// by an entry will be take to have no dependencies. |
There was a problem hiding this comment.
Not sure, but will have no dependencies maybe?
Yes: the object linking layers will continue to calculate and register these dependencies for you. In general it's now the responsibility of whoever calls |
When an EDU being emitted gains a new dependency (as a result of emission of some existing dependency which has its own unsatisfied dependencies) we need to add the EDU to the DependantEDUs sets for each of the new dependencies.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…encies.
Removes the MaterializationResponsibility::addDependencies and addDependenciesForAll methods, and transfers dependency registration to the notifyEmitted operation. The new dependency registration allows dependencies to be specified for arbitrary subsets of the MaterializationResponsibility's symbols (rather than just single symbols or all symbols) via an array of SymbolDependenceGroups (pairs of symbol sets and corresponding dependencies for that set).
This patch aims to both improve emission performance and simplify dependence tracking. By eliminating some states (e.g. symbols having registered dependencies but not yet being resolved or emitted) we make some errors impossible by construction, and reduce the number of error cases that we need to check. NonOwningSymbolStringPtrs are used for dependence tracking under the session lock, which should reduce ref-counting operations, and intra-emit dependencies are resolved outside the session lock, which should provide better performance when JITing concurrently (since some dependence tracking can happen in parallel).
The Orc C API is updated to account for this change, with the LLVMOrcMaterializationResponsibilityNotifyEmitted API being modified and the LLVMOrcMaterializationResponsibilityAddDependencies and LLVMOrcMaterializationResponsibilityAddDependenciesForAll operations being removed.