[PATCH] D53245: Teach the DominatorTree fallback to recalculation when applying updates to speedup JT (PR37929)

Sat Oct 13 09:24:49 PDT 2018

NutshellySima added a comment.

In https://reviews.llvm.org/D53245#1264493, @kuhar wrote:

> Thanks for looking into this. A couple of high-level questions:
>
> - how was the constant "650" picked? Is it just playing with different values and choosing something that seemed reasonable?
> - why is the threshold independent of the tree size? Intuitively the policy could be to recalculate if the number of updates is greater than some multiple of size. Maybe we can refer to the average-case complexity from Lucuas' paper?
> - why do we want to put it in the incremental updater instead of the DomTreeUpdater class?

- For the constant "650", I used the 2nd reproducer in bug PR37929 and collected some statistics on the size of the updates after legalization and the time used by applying updates. The statistics shows that the time complexity is about Θ(n^2.4) (let n be the size of the update vector after legalization) and it seems that at least for a CFG size no larger than the one in PR37929, "650" is a reasonable value that the time used by recalculating is sure to be less than applying updates directly. (I think there sure should be some better heuristics deciding this.)
- It is definitely better to get it related to the tree size but I'm not sure how to get information on the tree size in LLVM and also lack of inputs with different CFG sizes to experiment on.
- I acknowledged this as an issue of the incremental updater not handling a huge update vector correctly; It definitely should fallback to recalculating if it is much faster. I believe this constant is related to the size of updates after legalization so I put it in the incremental updater. [Currently, there is a similar functionality in DTU which is Ω(n^2) and I believe it is slow and (redundant if JT submits updates normally) so I am considering removing it from DTU, then it is impossible to know the size of the legalized update vector]. Besides, it can also benefit other places not using the DomTreeUpdater.

Repository:
  rL LLVM

https://reviews.llvm.org/D53245