From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14602 invoked by alias); 11 Oct 2014 00:02:39 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 14587 invoked by uid 89); 11 Oct 2014 00:02:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: mx2.suse.de Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Sat, 11 Oct 2014 00:02:37 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id E8179AB0C; Sat, 11 Oct 2014 00:02:33 +0000 (UTC) Message-ID: <54387443.1010105@suse.cz> Date: Sat, 11 Oct 2014 00:08:00 -0000 From: =?windows-1252?Q?Martin_Li=9Aka?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Jan Hubicka CC: gcc-patches@gcc.gnu.org Subject: Re: [PATCH 3/5] IPA ICF pass References: <20140620073156.GC12633@tsaunders-iceball.corp.tor1.mozilla.com> <20140705225351.GK16837@kam.mff.cuni.cz> <53C7E626.8080400@suse.cz> <54255A09.1090305@suse.cz> <20140926204654.GD26922@atrey.karlin.mff.cuni.cz> In-Reply-To: <20140926204654.GD26922@atrey.karlin.mff.cuni.cz> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-10/txt/msg01036.txt.bz2 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