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

[David Malcolm]

On Sat, 2014-11-08 at 13:07 +0100, Richard Biener wrote:
> On Fri, Nov 7, 2014 at 10:01 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
> > 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.
> 
> To be constructive here - the above case is from within a
> GIMPLE_ASSIGN case label
> and thus I'd have expected
> 
>     case GIMPLE_ASSIGN:
>       {
>         gassign *a1 = as_a <gassign *> (s1);
>         gassign *a2 = as_a <gassign *> (s2);
>       lhs1 = gimple_assign_lhs (a1);
>       lhs2 = gimple_assign_lhs (a2);
>       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 (a1),
>                                                  gimple_assign_rhs1 (a2)));
>       else if (TREE_CODE (lhs1) == SSA_NAME
>                && TREE_CODE (lhs2) == SSA_NAME)
>         return vn_valueize (lhs1) == vn_valueize (lhs2);
>       return false;
>       }
> 
> instead.  That's the kind of changes I have expected and have approved of.

I do make the above kind of change in some places within the
gimple-classes branch.

I think I didn't do it in this case because the body of the "case
GIMPLE_ASSIGN" doesn't yet have braces, so adding locals requires adding
them and re-indenting the case body.  I didn't spot the opportunity to
speed up the code as you do above by converting the two gimple_get_lhs
to gimple_assign_lhs.  Without that, I guess I decided to simply add the
two as_a<> directly in-place to avoid the reindent.   With your speedup
it's clearly better to reindent the code.


(Got to go now, sorry; I hope to write a better reply on Monday)

Thanks
Dave