updating domtrees dynamically, removing all unreachable blocks #33730

yhls · 2019-10-31T16:14:48Z

This all started with #29107, which (is at least one thing that) stands in the way of turning DCE back on globally (it was disabled in #29265). The problem is that when there is only one predecessor, replacing a phi node with its only possible value can result in a forward reference when this phi node is in an unreachable loop. This can cause problems with passes that process all IR, whether or not the code is dead.

We can remove dead loops by removing all unreachable blocks, instead of just removing blocks with no predecessors, which is what we currently do. To determine reachability, all that would be required is a depth-first traversal of the CFG, but as suggested by #29140, I have implemented an algorithm for updating the dominator tree of a CFG dynamically. I hear that this would be useful to have anyway. Just to be safe, I’ve also prevented a phi node from being replaced with its value if that value is defined after the phi node is.

The dynamic domtree algorithm basically works by figuring out if we can get away with doing nothing or recomputing only some of the immediate dominators when we add or delete an edge in the CFG, or if we have to recompute the whole domtree from scratch.

As this PR stands, domtrees are not updated dynamically yet, even though the dynamic algorithm has been implemented. First, I tried just computing the domtree from scratch when we decide which blocks are dead, which happens when an IncrementalCompact is created. I tried to get an idea of the performance impact by comparing how long Base and Stdlibs took to compile, but the compile times were pretty noisy and I couldn’t confidently notice any difference. Next, I tried adding a DomTree to every CFG and computing it every time a CFG is constructed. This appears to lengthen compile times by about 5% compared with master, and 3% compared with master with DCE on. The only times a new CFG is made from an old one are when we finish iterating over an IncrementalCompact, and in domsort_ssa!. Presumably we would be able to recover some performance by updating the domtree dynamically in these places.

I’ve also addressed a bug wherein references to dead blocks that have been removed remain in phi nodes.

yhls · 2019-10-31T17:45:47Z

Hmm, it seems to take forever on test/compiler/irpasses.jl, will investigate.

yhls · 2019-11-01T17:27:46Z

Fixed. Turns out that was because I was verifying the domtree against output from the naive immediate dominator algorithm, which takes way too long on a big test case in test/compiler/irpasses.jl.

base/compiler/ssair/ir.jl

Keno · 2019-11-03T01:34:50Z

