[PATCH] D54730: [DomTree] Fix order of domtree updates in MergeBlockIntoPredecessor.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.
Herald added a project: LLVM.

Since I sent out D62981 <https://reviews.llvm.org/D62981> with identical changes and the same motivation (thank you for pointing this out @NutshellySima), please let me add a small comment here.

> Intuitively, it would make sense to schedule insertions when we anticipate many `DeleteUnreachable` deletions -- insertions cause the CFG to be more dense, which turns unreachable deletions into reachable ones. I'm don't know of any way to guess whether a batch of deletions will cause more unreachable than reachable deletions, and I don't believe scheduling insertions before deletions is always going to be profitable.

I think this is entirely correct. We have no way of knowing *in general* that insertions are more profitable to do before deletions. So I take back the comment of "let's sort these in the DTU".
In this *particular case* however (inside MergeBlockIntoPredecessor), I think it *is* always profitable. This method only merges 2 blocks PredBB-BB, where PredBB has no other successors and BB has no other predecessors. Deleting edges is very likely to make some blocks unreachable, then again reachable after an insert.
So my suggestion would be to make the change only inside MergeBlockIntoPredecessor with a detailed comment explaining why the order matters here.

Thoughts?

@efriedma: Do you have the time to continue this, or should I take over in D62981 <https://reviews.llvm.org/D62981> instead?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54730