[all-commits] [llvm/llvm-project] 963378: [ORC] Improve computeLocalDeps / computeNamedSymbo...

[lhames via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 963378bd8278220eb382bec76846ef39e4ea597e
      https://github.com/llvm/llvm-project/commit/963378bd8278220eb382bec76846ef39e4ea597e
  Author: Lang Hames <lhames at gmail.com>
  Date:   2021-07-08 (Thu, 08 Jul 2021)

  Changed paths:
    M llvm/include/llvm/ExecutionEngine/Orc/MachOPlatform.h
    M llvm/include/llvm/ExecutionEngine/Orc/ObjectLinkingLayer.h
    M llvm/lib/ExecutionEngine/Orc/MachOPlatform.cpp
    M llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp

  Log Message:
  -----------
  [ORC] Improve computeLocalDeps / computeNamedSymbolDependencies performance.

The computeNamedSymbolDependencies and computeLocalDeps methods on
ObjectLinkingLayerJITLinkContext are responsible for computing, for each symbol
in the current MaterializationResponsibility, the set of non-locally-scoped
symbols that are depended on. To calculate this we have to consider the effect
of chains of dependence through locally scoped symbols in the LinkGraph. E.g.

        .text
        .globl  foo
foo:
        callq   bar                    ## foo depneds on external 'bar'
        movq    Ltmp1(%rip), %rcx      ## foo depends on locally scoped 'Ltmp1'
        addl    (%rcx), %eax
        retq

        .data
Ltmp1:
        .quad   x                      ## Ltmp1 depends on external 'x'

In this example symbol 'foo' depends directly on 'bar', and indirectly on 'x'
via 'Ltmp1', which is locally scoped.

Performance of the existing implementations appears to have been mediocre:
Based on flame graphs posted by @drmeister (in #jit on the LLVM discord server)
the computeLocalDeps function was taking up a substantial amount of time when
starting up Clasp (https://github.com/clasp-developers/clasp).

This commit attempts to address the performance problems in three ways:

1. Using jitlink::Blocks instead of jitlink::Symbols as the nodes of the
dependencies-introduced-by-locally-scoped-symbols graph.

Using either Blocks or Symbols as nodes provides the same information, but since
there may be more than one locally scoped symbol per block the block-based
version of the dependence graph should always be a subgraph of the Symbol-based
version, and so faster to operate on.

2. Improved worklist management.

The older version of computeLocalDeps used a fixed worklist containing all
nodes, and iterated over this list propagating dependencies until no further
changes were required. The worklist was not sorted into a useful order before
the loop started.

The new version uses a variable work-stack, visiting nodes in DFS order and
only adding nodes when there is meaningful work to do on them.

Compared to the old version the new version avoids revisiting nodes which
haven't changed, and I suspect it converges more quickly (due to the DFS
ordering).

3. Laziness and caching.

Mappings of...

jitlink::Symbol* -> Interned Name (as SymbolStringPtr)
jitlink::Block* -> Immediate dependencies (as SymbolNameSet)
jitlink::Block* -> Transitive dependencies (as SymbolNameSet)

are all built lazily and cached while running computeNamedSymbolDependencies.

According to @drmeister these changes reduced Clasp startup time in his test
setup (averaged over a handful of starts) from 4.8 to 2.8 seconds (with
ORC/JITLink linking ~11,000 object files in that time), which seems like
enough to justify switching to the new algorithm in the absence of any other
perf numbers.


  Commit: d7afd11e3dc14d50156618cb27689f1425239c86
      https://github.com/llvm/llvm-project/commit/d7afd11e3dc14d50156618cb27689f1425239c86
  Author: Lang Hames <lhames at gmail.com>
  Date:   2021-07-08 (Thu, 08 Jul 2021)

  Changed paths:
    M llvm/include/llvm/ExecutionEngine/Orc/Shared/CommonOrcRuntimeTypes.h

  Log Message:
  -----------
  [ORC] Introduce ExecutorAddress type, fix broken LLDB bot.

ExecutorAddressRange depended on JITTargetAddress, but JITTargetAddress is
defined in ExecutionEngine, which OrcShared should not depend on.

This seems like as good a time as any to introduce a new ExecutorAddress type
to eventually replace JITTargetAddress. For now it's just another uint64_t
alias, but it will soon be changed to a class type to provide greater type
safety.


Compare: https://github.com/llvm/llvm-project/compare/0fd5e7b2d8ca...d7afd11e3dc1