Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign

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

On Fri, Nov 7, 2014 at 4:21 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> gcc/ChangeLog.gimple-classes:
>         * tree-ssa-tail-merge.c (same_succ_hash): Add checked cast.
>         (gimple_equal_p): Add checked casts.
> ---
>  gcc/ChangeLog.gimple-classes | 5 +++++
>  gcc/tree-ssa-tail-merge.c    | 8 +++++---
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/ChangeLog.gimple-classes b/gcc/ChangeLog.gimple-classes
> index f43df63..0bd0421 100644
> --- a/gcc/ChangeLog.gimple-classes
> +++ b/gcc/ChangeLog.gimple-classes
> @@ -1,5 +1,10 @@
>  2014-11-06  David Malcolm  <dmalcolm@redhat.com>
>
> +       * tree-ssa-tail-merge.c (same_succ_hash): Add checked cast.
> +       (gimple_equal_p): Add checked casts.
> +
> +2014-11-06  David Malcolm  <dmalcolm@redhat.com>
> +
>         * tree-ssa-structalias.c (find_func_aliases): Replace
>         is_gimple_assign with a dyn_cast, introducing local gassign *
>         "t_assign", using it in place of "t" for typesafety.
> diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
> index 5678657..b822214 100644
> --- a/gcc/tree-ssa-tail-merge.c
> +++ b/gcc/tree-ssa-tail-merge.c
> @@ -484,7 +484,7 @@ same_succ_hash (const_same_succ e)
>
>        hstate.add_int (gimple_code (stmt));
>        if (is_gimple_assign (stmt))
> -       hstate.add_int (gimple_assign_rhs_code (stmt));
> +       hstate.add_int (gimple_assign_rhs_code (as_a <gassign *> (stmt)));
>        if (!is_gimple_call (stmt))
>         continue;
>        if (gimple_call_internal_p (stmt))
> @@ -1172,8 +1172,10 @@ gimple_equal_p (same_succ same_succ, gimple s1, gimple s2)
>        if (TREE_CODE (lhs1) != SSA_NAME
>           && TREE_CODE (lhs2) != SSA_NAME)
>         return (operand_equal_p (lhs1, lhs2, 0)
> -               && gimple_operand_equal_value_p (gimple_assign_rhs1 (s1),
> -                                                gimple_assign_rhs1 (s2)));
> +               && gimple_operand_equal_value_p (gimple_assign_rhs1 (
> +                                                  as_a <gassign *> (s1)),
> +                                                gimple_assign_rhs1 (
> +                                                  as_a <gassign *> (s2))));

Just a comment as these patches flow by - I think this is a huge step
backwards from "enforcing" s1/s2 being a gimple_assign inside
gimple_assign_rhs1 to this as_a <gassign *> boilerplate at _each_ callsite!

Which means this step of the refactoring is totally broken and probably
requires much more manual work to avoid this kind of uglyness.

I definitely won't approve of this kind of changes.

Thanks,
Richard.

>        else if (TREE_CODE (lhs1) == SSA_NAME
>                && TREE_CODE (lhs2) == SSA_NAME)
>         return vn_valueize (lhs1) == vn_valueize (lhs2);
> --
> 1.7.11.7
>