Amazing work (seriously, I'm quite impressed). However, before getting into the implementation, I think we have a more high-level problem: I don't think eliminating dead blocks in the IncrementalCompact constructor is enough. The issue that caused the original problem is that removing edges can turn instructions in unreachable blocks invalid and if the pass then goes to process them we can get into a bad state. Of course the dynamic domtree path is designed to address that, so we're almost there with this PR, but I don't think we can turn it on by default until we actually update it in IncrementalCompact whenever we remove an edge and kill all invalid blocks right then (or at least remove all contained instructions). Am I missing something?

yhls · 2019-11-05T18:41:48Z

Removing dead blocks in the constructor is actually enough to fix #29107, because the particular thing that was going wrong (phi nodes being replaced if they only had one possible value, which can be bad in dead loops) was only ever done while iterating through an IncrementalCompact, while it looks like edges are currently only ever removed outside of them, during inlining. But I agree that there could potentially be a case where something goes wrong if we remove an edge while iterating through an IncrementalCompact (although the kill_edge! for IncrementalCompact objects is currently unused).

According to #29107, invalid IR in dead code is explicitly allowed (I think this is also true for LLVM). Do we want to keep this convention? If so, once we really do update the domtree dynamically, passes will be able to check if a block is dead or not instantaneously. Or do we actually want never to have any dead code at all?

yhls · 2019-11-05T20:33:21Z

Wait, the kill_edge! for IncrementalCompact objects is used...

yhls · 2019-12-17T17:08:33Z

Ok, domtrees are updated dynamically now, and I fixed the bugs that cropped up when I turned DCE back on. Here's a summary of changes in this PR:

DCE is turned back on (reverting Disable CFG transforms for now #29265), and all unreachable blocks are removed (not just ones with no predecessors), as determined when IncrementalCompacts are constructed
if kill_edge! makes blocks unreachable, all their statements are now killed immediately (set to nothing or ReturnNode())--this should prevent passes from seeing invalid statements in dead blocks
phi node values no longer get forwarded to their uses in the case that there's only one incoming edge, if this value is coming from after the phi node (this fixes Compiler bug uncovered by BaseBenchmarks #29107, although is just to be safe because removing all unreachable blocks also fixes it)
fixed Regression with newest Julia? #29253, an infinite loop in kill_edge!, block merging cycles in cfg_simplify!, missing used_ssas counts, and removed blocks being referenced in phi nodes

Some changes to the code:

every CFG now contains a DomTree, which must be kept up to date
DomTrees are no longer explicitly passed to functions like domsort_ssa!
construct_domtree now takes Vector{BasicBlock} instead of CFG
DFSTrees now keep track of postorder numbers as well as preorder numbers, and are stored in every DomTree

Anything that modifies the CFG will have to keep the domtree up to date as well. kill_edge!(IncrementalCompact, ...) does this already, but otherwise, cfg_insert_edge!, cfg_delete_edge!, domtree_insert_edge!, domtree_delete_edge!, rename_blocks!, and combine_blocks! can be used. If many edge insertions or deletions are being done, it may be faster (and simpler) just to reconstruct the domtree from scratch, because the algorithm resorts to reconstructing the whole domtree if the change affects the preorder number of the blocks. (This is why the domtree is recomputed from scratch in domsort_ssa! and batch_inline.)

yhls · 2019-12-17T17:18:32Z

Here's a rough measurement of impact on performance: in one run, master took a total of 67.2 seconds to compile the sysimage, and with this PR, 70.1 seconds (104%). Note that master with the modification that allow_cfg_transforms=true took 68.7 seconds (102%).

StefanKarpinski · 2019-12-17T19:31:34Z

Perhaps it’s obvious or stated somewhere I’m missing, but what is the benefit of this change?

ViralBShah · 2019-12-17T20:12:56Z

As @vchuravy explained on slack: Cleaner IR, better readability, and better for tools like XLA.jl that operate on typed IR

yhls · 2019-12-17T21:16:54Z

Just edited some commits to avoid changing some structs into mutable structs.

vchuravy

LGTM from my side, although I would like @Keno to sign off as well, since he has a deeper understanding
of that part of the code.

vchuravy · 2020-01-06T16:51:04Z

base/compiler/ssair/domtree.jl

+length(D::DFSTree) = length(D.from_pre)
+
+function DFS!(D::DFSTree, blocks::Vector{BasicBlock})
+    copy!(D, DFSTree(length(blocks)))


Is this empty!/resize!?

It would be a resize! if it weren't for the fact that the to_pre, to_post, and to_parent_pre fields need to be reset to all zeros.

Phi nodes are optimized away when there is only one predecessor, but this can cause problems in dead loops because forward references can be created, leading to issues with optimization passes that look at all code, dead or not. This fixes issue JuliaLang#29107 when DCE is turned on.

yhls · 2020-08-03T06:04:55Z

Rebased onto master, got things working with DCE turned on again with a small bugfix for #36684

…ng phi nodes

…ot counted This fixes JuliaLang#29253, which was caused by `simple_dce!` erroneously erasing SSA values that did not appear to be used, because these uses were only discovered in `just_fixup!` at the end of iterating over an `IncrementalCompact`.

PR JuliaLang#36684 changes `iterate(IncrementalCompact)` to return an extra index, but leaves its arguments unchanged. However, the PR decremented the index argument in a particular recursive call to `iterate`. This caused `iterate` not to recognise that it was done when `allow_cfg_transforms` was turned on.

…ons and deletions The DFS tree associated with a CFG now keeps track of postorder as well as preorder numbers. The DFS tree, as well as the state associated with the SNCA algorithm for finding (immediate) dominators is now stored in DomTree and reused for Dynamic SNCA.

… predecessors For now, just construct the domtree when we make an `IncrementalCompact` rather than try to update it (the domtree) incrementally.

…of CFG This is in anticipation of domtrees being added to CFGs.

…n needed Every time a CFG is created, its corresponding dominator tree is as well.

…namic domtree implementation

… edges This change only affects statements that we have yet to encounter after killing an edge, while iterating through `IncrementalCompact`. Statements in dead blocks that come before the point at which the edge is killed are killed in `kill_edge!`, when the edge is killed.

If a statement was `nothing`, `kill_edge!` would never move on from trying to kill it because the index wasn't incremented.

This is so we can add type declarations to fields in ir.jl that are domtrees, by breaking the dependency loop between domtree.jl (uses basic blocks but defines domtrees) and ir.jl (uses domtrees but defined basic blocks).

yhls · 2020-08-03T08:24:34Z

I've separated the freestanding bugfixes into #36888

vchuravy · 2020-08-21T17:12:20Z

@Keno is this ok to be merged with 6a250c4 removed?

vchuravy · 2020-10-05T00:14:35Z

Replaced by #37882

yhls force-pushed the yhls/dynamicdomtree branch from 7a9608c to 1fd4d72 Compare October 31, 2019 16:51

vchuravy requested a review from Keno October 31, 2019 16:59

vchuravy added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Oct 31, 2019

yhls force-pushed the yhls/dynamicdomtree branch from 1fd4d72 to c9f383d Compare October 31, 2019 17:27

yhls force-pushed the yhls/dynamicdomtree branch from c9f383d to cd53d28 Compare November 1, 2019 17:24

Keno reviewed Nov 3, 2019

View reviewed changes

base/compiler/ssair/ir.jl Outdated Show resolved Hide resolved

yhls force-pushed the yhls/dynamicdomtree branch 2 times, most recently from 7e4f0e0 to 3920b30 Compare November 25, 2019 20:54

yhls force-pushed the yhls/dynamicdomtree branch from 5479a4f to b3f91d6 Compare December 15, 2019 03:53

vchuravy requested a review from Keno December 16, 2019 21:20

vchuravy marked this pull request as ready for review December 17, 2019 19:27

vchuravy changed the title ~~WIP/RFC: updating domtrees dynamically, removing all unreachable blocks~~ updating domtrees dynamically, removing all unreachable blocks Dec 17, 2019

yhls force-pushed the yhls/dynamicdomtree branch from b3f91d6 to a8913b5 Compare December 17, 2019 21:11

yhls force-pushed the yhls/dynamicdomtree branch from a8913b5 to 5a1114c Compare December 17, 2019 21:20

vchuravy approved these changes Jan 6, 2020

View reviewed changes

yhls force-pushed the yhls/dynamicdomtree branch 2 times, most recently from b677df8 to b63d3f7 Compare March 11, 2020 18:33

yhls force-pushed the yhls/dynamicdomtree branch from b63d3f7 to a978294 Compare April 1, 2020 17:42

vchuravy assigned Keno Jul 28, 2020

vchuravy mentioned this pull request Jul 28, 2020

add LoopInfo + LICM #36832

Open

3 tasks

yhls added 2 commits August 2, 2020 22:01

Remove edges from dead blocks from phi nodes

714e28c

yhls force-pushed the yhls/dynamicdomtree branch from a978294 to f638a1a Compare August 3, 2020 06:02

yhls added 14 commits August 3, 2020 00:51

Fix possible merge cycle bug in cfg_simplify!

1c9d498

Fix bug in kill_edge! which caused edges to be removed from the wro…

32fbdce

…ng phi nodes

Remove dead blocks as determined by reachability instead of number of…

c73f19e

… predecessors For now, just construct the domtree when we make an `IncrementalCompact` rather than try to update it (the domtree) incrementally.

Make domtree construction take Vector{BasicBlock} as input instead …

12b34fe

…of CFG This is in anticipation of domtrees being added to CFGs.

Add a domtree to every CFG, avoiding constructing them explicitly whe…

88534d5

…n needed Every time a CFG is created, its corresponding dominator tree is as well.

Update domtrees dynamically when CFGs get modified, and fix bug in dy…

07f5c08

…namic domtree implementation

Update domtree dynamically in cfg_simplify

e2e00b0

Fix infinite loop bug in kill_edge!

0b3c63c

If a statement was `nothing`, `kill_edge!` would never move on from trying to kill it because the index wasn't incremented.

Move BasicBlock to separate source file

ff20764

This is so we can add type declarations to fields in ir.jl that are domtrees, by breaking the dependency loop between domtree.jl (uses basic blocks but defines domtrees) and ir.jl (uses domtrees but defined basic blocks).

Enable CFG transforms; turn DCE back on globally

6a250c4

yhls mentioned this pull request Aug 3, 2020

various DCE-related bugfixes #36888

Closed

yhls force-pushed the yhls/dynamicdomtree branch from f638a1a to 6a250c4 Compare August 3, 2020 08:22

vchuravy mentioned this pull request Oct 5, 2020

updating domtrees dynamically, removing all unreachable blocks #37882

Draft

vchuravy closed this Oct 5, 2020

vchuravy mentioned this pull request Oct 5, 2020

Implement dynamic SNCA domtree algorithm #37884

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updating domtrees dynamically, removing all unreachable blocks #33730

updating domtrees dynamically, removing all unreachable blocks #33730

yhls commented Oct 31, 2019 •

edited

Loading

yhls commented Oct 31, 2019

yhls commented Nov 1, 2019

Keno commented Nov 3, 2019

yhls commented Nov 5, 2019

yhls commented Nov 5, 2019

yhls commented Dec 17, 2019 •

edited

Loading

yhls commented Dec 17, 2019

StefanKarpinski commented Dec 17, 2019

ViralBShah commented Dec 17, 2019

yhls commented Dec 17, 2019

vchuravy left a comment

vchuravy Jan 6, 2020

yhls Jan 6, 2020 •

edited

Loading

yhls commented Aug 3, 2020

yhls commented Aug 3, 2020

vchuravy commented Aug 21, 2020

vchuravy commented Oct 5, 2020

updating domtrees dynamically, removing all unreachable blocks #33730

updating domtrees dynamically, removing all unreachable blocks #33730

Conversation

yhls commented Oct 31, 2019 • edited Loading

yhls commented Oct 31, 2019

yhls commented Nov 1, 2019

Keno commented Nov 3, 2019

yhls commented Nov 5, 2019

yhls commented Nov 5, 2019

yhls commented Dec 17, 2019 • edited Loading

yhls commented Dec 17, 2019

StefanKarpinski commented Dec 17, 2019

ViralBShah commented Dec 17, 2019

yhls commented Dec 17, 2019

vchuravy left a comment

Choose a reason for hiding this comment

vchuravy Jan 6, 2020

Choose a reason for hiding this comment

yhls Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

yhls commented Aug 3, 2020

yhls commented Aug 3, 2020

vchuravy commented Aug 21, 2020

vchuravy commented Oct 5, 2020

yhls commented Oct 31, 2019 •

edited

Loading

yhls commented Dec 17, 2019 •

edited

Loading

yhls Jan 6, 2020 •

edited

Loading