On 10/11/2014 02:05 AM, Martin Liška wrote: > On 09/26/2014 09:46 PM, Jan Hubicka wrote: >> Hi, >> this is on ipa-icf-gimple.c >> >> @@ -2827,11 +2829,19 @@ cgraph_node::verify_node (void) >> { >> if (verify_edge_corresponds_to_fndecl (e, decl)) >> { >> - error ("edge points to wrong declaration:"); >> - debug_tree (e->callee->decl); >> - fprintf (stderr," Instead of:"); >> - debug_tree (decl); >> - error_found = true; >> + /* The edge can be redirected in WPA by IPA ICF. >> + Following check really ensures that it's >> + not the case. */ >> + >> + cgraph_node *current_node = cgraph_node::get (decl); >> + if (!current_node || !current_node->icf_merged) >> >> I would move this into verify_edge_corresponds_to_fndecl. >> >> diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c >> new file mode 100644 >> index 0000000..7031eaa >> --- /dev/null >> +++ b/gcc/ipa-icf-gimple.c >> @@ -0,0 +1,384 @@ >> +/* Interprocedural Identical Code Folding pass >> + Copyright (C) 2014 Free Software Foundation, Inc. >> + >> + Contributed by Jan Hubicka and Martin Liska >> + >> +This file is part of GCC. >> + >> +GCC is free software; you can redistribute it and/or modify it under >> +the terms of the GNU General Public License as published by the Free >> +Software Foundation; either version 3, or (at your option) any later >> +version. >> + >> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY >> +WARRANTY; without even the implied warranty of MERCHANTABILITY or >> +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >> +for more details. >> + >> +You should have received a copy of the GNU General Public License >> +along with GCC; see the file COPYING3. If not see >> +. */ >> >> Please add toplevel comment about what the code does and how to use it. >> >> +namespace ipa_icf { >> + >> +/* Basic block equivalence comparison function that returns true if >> + basic blocks BB1 and BB2 (from functions FUNC1 and FUNC2) correspond. */ >> ... to each other? >> I would add short comment that as comparsion goes you build voclabulary >> of equivalences of variables/ssanames etc. >> So people reading the code do not get lost at very beggining. >> >> + >> +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 (); >> >> The UPPERCASE looks ugly. I see that RETURN_FALSE is a warpper for return_false_with_msg >> that outputs line and file information. >> >> I would make it lowercase even if it is macro. You may consider using >> CXX_MEM_STAT_INFO style default argument to avoid function macro completely. >> Probably not big win given that it won't save you from preprocesor mess. >> + >> + 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); >> + >> + if (gimple_code (s1) != gimple_code (s2)) >> + return RETURN_FALSE_WITH_MSG ("gimple codes are different"); >> >> I think you need to compare EH here. Consider case where one unit >> is compiled with -fno-exception and thus all EH regions are removed, >> while other function has EH regions in it. Those are not equivalent. >> >> EH region is obtained by lookup_stmt_eh and then you need to comapre >> them for match as you do with gimple_resx_regoin. >> >> + t1 = gimple_call_fndecl (s1); >> + t2 = gimple_call_fndecl (s2); >> + >> + /* Function pointer variables are not supported yet. */ >> >> They seems to be, compare_operand seems just right. >> >> + >> +/* Verifies for given GIMPLEs S1 and S2 that >> + label statements are semantically equivalent. */ >> + >> +bool >> +func_checker::compare_gimple_label (gimple g1, gimple g2) >> +{ >> + if (m_ignore_labels) >> + return true; >> + >> + tree t1 = gimple_label_label (g1); >> + tree t2 = gimple_label_label (g2); >> + >> + return compare_tree_ssa_label (t1, t2); >> +} >> >> I would expect the main BB loop to record BB in which label belongs to >> and the BB assciatio neing checked here. >> Otherwise I do not see how switch statements are compared to not have >> different permutations of targets. Also note that one BB may have >> multiple labels in them and they are equivalent. >> >> Also I would punt on occurence of FORCED_LABEL. Those are tricky as they >> may be passed around and compared for address and no one really defines >> what should happen. Better to avoid those. > > Hi. > > I will remove this support in the pass. > >> >> + >> +/* Verifies for given GIMPLEs S1 and S2 that >> + switch statements are semantically equivalent. */ >> + >> +bool >> +func_checker::compare_gimple_switch (gimple g1, gimple g2) >> +{ >> + unsigned lsize1, lsize2, i; >> + tree t1, t2, low1, low2, high1, high2; >> + >> + lsize1 = gimple_switch_num_labels (g1); >> + lsize2 = gimple_switch_num_labels (g2); >> + >> + if (lsize1 != lsize2) >> + return false; >> + >> + t1 = gimple_switch_index (g1); >> + t2 = gimple_switch_index (g2); >> + >> + if (TREE_CODE (t1) != SSA_NAME || TREE_CODE(t2) != SSA_NAME) >> + return false; >> >> Why non-SSA_NAMES are excluded? I see that constants should be optimized out, >> but I do not see a need for specific test here. >> + >> + if (!compare_operand (t1, t2)) >> + return false; >> + >> + for (i = 0; i < lsize1; i++) >> + { >> + low1 = CASE_LOW (gimple_switch_label (g1, i)); >> + low2 = CASE_LOW (gimple_switch_label (g2, i)); >> + >> + if ((low1 != NULL) != (low2 != NULL) >> + || (low1 && low2 && TREE_INT_CST_LOW (low1) != TREE_INT_CST_LOW (low2))) >> + return false; >> >> You want to compare integers for equivality. Just use tree_int_cst_equal. >> + >> + high1 = CASE_HIGH (gimple_switch_label (g1, i)); >> + high2 = CASE_HIGH (gimple_switch_label (g2, i)); >> + >> + if ((high1 != NULL) != (high2 != NULL) >> + || (high1 && high2 >> + && TREE_INT_CST_LOW (high1) != TREE_INT_CST_LOW (high2))) >> + return false; >> >> Same here. >> + } >> + >> + return true; >> +} >> + >> +/* Verifies for given GIMPLEs S1 and S2 that >> + return statements are semantically equivalent. */ >> + >> +bool >> +func_checker::compare_gimple_return (gimple g1, gimple g2) >> +{ >> + tree t1, t2; >> + >> + t1 = gimple_return_retval (g1); >> + t2 = gimple_return_retval (g2); >> + >> + /* Void return type. */ >> + if (t1 == NULL && t2 == NULL) >> + return true; >> + else >> + return compare_operand (t1, t2); >> >> Do you check somewhere DECL_BY_REFERENCE (DECL_RESULT (struct function))? Those needs to match, too. >> Perhaps it is always the case that SSA form differs, but I would test it. >> +} >> + >> +/* Verifies for given GIMPLEs S1 and S2 that >> + goto statements are semantically equivalent. */ >> + >> +bool >> +func_checker::compare_gimple_goto (gimple g1, gimple g2) >> +{ >> + tree dest1, dest2; >> + >> + dest1 = gimple_goto_dest (g1); >> + dest2 = gimple_goto_dest (g2); >> + >> + if (TREE_CODE (dest1) != TREE_CODE (dest2) || TREE_CODE (dest1) != SSA_NAME) >> + return false; >> + >> + return compare_operand (dest1, dest2); >> >> You probably need to care only about indirect gotos, the direct ones are checked by >> CFG compare. So is the condtional jump. > > It looks that this code is visited quite rare. > >> + >> +/* Verifies for given GIMPLEs S1 and S2 that ASM statements are equivalent. >> + For the beginning, the pass only supports equality for >> + '__asm__ __volatile__ ("", "", "", "memory")'. */ >> + >> +bool >> +func_checker::compare_gimple_asm (gimple g1, gimple g2) >> +{ >> + if (gimple_asm_volatile_p (g1) != gimple_asm_volatile_p (g2)) >> + return false; >> + >> + if (gimple_asm_ninputs (g1) || gimple_asm_ninputs (g2)) >> + return false; >> + >> + if (gimple_asm_noutputs (g1) || gimple_asm_noutputs (g2)) >> + return false; >> + >> + if (gimple_asm_nlabels (g1) || gimple_asm_nlabels (g2)) >> + return false; >> + >> + if (gimple_asm_nclobbers (g1) != gimple_asm_nclobbers (g2)) >> + return false; >> + >> + for (unsigned i = 0; i < gimple_asm_nclobbers (g1); i++) >> + { >> + tree clobber1 = TREE_VALUE (gimple_asm_clobber_op (g1, i)); >> + tree clobber2 = TREE_VALUE (gimple_asm_clobber_op (g2, i)); >> + >> + if (!operand_equal_p (clobber1, clobber2, OEP_ONLY_CONST)) >> + return false; >> + } >> + >> >> Even asm statements with no inputs or outputs can differ by the actual >> asm statement. Compare it too. >> >> Comparing inputs/outputs/labels should be very easy to do. >> >> Compare all gimple_asm_n* for equivalency. > > This makes fully sense, but I don't understand what kind of operands do you mean? > >> At the end walk operands and watch the case they are TREE_LIST. >> THen compare TREE_VALUE (op) of the list for operand_equal_p >> and TREE_VALUE (TREE_PURPOSE (op)) for equivalency >> (those are the constraints) >> >> If they are not (clobbers are not, those are just strings), operand_equal_p >> should do. >> >> + return true; >> +} >> + >> +} // ipa_icf namespace >> >> Otherwise I think ipa-gimple-icf is quite fine now. >> Please send updated version and I think it can go to mainline before the actual ipa-icf. > > I renamed both files and put them to a newly created namespace ipa_icf_gimple. > > Thank you, > Martin > Hello. There's latest version of the patch. Changes: + gimple_asm should be correctly handled + FORCED_LABEL prevents function merging Thanks, Martin