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

Sun Feb 17 20:37:36 PST 2019

MaskRay added a comment.

In D58327#1400612 <https://reviews.llvm.org/D58327#1400612>, @kuhar wrote:

> Hi @MaskRay,

Thanks for the quick feedback!

> This looks like a fantastic improvement! The only issue is that the code, both original and after your changes, lacks comments.
> 
> For example, I can see you put a comment that identified that the second part of the function does path compression, but it's not clear how exactly, and what is the first part of the function doing.

I added some comments explaining the `eval()` function as used in the link-eval forest. The file header didn't say Semi-NCA is O(N^2) so I have changed it to mention that, as the original one discussing `eval()` time complexity may give people false impression that this is almost linear or O(N log N) (at least me when I was reading the implementation).

> Next, what is the motivation behind this change? Have you identified some bottlenecks in the path compression part, have you suffered form the pathological quadratic complexity of SemiNCA? Perhaps it would make more sense to implement a hybrid algorithm that uses Lengauer-Tarjan for the initial full construction and SemiNCA for incremental updates? I have been thinking about it for quite some time now, but there weren't any complaints about the quadratic complexity yet, thus I figured the additional maintenance cost is not justified.

No particular motivation when I started doing this :) I just came across this file and learned some dominators in the weekend. 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.

> What was your evaluation for this patch? What benchmarks did you run? How do the numbers look like?

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

I'll have to read //An Experimental Study of Dynamic Dominators// and the other parts of the dominator implementation in llvm first to get a better idea where can be improved :)

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

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58327