This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 3/5] IPA ICF pass


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]