| Message ID | alpine.LSU.2.20.1709071041050.14191@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
| Series | Enhance PHI processing in VN | expand |
On Thu, 7 Sep 2017, Richard Biener wrote: > > This enhances VN to do the same PHI handling as CCP, meeting > undefined and constant to constant. I've gone a little bit > further (and maybe will revisit this again) in also meeting > all-undefined to undefined taking one of the undefined args > as the value number. I feel like this might run into > the equation issues I mentioned in the other mail so it > would be cleaner to invent a "new" undefined value number > here -- but I have to make sure to not create too many > default-defs or break iteration convergence (default defs are also > expensive given they require a decl - sth I want to change as well). > > So for now I guess I'll stick with the slightly bogus(?) way also > hoping for a testcase that shows the issue this uncovers. > > Note it's required to handle > > _3 = PHI <_1(D), _2(D)> > .. > > _4 = PHI <_3, 1> > > consistently with > > _4 = PHI <_1(D), _2(D), 1> > > aka with/without extra forwarders. That said, "fallout" is we simplify int foo (int b) { int i, j, k; if (b) k = i; else k = j; if (k == i) return 1; else if (k == j) return 2; return 0; to if (j == i) return 1; else return 2; or even just return 2; dependent on PHI argument order of k = PHI <i(D), j(D)>. Likewise we'd say that either k - i or k - j is zero. The complication with PHIs is that they do not always only appear in places where uses of the args dominate it but the other way around so we can't really invoke the undefined behavior rule on a PHI node with undefined args itself. The question is whether we may for PHIs with just undefined args ... but my guess is no so I do have to fix the above. Anybody can produce a testcase that we'd consider wrong-code? (the above examples clearly are not) Thanks, Richard. > Similar to CCP/copyprop this doesn't allow copies to be propagated > this way as this removes most gcc.dg/Wuninitialized* warnings > we test for... > > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress. > > Richard. > > 2017-09-07 Richard Biener <rguenther@suse.de> > > * tree-ssa-sccnv.c (visit_phi): Handle meeting undefined > values. > > * gcc.dg/tree-ssa/ssa-fre-59.c: New testcase. > > Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c > =================================================================== > --- gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c (nonexistent) > +++ gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c (working copy) > @@ -0,0 +1,14 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O -fdump-tree-fre1" } */ > + > +int i; > +int foo (int b) > +{ > + int j; > + i = 1; > + if (b) > + j = i; > + return i - j; > +} > + > +/* { dg-final { scan-tree-dump "return 0;" "fre1" } } */ > Index: gcc/tree-ssa-sccvn.c > =================================================================== > --- gcc/tree-ssa-sccvn.c (revision 251833) > +++ gcc/tree-ssa-sccvn.c (working copy) > @@ -3864,6 +3864,7 @@ visit_phi (gimple *phi) > tree result; > tree sameval = VN_TOP; > bool allsame = true; > + tree seen_undef = NULL_TREE; > unsigned n_executable = 0; > > /* TODO: We could check for this in init_sccvn, and replace this > @@ -3884,8 +3885,12 @@ visit_phi (gimple *phi) > if (TREE_CODE (def) == SSA_NAME) > def = SSA_VAL (def); > if (def == VN_TOP) > - continue; > - if (sameval == VN_TOP) > + ; > + /* Ignore undefined defs here. */ > + else if (TREE_CODE (def) == SSA_NAME > + && ssa_undefined_value_p (def, false)) > + seen_undef = def; > + else if (sameval == VN_TOP) > sameval = def; > else if (!expressions_equal_p (def, sameval)) > { > @@ -3893,7 +3898,18 @@ visit_phi (gimple *phi) > break; > } > } > - > + /* If we found undefined values and didn't end up with a constant > + drop back to varying as otherwise we end up removing most late > + uninitialized use warnings. CCP/copyprop have the same restriction. */ > + if (allsame && seen_undef) > + { > + /* Unless this was the only value we've seen. */ > + if (sameval == VN_TOP) > + sameval = seen_undef; > + else if (! is_gimple_min_invariant (sameval)) > + allsame = false; > + } > + > /* If none of the edges was executable or all incoming values are > undefined keep the value-number at VN_TOP. If only a single edge > is exectuable use its value. */ >
On Thu, 7 Sep 2017, Richard Biener wrote: > On Thu, 7 Sep 2017, Richard Biener wrote: > > > > > This enhances VN to do the same PHI handling as CCP, meeting > > undefined and constant to constant. I've gone a little bit > > further (and maybe will revisit this again) in also meeting > > all-undefined to undefined taking one of the undefined args > > as the value number. I feel like this might run into > > the equation issues I mentioned in the other mail so it > > would be cleaner to invent a "new" undefined value number > > here -- but I have to make sure to not create too many > > default-defs or break iteration convergence (default defs are also > > expensive given they require a decl - sth I want to change as well). > > > > So for now I guess I'll stick with the slightly bogus(?) way also > > hoping for a testcase that shows the issue this uncovers. > > > > Note it's required to handle > > > > _3 = PHI <_1(D), _2(D)> > > .. > > > > _4 = PHI <_3, 1> > > > > consistently with > > > > _4 = PHI <_1(D), _2(D), 1> > > > > aka with/without extra forwarders. > > That said, "fallout" is we simplify > > int foo (int b) > { > int i, j, k; > if (b) > k = i; > else > k = j; > if (k == i) > return 1; > else if (k == j) > return 2; > return 0; > > to > > if (j == i) > return 1; > else > return 2; > > or even just > > return 2; > > dependent on PHI argument order of k = PHI <i(D), j(D)>. > > Likewise we'd say that either k - i or k - j is zero. > > The complication with PHIs is that they do not always only appear > in places where uses of the args dominate it but the other way > around so we can't really invoke the undefined behavior rule > on a PHI node with undefined args itself. The question is whether > we may for PHIs with just undefined args ... but my guess is no > so I do have to fix the above. > > Anybody can produce a testcase that we'd consider wrong-code? > (the above examples clearly are not) After some pondering I decided for PHIs with all undefs there's really no way things can go wrong. Thus the following is what I installed. Bootstrapped and tested on x86_64-unknown-linux-gnu. Richard. 2017-09-14 Richard Biener <rguenther@suse.de> * tree-ssa-sccvn.c (visit_phi): Merge undefined values similar to VN_TOP. * gcc.dg/tree-ssa/ssa-fre-59.c: New testcase. * gcc.dg/uninit-suppress_2.c: Adjust. * gcc.dg/tree-ssa/ssa-sccvn-2.c: Likewise. Index: gcc/tree-ssa-sccvn.c =================================================================== --- gcc/tree-ssa-sccvn.c (revision 252062) +++ gcc/tree-ssa-sccvn.c (working copy) @@ -3860,11 +3860,11 @@ visit_reference_op_store (tree lhs, tree static bool visit_phi (gimple *phi) { - bool changed = false; - tree result; - tree sameval = VN_TOP; - bool allsame = true; + tree result, sameval = VN_TOP, seen_undef = NULL_TREE; unsigned n_executable = 0; + bool allsame = true; + edge_iterator ei; + edge e; /* TODO: We could check for this in init_sccvn, and replace this with a gcc_assert. */ @@ -3873,8 +3873,6 @@ visit_phi (gimple *phi) /* See if all non-TOP arguments have the same value. TOP is equivalent to everything, so we can ignore it. */ - edge_iterator ei; - edge e; FOR_EACH_EDGE (e, ei, gimple_bb (phi)->preds) if (e->flags & EDGE_EXECUTABLE) { @@ -3884,8 +3882,12 @@ visit_phi (gimple *phi) if (TREE_CODE (def) == SSA_NAME) def = SSA_VAL (def); if (def == VN_TOP) - continue; - if (sameval == VN_TOP) + ; + /* Ignore undefined defs for sameval but record one. */ + else if (TREE_CODE (def) == SSA_NAME + && ssa_undefined_value_p (def, false)) + seen_undef = def; + else if (sameval == VN_TOP) sameval = def; else if (!expressions_equal_p (def, sameval)) { @@ -3893,30 +3895,39 @@ visit_phi (gimple *phi) break; } } - - /* If none of the edges was executable or all incoming values are - undefined keep the value-number at VN_TOP. If only a single edge - is exectuable use its value. */ - if (sameval == VN_TOP - || n_executable == 1) - return set_ssa_val_to (PHI_RESULT (phi), sameval); + + /* If none of the edges was executable keep the value-number at VN_TOP, + if only a single edge is exectuable use its value. */ + if (n_executable <= 1) + result = seen_undef ? seen_undef : sameval; + /* If we saw only undefined values create a new undef SSA name to + avoid false equivalences. */ + else if (sameval == VN_TOP) + { + gcc_assert (seen_undef); + result = seen_undef; + } /* First see if it is equivalent to a phi node in this block. We prefer this as it allows IV elimination - see PRs 66502 and 67167. */ - result = vn_phi_lookup (phi); - if (result) - changed = set_ssa_val_to (PHI_RESULT (phi), result); - /* Otherwise all value numbered to the same value, the phi node has that - value. */ - else if (allsame) - changed = set_ssa_val_to (PHI_RESULT (phi), sameval); + else if ((result = vn_phi_lookup (phi))) + ; + /* If all values are the same use that, unless we've seen undefined + values as well and the value isn't constant. + CCP/copyprop have the same restriction to not remove uninit warnings. */ + else if (allsame + && (! seen_undef || is_gimple_min_invariant (sameval))) + result = sameval; else { - vn_phi_insert (phi, PHI_RESULT (phi)); - changed = set_ssa_val_to (PHI_RESULT (phi), PHI_RESULT (phi)); + result = PHI_RESULT (phi); + /* Only insert PHIs that are varying, for constant value numbers + we mess up equivalences otherwise as we are only comparing + the immediate controlling predicates. */ + vn_phi_insert (phi, result); } - return changed; + return set_ssa_val_to (PHI_RESULT (phi), result); } /* Try to simplify RHS using equivalences and constant folding. */ Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c (nonexistent) +++ gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c (working copy) @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-O -fdump-tree-fre1" } */ + +int i; +int foo (int b) +{ + int j; + i = 1; + if (b) + j = i; + return i - j; +} + +/* { dg-final { scan-tree-dump "return 0;" "fre1" } } */ Index: gcc/testsuite/gcc.dg/uninit-suppress_2.c =================================================================== --- gcc/testsuite/gcc.dg/uninit-suppress_2.c (revision 252062) +++ gcc/testsuite/gcc.dg/uninit-suppress_2.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-fno-tree-ccp -fno-tree-vrp -O2 -Wuninitialized -Werror=uninitialized -Wno-error=maybe-uninitialized" } */ +/* { dg-options "-fno-tree-ccp -fno-tree-vrp -fno-tree-fre -fno-tree-pre -fno-code-hoisting -O2 -Wuninitialized -Werror=uninitialized -Wno-error=maybe-uninitialized" } */ void blah(); void bar (int); int gflag; Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-sccvn-2.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/ssa-sccvn-2.c (revision 252062) +++ gcc/testsuite/gcc.dg/tree-ssa/ssa-sccvn-2.c (working copy) @@ -21,4 +21,4 @@ int vnum_test8(int *data) } /* We should eliminate m - n, and set n = n + k into n = m, and set p to 0 */ -/* { dg-final { scan-tree-dump-times "Eliminated: 4" 1 "fre1"} } */ +/* { dg-final { scan-tree-dump-times "Eliminated: 5" 1 "fre1"} } */
Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c (nonexistent) +++ gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c (working copy) @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-O -fdump-tree-fre1" } */ + +int i; +int foo (int b) +{ + int j; + i = 1; + if (b) + j = i; + return i - j; +} + +/* { dg-final { scan-tree-dump "return 0;" "fre1" } } */ Index: gcc/tree-ssa-sccvn.c =================================================================== --- gcc/tree-ssa-sccvn.c (revision 251833) +++ gcc/tree-ssa-sccvn.c (working copy) @@ -3864,6 +3864,7 @@ visit_phi (gimple *phi) tree result; tree sameval = VN_TOP; bool allsame = true; + tree seen_undef = NULL_TREE; unsigned n_executable = 0; /* TODO: We could check for this in init_sccvn, and replace this @@ -3884,8 +3885,12 @@ visit_phi (gimple *phi) if (TREE_CODE (def) == SSA_NAME) def = SSA_VAL (def); if (def == VN_TOP) - continue; - if (sameval == VN_TOP) + ; + /* Ignore undefined defs here. */ + else if (TREE_CODE (def) == SSA_NAME + && ssa_undefined_value_p (def, false)) + seen_undef = def; + else if (sameval == VN_TOP) sameval = def; else if (!expressions_equal_p (def, sameval)) { @@ -3893,7 +3898,18 @@ visit_phi (gimple *phi) break; } } - + /* If we found undefined values and didn't end up with a constant + drop back to varying as otherwise we end up removing most late + uninitialized use warnings. CCP/copyprop have the same restriction. */ + if (allsame && seen_undef) + { + /* Unless this was the only value we've seen. */ + if (sameval == VN_TOP) + sameval = seen_undef; + else if (! is_gimple_min_invariant (sameval)) + allsame = false; + } + /* If none of the edges was executable or all incoming values are undefined keep the value-number at VN_TOP. If only a single edge is exectuable use its value. */