On Fri, Nov 7, 2014 at 10:01 PM, Richard Biener <richard.guent...@gmail.com> wrote: > On Fri, Nov 7, 2014 at 4:21 PM, David Malcolm <dmalc...@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 <dmalc...@redhat.com> >> >> + * tree-ssa-tail-merge.c (same_succ_hash): Add checked cast. >> + (gimple_equal_p): Add checked casts. >> + >> +2014-11-06 David Malcolm <dmalc...@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 >>