From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26814 invoked by alias); 22 Oct 2014 21:18:40 -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 26804 invoked by uid 89); 22 Oct 2014 21:18:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qa0-f43.google.com Received: from mail-qa0-f43.google.com (HELO mail-qa0-f43.google.com) (209.85.216.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 22 Oct 2014 21:18:37 +0000 Received: by mail-qa0-f43.google.com with SMTP id j7so3083560qaq.16 for ; Wed, 22 Oct 2014 14:18:34 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.140.49.107 with SMTP id p98mr1272302qga.20.1414012714241; Wed, 22 Oct 2014 14:18:34 -0700 (PDT) Received: by 10.140.93.119 with HTTP; Wed, 22 Oct 2014 14:18:34 -0700 (PDT) In-Reply-To: <543EA8EC.9070500@suse.cz> 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> <54387443.1010105@suse.cz> <543BEAD2.6010508@suse.cz> <20141014160437.GA26768@atrey.karlin.mff.cuni.cz> <543EA8EC.9070500@suse.cz> Date: Wed, 22 Oct 2014 21:20:00 -0000 Message-ID: Subject: Re: [PATCH 3/5] IPA ICF pass From: Jiong Wang To: =?UTF-8?Q?Martin_Li=C5=A1ka?= Cc: "gcc-patches@gcc.gnu.org" , "hubicka >> Jan Hubicka" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2014-10/txt/msg02308.txt.bz2 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=C5=A1ka : > 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=3D@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 =3D TREE_OPERAND (t1, 0); >>> + x2 =3D TREE_OPERAND (t2, 0); >>> + y1 =3D TREE_OPERAND (t1, 1); >>> + y2 =3D 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 =3D false; >>> + >>> + if (t1 =3D=3D t2) >>> + return true; >>> + >>> + symtab_node *n1 =3D symtab_node::get (t1); >>> + symtab_node *n2 =3D symtab_node::get (t2); >>> + >>> + if (m_ignored_source_nodes !=3D NULL && m_ignored_target_nodes !=3D = NULL) >>> + { >>> + ret =3D 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 =3D cgraph_node::get (t1); >>> + cgraph_node *f2 =3D cgraph_node::get (t2); >>> + >>> + if(f1 && f2 && f1->weakref && f2->weakref) >>> + ret =3D f1->alias_target =3D=3D 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 =3D 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 =3D gsi_start_bb (bb->bb); !gsi_end_p >>> (gsi); >>> + gsi_next (&gsi)) >>> + { >>> + gimple stmt =3D gsi_stmt (gsi); >>> + >>> + if (gimple_code (stmt) =3D=3D GIMPLE_LABEL) >>> + { >>> + tree t =3D gimple_label_label (stmt); >>> + gcc_assert (TREE_CODE (t) =3D=3D 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) correspon= d. >>> + >>> + 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 !=3D bb2->nondbg_stmt_count >>> + || bb1->edge_count !=3D bb2->edge_count) >>> + return return_false (); >>> + >>> + gsi1 =3D gsi_start_bb (bb1->bb); >>> + gsi2 =3D gsi_start_bb (bb2->bb); >>> + >>> + for (i =3D 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 =3D gsi_stmt (gsi1); >>> + s2 =3D gsi_stmt (gsi2); >>> + >>> + int eh1 =3D lookup_stmt_eh_lp_fn >>> + (DECL_STRUCT_FUNCTION (m_source_func_decl), s1); >>> + int eh2 =3D lookup_stmt_eh_lp_fn >>> + (DECL_STRUCT_FUNCTION (m_target_func_decl), s2); >>> + >>> + if (eh1 !=3D eh2) >>> + return return_false_with_msg ("EH regions are different"); >>> + >>> + if (gimple_code (s1) !=3D 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) !=3D gimple_call_num_args (s2)) >>> + return false; >>> + >>> + t1 =3D gimple_call_fndecl (s1); >>> + t2 =3D gimple_call_fndecl (s2); >>> + >>> + /* Function pointer variables are not supported yet. */ >>> + if (t1 =3D=3D NULL || t2 =3D=3D 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 =3D=3D cn2->alias_target) >>> + return true; >> >> >> Lets consistently drop the weakrefs handling and add full alias handling >> incrementally. >>> >>> + >>> + /* Checking function arguments. */ >> >> attributes >>> >>> + tree decl1 =3D DECL_ATTRIBUTES (decl); >>> + tree decl2 =3D DECL_ATTRIBUTES (m_compared_func->decl); >> >> >> You can still do this as part of the wap_comparison, right? >>> >>> + >>> + m_checker =3D new func_checker (decl, m_compared_func->decl, >>> + compare_polymorphic_p (), >>> + false, >>> + &refs_set, >>> + &m_compared_func->refs_set); >>> + while (decl1) >>> + { >>> + if (decl2 =3D=3D NULL) >>> + return return_false (); >>> + >>> + if (get_attribute_name (decl1) !=3D get_attribute_name (decl2)) >>> + return return_false (); >>> + >>> + tree attr_value1 =3D TREE_VALUE (decl1); >>> + tree attr_value2 =3D TREE_VALUE (decl2); >>> + >>> + if (attr_value1 && attr_value2) >>> + { >>> + bool ret =3D 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 =3D TREE_CHAIN (decl1); >>> + decl2 =3D TREE_CHAIN (decl2); >>> + } >>> + >>> + if (decl1 !=3D decl2) >>> + return return_false(); >>> + >>> + >>> + for (arg1 =3D DECL_ARGUMENTS (decl), >>> + arg2 =3D DECL_ARGUMENTS (m_compared_func->decl); >>> + arg1; arg1 =3D DECL_CHAIN (arg1), arg2 =3D DECL_CHAIN (arg2)) >>> + if (!m_checker->compare_decl (arg1, arg2)) >>> + return return_false (); >>> + >>> + /* Fill-up label dictionary. */ >>> + for (unsigned i =3D 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 =3D 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 =3D 0; i < bb_sorted.length (); ++i) >>> + { >>> + bb_dict =3D XNEWVEC (int, bb_sorted.length () + 2); >>> + memset (bb_dict, -1, (bb_sorted.length () + 2) * sizeof (int)); >>> + >>> + bb1 =3D bb_sorted[i]->bb; >>> + bb2 =3D m_compared_func->bb_sorted[i]->bb; >>> + >>> + ei2 =3D ei_start (bb2->preds); >>> + >>> + for (ei1 =3D ei_start (bb1->preds); ei_cond (ei1, &e1); ei_next >>> (&ei1)) >>> + { >>> + ei_cond (ei2, &e2); >>> + >>> + if (e1->flags !=3D 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 =3D 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