Re: [committed][PATCH] Fix bogus propagation in DOM

[Richard Biener <richard dot guenther at gmail dot com>]

On Sun, Nov 19, 2017 at 9:16 PM, Jeff Law <law@redhat.com> wrote:
> On my local branch gcc.dg/torture/pr56349.c fails by sending GCC into an
> infinite loop trying to simplify a self-referring statement. ie
> something like
>
> x_1 = x_1 + 10;
>
> That, of course, shouldn't be happening in SSA form.  After some digging
> I've found the culprit.
>
> Let's say we've got a PHI.
>
> a_1 = PHI (a_0, a_2)
>
> If DOM decides that the edge associated with a_2 is not executable, then
> DOM will consider the PHI a degenerate and enter a_1 = a_0 into its
> equivalence table.
>
> That in turn will result in propagation of a_0 into uses of a_1.
>
> That, of course, isn't right.  There's nothing that guarantees that the
> definition of a_0 dominates the uses of a_1.  In the testcase that bogus
> propagation cascades and eventually results in a self-referring node
> like I showed above.
>
> The solution here is to note whether or not we ignored any PHI
> arguments.  If we do and the equivalence we want to enter is SSA_NAME =
> SSA_NAME, then we must reject the equivalence.    Obviously if we wanted
> to enter SSA_NAME = CONST, then we can still do so.
>
> Bootstrapped and regression tested on x86_64.  Installing on the trunk.

Hmm, but if the edge isn't executable then it will be removed and thus the
definition _will_ dominate.  So I think the error happens elsewhere.  With
the change you remove one of the advantages of tracking unexecutable
edges, namely that we can treat those merges optimistically resulting in
more CSE.

You didn't add a testcase so I can't have a quick look myself.

Short: I think you're papering over an issue elsehwere.

Richard.

> jeff
>
> commit 2b37e05b327e6b1170f72c51c21681b1dc5b8337
> Author: Jeff Law <law@redhat.com>
> Date:   Sun Nov 19 13:14:55 2017 -0700
>
>             * tree-ssa-dom.c (record_equivalences_from_phis): Fix handling
>             of degenerates resulting from ignoring an edge.
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index ae05dacf791..5ce981d0871 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,8 @@
> +2017-11-19  Jeff Law  <law@redhat.com>
> +
> +       * tree-ssa-dom.c (record_equivalences_from_phis): Fix handling
> +       of degenerates resulting from ignoring an edge.
> +
>  2017-11-19  Jan Hubicka  <hubicka@ucw.cz>
>
>         PR ipa/81360
> diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
> index eb85b4a09ad..916d66134c3 100644
> --- a/gcc/tree-ssa-dom.c
> +++ b/gcc/tree-ssa-dom.c
> @@ -1011,6 +1011,7 @@ record_equivalences_from_phis (basic_block bb)
>        tree rhs = NULL;
>        size_t i;
>
> +      bool ignored_phi_arg = false;
>        for (i = 0; i < gimple_phi_num_args (phi); i++)
>         {
>           tree t = gimple_phi_arg_def (phi, i);
> @@ -1021,10 +1022,14 @@ record_equivalences_from_phis (basic_block bb)
>           if (lhs == t)
>             continue;
>
> -         /* If the associated edge is not marked as executable, then it
> -            can be ignored.  */
> +         /* We want to track if we ignored any PHI arguments because
> +            their associated edges were not executable.  This impacts
> +            whether or not we can use any equivalence we might discover.  */
>           if ((gimple_phi_arg_edge (phi, i)->flags & EDGE_EXECUTABLE) == 0)
> -           continue;
> +           {
> +             ignored_phi_arg = true;
> +             continue;
> +           }
>
>           t = dom_valueize (t);
>
> @@ -1049,9 +1054,15 @@ record_equivalences_from_phis (basic_block bb)
>          a useful equivalence.  We do not need to record unwind data for
>          this, since this is a true assignment and not an equivalence
>          inferred from a comparison.  All uses of this ssa name are dominated
> -        by this assignment, so unwinding just costs time and space.  */
> +        by this assignment, so unwinding just costs time and space.
> +
> +        Note that if we ignored a PHI argument and the resulting equivalence
> +        is SSA_NAME = SSA_NAME.  Then we can not use the equivalence as the
> +        uses of the LHS SSA_NAME are not necessarily dominated by the
> +        assignment of the RHS SSA_NAME.  */
>        if (i == gimple_phi_num_args (phi)
> -         && may_propagate_copy (lhs, rhs))
> +         && may_propagate_copy (lhs, rhs)
> +         && (!ignored_phi_arg || TREE_CODE (rhs) != SSA_NAME))
>         set_ssa_name_value (lhs, rhs);
>      }
>  }
>