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 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.

Thanks,
Richard.

> 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
>>