[PATCH] D58327: [Dominators] Simplify and optimize path compression used in link-eval forest.

Sun Feb 17 21:47:00 PST 2019

kuhar added a comment.

Thanks for the comments, it looks amazing now.

> No particular motivation when I started doing this :) I just came across this file and learned some dominators in the weekend.

Cool.

>   I haven't suffered from the worst-case O(N^2) complexity and I doubt it may happen in practice. sncaworst in Linear-Time Algorithms for Dominators and Related Problems requires an O(N) dominator tree path with O(N) children reachable from the top of the tree path. Anyway, I've changed the file header to mention this may require attention.

Great. It was indirectly mentioned in the part that described various checks, but it seems much more explicit now.

> This patch should be obvious improvement but it may be small. I think there are several other aspects that can be improved and may have a larger impact. The biggest problem from my view is the representation. The NumToNode hash table lookup immediately followed by NodeToInfo lookup is a common pattern in the implementation and this may be expensive. Label can be an unsigned instead of a NodePtr to save some indirect lookups (this is the optimization opportunity we can leverage after the transition from SLT to Semi-NCA).

The patch changes the stack size for eval, which may have unexpected impact on caching and code layout. This is often very surprising and difficult to reason about.
As expected, the current version of SNCA spends most of time on DenseMap lookups, and profiles are not very informative (at least to me). Reducing the use of hashtables can be quite some improvement.

> `IDom` can probably do the same but that seems to require more efforts. `DFSNum` may be a candidate to be removed, etc. I don't understand why numbering starts from 1 but it is only a small aesthetic issue.
Keep in mind that `IDom` is a part of the public interface of DomTreeNode.

> Seeking for suggestions of tests. What workflow do you use when changing this file? I'm currently thinking of IRTests and check-llvm-analysis when experimenting and check-llvm or check-all for a comprehensive testing.
>  Could you please share yours? Especially the statistics in the talk Dominator Trees and incremental updates that transcend time...

For the incremental updater, I'd usually manually instrument some of the GenericDomTree functions and count the number of nodes visited, times spent doing insertions, deletions, SNCA, etc. You can find some code for it in D50303 <https://reviews.llvm.org/D50303> and D36884 <https://reviews.llvm.org/D36884>. For incremental updates, I just run -O3  on some big bitcode files, like clang, opt, sqlite.

For the construction algorithm itself, I had an llvm tool that loaded an LLVM module and randomly run construction on each functions a couple of times. See D36897 <https://reviews.llvm.org/D36897>.

I usually run all tests in llvm (ninja check-all), on the llvm-test-suite with LNT, run it on the it on bitcode linked into a single file (on clang, opt, sqlite, and more recently webassembly and rippled) -- I use gllvm <https://github.com/SRI-CSL/gllvm> for it. In order to make it work with gllvm, I usually compile projects with something like this:

  export LLVM_COMPILER_PATH=my_path/llvm-9.0/bin
  cmake .. -GNinja -DCMAKE_CXX_COMPILER=/home/kuba/go/bin/gclang++ DCMAKE_C_COMPILER=/home/kuba/go/bin/gclang -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS='-Xclang -disable-llvm-optzns' -DCMAKE_C_FLAGS='-Xclang -disable-llvm-optzns'

And then extract bitcode with `get-bc` and feed it into `opt` with `-O3` -- aggressive inlining helps with exercising dominator construction due to larger CFGs.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58327