From: Joey Ye <joey.ye.cc@gmail.com>
To: Jiong Wang <wong.kwongyuan.tools@gmail.com>
Cc: "Martin Liška" <mliska@suse.cz>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
"hubicka >> Jan Hubicka" <hubicka@ucw.cz>
Subject: Re: [PATCH 3/5] IPA ICF pass
Date: Thu, 06 Nov 2014 03:01:00 -0000 [thread overview]
Message-ID: <CAL0py24Ou5u5t5bahaLz96h7P1TnNF+zWcDG2=VRah+j1Uu_eQ@mail.gmail.com> (raw)
In-Reply-To: <CAAfDdZ2Y94zUi7+fgvbznpOR92VcgKWUp1h4M=qzD+aCxkQYFA@mail.gmail.com>
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?
Thanks,
Joey
On Thu, Oct 23, 2014 at 5:18 AM, Jiong Wang
<wong.kwongyuan.tools@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 <mliska@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
next prev parent reply other threads:[~2014-11-06 3:01 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-16 10:07 [PATCH 1/5] New Identical Code Folding IPA pass mliska
2014-06-16 10:07 ` [PATCH 4/5] Existing tests fix mliska
2014-06-17 19:52 ` Jeff Law
2014-06-17 20:50 ` Rainer Orth
2014-06-18 9:02 ` Martin Liška
2014-06-30 12:12 ` Martin Liška
2014-09-26 12:21 ` Martin Liška
2014-06-16 10:07 ` [PATCH 3/5] IPA ICF pass mliska
2014-06-20 7:32 ` Trevor Saunders
2014-06-26 11:18 ` Martin Liška
2014-07-05 21:44 ` Gerald Pfeifer
2014-07-05 22:53 ` Jan Hubicka
2014-07-17 15:23 ` Martin Liška
2014-09-26 12:20 ` Martin Liška
2014-09-26 14:44 ` Markus Trippelsdorf
2014-09-26 23:27 ` Jan Hubicka
2014-09-27 5:59 ` Markus Trippelsdorf
2014-09-27 7:47 ` Markus Trippelsdorf
2014-09-27 10:46 ` Martin Liška
2014-09-27 10:37 ` Martin Liška
2014-09-28 2:21 ` Jan Hubicka
2014-10-10 23:54 ` Fakturace
2014-10-11 0:02 ` Martin Liška
2014-10-11 8:23 ` Jan Hubicka
2014-10-13 13:20 ` Martin Liška
2014-10-13 13:29 ` Jan Hubicka
2014-09-27 10:32 ` Martin Liška
2014-09-26 20:47 ` Jan Hubicka
2014-10-11 0:08 ` Martin Liška
2014-10-11 8:26 ` Jan Hubicka
2014-10-13 15:17 ` Martin Liška
2014-10-14 16:12 ` Jan Hubicka
2014-10-15 17:06 ` Martin Liška
2014-10-22 21:20 ` Jiong Wang
2014-11-06 3:01 ` Joey Ye [this message]
2014-11-06 9:03 ` Jan Hubicka
2014-11-13 22:26 ` H.J. Lu
2015-01-20 21:29 ` H.J. Lu
2014-09-26 22:27 ` Jan Hubicka
2014-10-11 0:23 ` Martin Liška
2014-10-11 9:05 ` Jan Hubicka
2014-06-24 20:31 ` Jeff Law
2014-06-26 16:02 ` Martin Liška
2014-06-26 18:46 ` Jan Hubicka
2014-06-30 12:05 ` Martin Liška
2014-07-01 23:45 ` Trevor Saunders
2014-06-30 19:06 ` Jeff Law
2014-06-16 10:07 ` [PATCH 5/5] New tests introduction mliska
2014-06-17 19:53 ` Jeff Law
2014-06-30 12:14 ` Martin Liška
2014-10-19 8:19 ` Andreas Schwab
2014-10-23 12:34 ` Martin Liška
2014-06-16 10:07 ` [PATCH 2/5] Existing call graph infrastructure enhancement mliska
2014-06-17 20:00 ` Jeff Law
2014-06-30 11:49 ` Martin Liška
2014-06-30 18:54 ` Jeff Law
2014-07-17 14:54 ` Martin Liška
2014-08-25 9:56 ` Martin Liška
2014-08-25 16:12 ` Jan Hubicka
2014-08-27 21:12 ` Jeff Law
2014-09-24 14:23 ` Martin Liška
2014-09-24 15:01 ` Jan Hubicka
2014-09-26 10:19 ` Martin Liška
2014-06-17 19:49 ` [PATCH 1/5] New Identical Code Folding IPA pass Jeff Law
2014-06-18 19:05 ` Jan Hubicka
2014-06-17 20:09 ` Paolo Carlini
2014-06-18 8:46 ` Martin Liška
2014-06-18 8:49 ` Paolo Carlini
2014-06-17 20:17 ` David Malcolm
2014-06-18 8:10 ` Martin Liška
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAL0py24Ou5u5t5bahaLz96h7P1TnNF+zWcDG2=VRah+j1Uu_eQ@mail.gmail.com' \
--to=joey.ye.cc@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hubicka@ucw.cz \
--cc=mliska@suse.cz \
--cc=wong.kwongyuan.tools@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).