> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63747 is likely caused by > this patch. compare_gimple_switch does not check CASE_LOW and > CASE_HIGH, resulting merging functions not identical. > > Interestingly in the first a few versions of this patch CASE_LOW/HIGH > were checked. But last versions only checks CASE_LABEL. What was the > intention?
It sounds like an accidental change. CASE_LOW/CASE_HIGH needs to be compared ;) Honza > > Thanks, > Joey > > On Thu, Oct 23, 2014 at 5:18 AM, Jiong Wang > <wong.kwongyuan.to...@gmail.com> wrote: > > PR 63574 ICE building libjava (segfault) on arm-linux-gnueabihf is > > caused by this commit. > > > > from the backtrace, the ICF pass is trying to compare two label tree > > node without type info. > > > > while looks like "compare_operand" expect the type info always be not > > empty before invoking "func_checker::compatible_types_p". > > > > I think we should add a similiar t1/t2 check at the start of > > "func_checker::compatible_types_p", just > > like what has been done at the head of "func_checker::compare_operand". > > > > And I verified if we add that check, the crash gone away. > > > > Regards, > > Jiong > > > > > > 2014-10-15 18:03 GMT+01:00 Martin Liška <mli...@suse.cz>: > >> On 10/14/2014 06:04 PM, Jan Hubicka wrote: > >>>> > >>>> diff --git a/gcc/cgraph.h b/gcc/cgraph.h > >>>> index fb41b01..2de98b4 100644 > >>>> --- a/gcc/cgraph.h > >>>> +++ b/gcc/cgraph.h > >>>> @@ -172,6 +172,12 @@ public: > >>>> /* Dump referring in list to FILE. */ > >>>> void dump_referring (FILE *); > >>>> > >>>> + /* Get number of references for this node. */ > >>>> + inline unsigned get_references_count (void) > >>>> + { > >>>> + return ref_list.references ? ref_list.references->length () : 0; > >>>> + } > >>> > >>> > >>> Probably better called num_references() (like we have num_edge in > >>> basic-block.h) > >>>> > >>>> @@ -8068,6 +8069,19 @@ it may significantly increase code size > >>>> (see @option{--param ipcp-unit-growth=@var{value}}). > >>>> This flag is enabled by default at @option{-O3}. > >>>> > >>>> +@item -fipa-icf > >>>> +@opindex fipa-icf > >>>> +Perform Identical Code Folding for functions and read-only variables. > >>>> +The optimization reduces code size and may disturb unwind stacks by > >>>> replacing > >>>> +a function by equivalent one with a different name. The optimization > >>>> works > >>>> +more effectively with link time optimization enabled. > >>>> + > >>>> +Nevertheless the behavior is similar to Gold Linker ICF optimization, > >>>> GCC ICF > >>>> +works on different levels and thus the optimizations are not same - > >>>> there are > >>>> +equivalences that are found only by GCC and equivalences found only by > >>>> Gold. > >>>> + > >>>> +This flag is enabled by default at @option{-O2}. > >>> > >>> ... and -Os? > >>>> > >>>> + case ARRAY_REF: > >>>> + case ARRAY_RANGE_REF: > >>>> + { > >>>> + x1 = TREE_OPERAND (t1, 0); > >>>> + x2 = TREE_OPERAND (t2, 0); > >>>> + y1 = TREE_OPERAND (t1, 1); > >>>> + y2 = TREE_OPERAND (t2, 1); > >>>> + > >>>> + if (!compare_operand (array_ref_low_bound (t1), > >>>> + array_ref_low_bound (t2))) > >>>> + return return_false_with_msg (""); > >>>> + if (!compare_operand (array_ref_element_size (t1), > >>>> + array_ref_element_size (t2))) > >>>> + return return_false_with_msg (""); > >>>> + if (!compare_operand (x1, x2)) > >>>> + return return_false_with_msg (""); > >>>> + return compare_operand (y1, y2); > >>>> + } > >>> > >>> > >>> No need for {...} if there are no local vars. > >>>> > >>>> +bool > >>>> +func_checker::compare_function_decl (tree t1, tree t2) > >>>> +{ > >>>> + bool ret = false; > >>>> + > >>>> + if (t1 == t2) > >>>> + return true; > >>>> + > >>>> + symtab_node *n1 = symtab_node::get (t1); > >>>> + symtab_node *n2 = symtab_node::get (t2); > >>>> + > >>>> + if (m_ignored_source_nodes != NULL && m_ignored_target_nodes != NULL) > >>>> + { > >>>> + ret = m_ignored_source_nodes->contains (n1) > >>>> + && m_ignored_target_nodes->contains (n2); > >>>> + > >>>> + if (ret) > >>>> + return true; > >>>> + } > >>>> + > >>>> + /* If function decl is WEAKREF, we compare targets. */ > >>>> + cgraph_node *f1 = cgraph_node::get (t1); > >>>> + cgraph_node *f2 = cgraph_node::get (t2); > >>>> + > >>>> + if(f1 && f2 && f1->weakref && f2->weakref) > >>>> + ret = f1->alias_target == f2->alias_target; > >>>> + > >>>> + return ret; > >>> > >>> > >>> Comparing aliases is bit more complicated than just handling weakrefs. I > >>> have > >>> patch for symtab_node::equivalent_address_p somewhre in queue. lets just > >>> drop > >>> the fancy stuff for the moment and compare f1&&f2 for equivalence. > >>>> > >>>> + ret = compare_decl (t1, t2); > >>> > >>> > >>> Why functions are not compared with compare_decl while variables are? > >>>> > >>>> + > >>>> + return return_with_debug (ret); > >>>> +} > >>>> + > >>>> +void > >>>> +func_checker::parse_labels (sem_bb *bb) > >>>> +{ > >>>> + for (gimple_stmt_iterator gsi = gsi_start_bb (bb->bb); !gsi_end_p > >>>> (gsi); > >>>> + gsi_next (&gsi)) > >>>> + { > >>>> + gimple stmt = gsi_stmt (gsi); > >>>> + > >>>> + if (gimple_code (stmt) == GIMPLE_LABEL) > >>>> + { > >>>> + tree t = gimple_label_label (stmt); > >>>> + gcc_assert (TREE_CODE (t) == LABEL_DECL); > >>>> + > >>>> + m_label_bb_map.put (t, bb->bb->index); > >>>> + } > >>>> + } > >>>> +} > >>>> + > >>>> +/* Basic block equivalence comparison function that returns true if > >>>> + basic blocks BB1 and BB2 (from functions FUNC1 and FUNC2) correspond. > >>>> + > >>>> + In general, a collection of equivalence dictionaries is built for > >>>> types > >>>> + like SSA names, declarations (VAR_DECL, PARM_DECL, ..). This > >>>> infrastructure > >>>> + is utilized by every statement-by-stament comparison function. */ > >>>> + > >>>> +bool > >>>> +func_checker::compare_bb (sem_bb *bb1, sem_bb *bb2) > >>>> +{ > >>>> + unsigned i; > >>>> + gimple_stmt_iterator gsi1, gsi2; > >>>> + gimple s1, s2; > >>>> + > >>>> + if (bb1->nondbg_stmt_count != bb2->nondbg_stmt_count > >>>> + || bb1->edge_count != bb2->edge_count) > >>>> + return return_false (); > >>>> + > >>>> + gsi1 = gsi_start_bb (bb1->bb); > >>>> + gsi2 = gsi_start_bb (bb2->bb); > >>>> + > >>>> + for (i = 0; i < bb1->nondbg_stmt_count; i++) > >>>> + { > >>>> + if (is_gimple_debug (gsi_stmt (gsi1))) > >>>> + gsi_next_nondebug (&gsi1); > >>>> + > >>>> + if (is_gimple_debug (gsi_stmt (gsi2))) > >>>> + gsi_next_nondebug (&gsi2); > >>>> + > >>>> + s1 = gsi_stmt (gsi1); > >>>> + s2 = gsi_stmt (gsi2); > >>>> + > >>>> + int eh1 = lookup_stmt_eh_lp_fn > >>>> + (DECL_STRUCT_FUNCTION (m_source_func_decl), s1); > >>>> + int eh2 = lookup_stmt_eh_lp_fn > >>>> + (DECL_STRUCT_FUNCTION (m_target_func_decl), s2); > >>>> + > >>>> + if (eh1 != eh2) > >>>> + return return_false_with_msg ("EH regions are different"); > >>>> + > >>>> + if (gimple_code (s1) != gimple_code (s2)) > >>>> + return return_false_with_msg ("gimple codes are different"); > >>>> + > >>>> + switch (gimple_code (s1)) > >>>> + { > >>>> + case GIMPLE_CALL: > >>>> + if (!compare_gimple_call (s1, s2)) > >>>> + return return_different_stmts (s1, s2, "GIMPLE_CALL"); > >>>> + break; > >>>> + case GIMPLE_ASSIGN: > >>>> + if (!compare_gimple_assign (s1, s2)) > >>>> + return return_different_stmts (s1, s2, "GIMPLE_ASSIGN"); > >>>> + break; > >>>> + case GIMPLE_COND: > >>>> + if (!compare_gimple_cond (s1, s2)) > >>>> + return return_different_stmts (s1, s2, "GIMPLE_COND"); > >>>> + break; > >>>> + case GIMPLE_SWITCH: > >>>> + if (!compare_gimple_switch (s1, s2)) > >>>> + return return_different_stmts (s1, s2, "GIMPLE_SWITCH"); > >>>> + break; > >>>> + case GIMPLE_DEBUG: > >>>> + case GIMPLE_EH_DISPATCH: > >>>> + break; > >>>> + case GIMPLE_RESX: > >>>> + if (!compare_gimple_resx (s1, s2)) > >>>> + return return_different_stmts (s1, s2, "GIMPLE_RESX"); > >>>> + break; > >>>> + case GIMPLE_LABEL: > >>>> + if (!compare_gimple_label (s1, s2)) > >>>> + return return_different_stmts (s1, s2, "GIMPLE_LABEL"); > >>>> + break; > >>>> + case GIMPLE_RETURN: > >>>> + if (!compare_gimple_return (s1, s2)) > >>>> + return return_different_stmts (s1, s2, "GIMPLE_RETURN"); > >>>> + break; > >>>> + case GIMPLE_GOTO: > >>>> + if (!compare_gimple_goto (s1, s2)) > >>>> + return return_different_stmts (s1, s2, "GIMPLE_GOTO"); > >>>> + break; > >>>> + case GIMPLE_ASM: > >>>> + if (!compare_gimple_asm (s1, s2)) > >>>> + return return_different_stmts (s1, s2, "GIMPLE_ASM"); > >>>> + break; > >>>> + case GIMPLE_PREDICT: > >>>> + case GIMPLE_NOP: > >>>> + return true; > >>>> + default: > >>>> + return return_false_with_msg ("Unknown GIMPLE code reached"); > >>>> + } > >>>> + > >>>> + gsi_next (&gsi1); > >>>> + gsi_next (&gsi2); > >>>> + } > >>>> + > >>>> + return true; > >>>> +} > >>>> + > >>>> +/* Verifies for given GIMPLEs S1 and S2 that > >>>> + call statements are semantically equivalent. */ > >>>> + > >>>> +bool > >>>> +func_checker::compare_gimple_call (gimple s1, gimple s2) > >>>> +{ > >>>> + unsigned i; > >>>> + tree t1, t2; > >>>> + > >>>> + if (gimple_call_num_args (s1) != gimple_call_num_args (s2)) > >>>> + return false; > >>>> + > >>>> + t1 = gimple_call_fndecl (s1); > >>>> + t2 = gimple_call_fndecl (s2); > >>>> + > >>>> + /* Function pointer variables are not supported yet. */ > >>>> + if (t1 == NULL || t2 == NULL) > >>>> + { > >>>> + if (!compare_operand (t1, t2)) > >>>> + return return_false(); > >>> > >>> > >>> I think the comment above is out of date. compare_operand should do the > >>> right > >>> job for indirect calls. > >>>> > >>>> + > >>>> + if (cn1 && cn2 && cn1->weakref && cn2->weakref > >>>> + && cn1->alias_target == cn2->alias_target) > >>>> + return true; > >>> > >>> > >>> Lets consistently drop the weakrefs handling and add full alias handling > >>> incrementally. > >>>> > >>>> + > >>>> + /* Checking function arguments. */ > >>> > >>> attributes > >>>> > >>>> + tree decl1 = DECL_ATTRIBUTES (decl); > >>>> + tree decl2 = DECL_ATTRIBUTES (m_compared_func->decl); > >>> > >>> > >>> You can still do this as part of the wap_comparison, right? > >>>> > >>>> + > >>>> + m_checker = new func_checker (decl, m_compared_func->decl, > >>>> + compare_polymorphic_p (), > >>>> + false, > >>>> + &refs_set, > >>>> + &m_compared_func->refs_set); > >>>> + while (decl1) > >>>> + { > >>>> + if (decl2 == NULL) > >>>> + return return_false (); > >>>> + > >>>> + if (get_attribute_name (decl1) != get_attribute_name (decl2)) > >>>> + return return_false (); > >>>> + > >>>> + tree attr_value1 = TREE_VALUE (decl1); > >>>> + tree attr_value2 = TREE_VALUE (decl2); > >>>> + > >>>> + if (attr_value1 && attr_value2) > >>>> + { > >>>> + bool ret = m_checker->compare_operand (TREE_VALUE > >>>> (attr_value1), > >>>> + TREE_VALUE > >>>> (attr_value2)); > >>>> + if (!ret) > >>>> + return return_false_with_msg ("attribute values are > >>>> different"); > >>>> + } > >>>> + else if (!attr_value1 && !attr_value2) > >>>> + {} > >>>> + else > >>>> + return return_false (); > >>>> + > >>>> + decl1 = TREE_CHAIN (decl1); > >>>> + decl2 = TREE_CHAIN (decl2); > >>>> + } > >>>> + > >>>> + if (decl1 != decl2) > >>>> + return return_false(); > >>>> + > >>>> + > >>>> + for (arg1 = DECL_ARGUMENTS (decl), > >>>> + arg2 = DECL_ARGUMENTS (m_compared_func->decl); > >>>> + arg1; arg1 = DECL_CHAIN (arg1), arg2 = DECL_CHAIN (arg2)) > >>>> + if (!m_checker->compare_decl (arg1, arg2)) > >>>> + return return_false (); > >>>> + > >>>> + /* Fill-up label dictionary. */ > >>>> + for (unsigned i = 0; i < bb_sorted.length (); ++i) > >>>> + { > >>>> + m_checker->parse_labels (bb_sorted[i]); > >>>> + m_checker->parse_labels (m_compared_func->bb_sorted[i]); > >>>> + } > >>>> + > >>>> + /* Checking all basic blocks. */ > >>>> + for (unsigned i = 0; i < bb_sorted.length (); ++i) > >>>> + if(!m_checker->compare_bb (bb_sorted[i], > >>>> m_compared_func->bb_sorted[i])) > >>>> + return return_false(); > >>>> + > >>>> + dump_message ("All BBs are equal\n"); > >>>> + > >>>> + /* Basic block edges check. */ > >>>> + for (unsigned i = 0; i < bb_sorted.length (); ++i) > >>>> + { > >>>> + bb_dict = XNEWVEC (int, bb_sorted.length () + 2); > >>>> + memset (bb_dict, -1, (bb_sorted.length () + 2) * sizeof (int)); > >>>> + > >>>> + bb1 = bb_sorted[i]->bb; > >>>> + bb2 = m_compared_func->bb_sorted[i]->bb; > >>>> + > >>>> + ei2 = ei_start (bb2->preds); > >>>> + > >>>> + for (ei1 = ei_start (bb1->preds); ei_cond (ei1, &e1); ei_next > >>>> (&ei1)) > >>>> + { > >>>> + ei_cond (ei2, &e2); > >>>> + > >>>> + if (e1->flags != e2->flags) > >>>> + return return_false_with_msg ("flags comparison returns > >>>> false"); > >>>> + > >>>> + if (!bb_dict_test (bb_dict, e1->src->index, e2->src->index)) > >>>> + return return_false_with_msg ("edge comparison returns > >>>> false"); > >>>> + > >>>> + if (!bb_dict_test (bb_dict, e1->dest->index, e2->dest->index)) > >>>> + return return_false_with_msg ("BB comparison returns false"); > >>>> + > >>>> + if (!m_checker->compare_edge (e1, e2)) > >>>> + return return_false_with_msg ("edge comparison returns > >>>> false"); > >>>> + > >>>> + ei_next (&ei2); > >>>> + } > >>>> + } > >>>> + > >>>> + /* Basic block PHI nodes comparison. */ > >>>> + for (unsigned i = 0; i < bb_sorted.length (); i++) > >>>> + if (!compare_phi_node (bb_sorted[i]->bb, > >>>> m_compared_func->bb_sorted[i]->bb)) > >>>> + return return_false_with_msg ("PHI node comparison returns > >>>> false"); > >>>> + > >>>> + return result; > >>>> +} > >>> > >>> > >>> The rest of patch seems fine. I think we went across enough of > >>> iteraitons, the patch is OK > >>> with changes above and lets handle rest incrementally. > >>> > >>> Thanks! > >>> Honza > >>> > >> > >> Hello > >> > >> There's final version of the patch I'm going to commit tomorrow in the > >> morning (CEST). > >> Thank you Honza for the review. > >> > >> Martin