[PATCH] D110292: Use a deterministic order when updating the DominatorTree

Thu Sep 23 04:36:08 PDT 2021

bjope added a comment.

In D110292#3017659 <https://reviews.llvm.org/D110292#3017659>, @lebedev.ri wrote:

> I think we need to first establish whether this is a bug in DomTree/DomTreeUpdater or the passes that depend on the dom tree node order or here.

Sure! Agreed!
When I started off with this patch I thought it was a single place to update (i.e. that it really was a one-location bug). But now it seems like this was more of a design choice since the patch grew so large.

Here are some of my ideas about it (and my motivations for/against are probably pretty vague):

One assumed upside with this patch is that if passes like EarlyCSE need to make an ordering for the nodes to be iterated themselves, then that would be an extra cost (at least potentially) compared to just doing things in a deterministic order from the start.
With that being said, the accumulative cost of doing things like in this patch up-front might end up being higher than doing a single sorting later. And it will also just add a cost even if there are no passes later in the pipeline that depends on the node order. So lots of uncertainty when considering which is best for compile-time.

One downside with this is that if the analysis is invalidated (and recalculated from scratch) then we probably get a different order of the nodes. Still making things a bit shaky when debugging/bisecting/reducing test cases.
With that being said, when debugging and printing the DomTree between passes etc it could be nice if the printout is deterministic. This could of course be solved by doing some kind of sorting before printing, just like we would have to do in EarlyCSE. And the performance of the print-function should not be that important.

(maybe this discussion should move back to the llvm-dev mailthread...)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110292