From 852ea80a318d106acc4b36cc4b1ce1b55db17d0a Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Sat, 2 Jan 2021 18:44:13 -0800 Subject: [PATCH] [SSA] Fix bug in to_ssa.py Fixes Issue #108. The optimization in prune_phis() is not sound. phi nodes that are "partially undefined" cannot be removed. It is only illegal to read from the result if the second to last executed label corresponds to an undefined argument. `to_ssa.py` generated incorrect code for the two tests added, before the fix. In most cases, these phi nodes aren't read from, and will be removed by DCE. However, in the case where the phi nodes are read, a correct optimization would be: 1. Detect partially undefined phi nodes whose value is always used. Let `undefined_labels` be the set of labels whose argument is undefined. 2. Leverage the undefined behavior to say that `undefined_labels` are in fact not predecessors to the basic block. `preds = preds - undefined_labels`. Though, I'm not sure how useful that optimization would be in practice. --- examples/test/to_ssa/if-const.bril | 14 ++++++++++ examples/test/to_ssa/if-const.out | 16 ++++++++++++ examples/test/to_ssa/if-ssa.bril | 16 ++++++++++++ examples/test/to_ssa/if-ssa.out | 17 +++++++++++++ examples/test/to_ssa/loop.out | 2 ++ examples/test/to_ssa/selfloop.out | 1 + examples/test/to_ssa/while.out | 3 +++ examples/to_ssa.py | 41 +++--------------------------- 8 files changed, 72 insertions(+), 38 deletions(-) create mode 100644 examples/test/to_ssa/if-const.bril create mode 100644 examples/test/to_ssa/if-const.out create mode 100644 examples/test/to_ssa/if-ssa.bril create mode 100644 examples/test/to_ssa/if-ssa.out diff --git a/examples/test/to_ssa/if-const.bril b/examples/test/to_ssa/if-const.bril new file mode 100644 index 000000000..7082cf83b --- /dev/null +++ b/examples/test/to_ssa/if-const.bril @@ -0,0 +1,14 @@ +@main() { + cond: bool = const true; + br cond .true .false; +.true: + a: int = const 0; + jmp .zexit; +.false: + b: int = const 1; + jmp .zexit; +# zexit to trigger a bug in to_ssa.py that depends on +# the order that basic blocks get renamed. +.zexit: + print a; +} \ No newline at end of file diff --git a/examples/test/to_ssa/if-const.out b/examples/test/to_ssa/if-const.out new file mode 100644 index 000000000..84b19da84 --- /dev/null +++ b/examples/test/to_ssa/if-const.out @@ -0,0 +1,16 @@ +@main { +.b1: + cond.0: bool = const true; + br cond.0 .true .false; +.true: + a.0: int = const 0; + jmp .zexit; +.false: + b.0: int = const 1; + jmp .zexit; +.zexit: + b.1: int = phi b.0 b.0 .false .true; + a.1: int = phi __undefined a.0 .false .true; + print a.1; + ret; +} diff --git a/examples/test/to_ssa/if-ssa.bril b/examples/test/to_ssa/if-ssa.bril new file mode 100644 index 000000000..75e4b8490 --- /dev/null +++ b/examples/test/to_ssa/if-ssa.bril @@ -0,0 +1,16 @@ +@main(cond: bool) { +.entry: + a.1: int = const 47; + br cond .left .right; +.left: + a.2: int = add a.1 a.1; + jmp .zexit; +.right: + a.3: int = mul a.1 a.1; + jmp .zexit; +# zexit to trigger a bug in to_ssa.py that depends on +# the order that basic blocks get renamed. +.zexit: + a.4: int = phi .left a.2 .right a.3; + print a.4; +} \ No newline at end of file diff --git a/examples/test/to_ssa/if-ssa.out b/examples/test/to_ssa/if-ssa.out new file mode 100644 index 000000000..c9d635ace --- /dev/null +++ b/examples/test/to_ssa/if-ssa.out @@ -0,0 +1,17 @@ +@main(cond: bool) { +.entry: + a.1.0: int = const 47; + br cond .left .right; +.left: + a.2.0: int = add a.1.0 a.1.0; + jmp .zexit; +.right: + a.3.0: int = mul a.1.0 a.1.0; + jmp .zexit; +.zexit: + a.3.1: int = phi __undefined a.3.0 .left .right; + a.2.1: int = phi a.2.0 a.2.0 .left .right; + a.4.0: int = phi a.2.1 a.3.1 .left .right; + print a.4.0; + ret; +} diff --git a/examples/test/to_ssa/loop.out b/examples/test/to_ssa/loop.out index 2d1ee29a7..9241158ef 100644 --- a/examples/test/to_ssa/loop.out +++ b/examples/test/to_ssa/loop.out @@ -3,7 +3,9 @@ i.0: int = const 1; jmp .loop; .loop: + max.0: int = phi __undefined max.1 .entry .body; i.1: int = phi i.0 i.2 .entry .body; + cond.0: bool = phi __undefined cond.1 .entry .body; max.1: int = const 10; cond.1: bool = lt i.1 max.1; br cond.1 .body .exit; diff --git a/examples/test/to_ssa/selfloop.out b/examples/test/to_ssa/selfloop.out index 8cdba15f5..597afe210 100644 --- a/examples/test/to_ssa/selfloop.out +++ b/examples/test/to_ssa/selfloop.out @@ -6,6 +6,7 @@ jmp .loop; .loop: x.1: int = phi x.0 x.2 .entry .br; + done.0: bool = phi __undefined done.1 .entry .br; x.2: int = sub x.1 one.0; done.1: bool = eq x.2 zero.0; jmp .br; diff --git a/examples/test/to_ssa/while.out b/examples/test/to_ssa/while.out index 0d1ff7e38..cf4c69971 100644 --- a/examples/test/to_ssa/while.out +++ b/examples/test/to_ssa/while.out @@ -2,6 +2,9 @@ .entry1: jmp .while.cond; .while.cond: + zero.0: int = phi __undefined zero.1 .entry1 .while.body; + one.0: int = phi __undefined one.1 .entry1 .while.body; + is_term.0: bool = phi __undefined is_term.1 .entry1 .while.body; a.0: int = phi a a.1 .entry1 .while.body; zero.1: int = const 0; is_term.1: bool = eq a.0 zero.1; diff --git a/examples/to_ssa.py b/examples/to_ssa.py index 9bae229de..093c96cb3 100644 --- a/examples/to_ssa.py +++ b/examples/to_ssa.py @@ -74,6 +74,9 @@ def _rename(block): for p in phis[s]: if stack[p]: phi_args[s][p].append((block, stack[p][0])) + else: + # The variable is not defined on this path + phi_args[s][p].append((block, "__undefined")) # Recursive calls. for b in sorted(domtree[block]): @@ -88,43 +91,6 @@ def _rename(block): return phi_args, phi_dests -def prune_phis(pred, phi_args, phi_dests): - """Prune possibly-undefined phi-nodes. - - Ordinary phi insertion will create phi-nodes that are "partially - undefined" because they represent a convergence of paths where the - variable is defined along some but not all paths. These phi-nodes - are useless because it is illegal to read from the result. And they - can confuse the out-of-SSA pass because it creates nonsensical - copies. This algorithm iteratively eliminates such phi-nodes, - propagating through to eliminate consumer phi-nodes until - convergence. - """ - # We build up a set of new names (phi destinations) to prune, and we - # iterate until this set stops growing. - old_prune_len = -1 - prune = set() - while len(prune) != old_prune_len: - old_prune_len = len(prune) - - # Look at each phi. - for block, args in phi_args.items(): - dests = phi_dests[block] - for v, v_args in args.items(): - # How many non-pruned arguments does this phi have? - live_args = [a for _, a in v_args if a not in prune] - if len(live_args) < len(pred[block]): - # Prune phis with insufficient arguments. - prune.add(dests[v]) - - # Actually delete all phis with pruned destinations. - for block, args in phi_args.items(): - dests = phi_dests[block] - to_del = {v for v, d in dests.items() if d in prune} - for v in to_del: - del args[v] - del dests[v] - def insert_phis(blocks, phi_args, phi_dests, types): for block, instrs in blocks.items(): @@ -166,7 +132,6 @@ def func_to_ssa(func): phis = get_phis(blocks, df, defs) phi_args, phi_dests = ssa_rename(blocks, phis, succ, dom_tree(dom), arg_names) - prune_phis(pred, phi_args, phi_dests) insert_phis(blocks, phi_args, phi_dests, types) func['instrs'] = reassemble(blocks)