[jakobbotsch]

Factor the dominator tree computation out to a FlowGraphDominatorTree class. Compute preorder and postorder numbers for this tree. Switch RBO and hoisting from using fgDominate which is querying dominator information computed all the way back in loop finding to actually use the dominator tree computed by SSA.

Small number of diffs are expected from RBO now not rejecting some cases because of more precise treatment of EH constructs. TP may be regressed a bit due to now numbering SSA's dominator tree -- we can regain that TP once we remove the old dominator computation.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Factor the dominator tree computation out to a FlowGraphDominatorTree class. Compute preorder and postorder numbers for this tree. Switch RBO and hoisting from using fgDominate which is querying dominator information computed all the way back in loop finding to actually use the dominator tree computed by SSA.

Small number of diffs are expected from RBO now not rejecting some cases because of more precise treatment of EH constructs. TP may be regressed a bit too due to now numbering SSA's dominator tree -- we can regain that TP once we remove the old dominator computation.

Author:	jakobbotsch
Assignees:	jakobbotsch
Labels:	area-CodeGen-coreclr
Milestone:	-

[jakobbotsch]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @AndyAyersMS. Should be ready assuming CI passes, the current failure looks like some sort of infra issue, I see no error in the console log.

Diffs. Surprisingly a TP improvement; I suppose SSA's dominator computation is just better optimized than fgComputeDoms() is, even though it also takes proper EH flow into account.

[jakobbotsch]

I'll submit a PR right after this one that replaces the above quirk with this code, so I left it here. It seems like it has some nice improvements.

[AndyAyersMS]

Are there any users of the old DomTreeVisitor left?

[jakobbotsch]

Are there any users of the old DomTreeVisitor left?

Yes, we still have fgNumberDomTree to compute the preorder/postorder numbers used by fgDominate. We're still using that for the old loop finding and for optFindAndScaleGeneralLoopBlocks. The latter is especially problematic since it makes use of both dominators and reachability, so I need to figure out what to do with it... Ideally I'd like to completely remove it, but will have to see the effects of that, particularly on prejit and NAOT where we don't have PGO. If I end up not being able to get rid of it I'll probably switch it to the new dominators added in this PR and compute the dense reachability sets only inside that particular pass.

[AndyAyersMS]

The latter is especially problematic since it makes use of both dominators and reachability

If we're finding the same set of loops (or a superset) with the new logic, then the reachability checks can become loop membership, and the dominator checks can become DFST ancestry checks, so seems like neither reachability nor dominance is actually needed?

[jakobbotsch]

If we're finding the same set of loops (or a superset) with the new logic, then the reachability checks can become loop membership, and the dominator checks can become DFST ancestry checks, so seems like neither reachability nor dominance is actually needed?

I haven't tried to understand what it is doing there, but from the function header/logic it seems like it might be more general than just natural loops. Not sure how much it matters... I'll try playing around with it when I get to that point.