diff mbox

Fix PR ipa/65908.

Message ID 5555F205.1060609@suse.cz
State New
Headers show

Commit Message

Martin Liška May 15, 2015, 1:17 p.m. UTC 
On 05/15/2015 12:38 PM, Jakub Jelinek wrote:
> On Fri, May 15, 2015 at 10:38:57AM +0200, Martin Liška wrote:
>> Following patch is fix for GCC-5 branch for PR ipa/65908, was tested on x86_64-linux-pc, as well as bootstrapped.
>> As soon as the patch is applied, I'm going to send the similar patch for trunk.
>
> I'll leave the review to Honza or Richi, just a few nits:
> 1) it should go to the trunk first, then to GCC-5 branch
>
>> --- a/gcc/ipa-icf.c
>> +++ b/gcc/ipa-icf.c
>> @@ -417,6 +417,33 @@ bool sem_function::compare_edge_flags (cgraph_edge *e1, cgraph_edge *e2)
>>     return true;
>>   }
>> +/* Return true if DECL_ARGUMENT types are valid to be merged.  */
>
> Please add another newline between } and the comment.
>> --- a/gcc/ipa-icf.h
>> +++ b/gcc/ipa-icf.h
>> @@ -364,6 +364,9 @@ private:
>>     bool equals_private (sem_item *item,
>>   		       hash_map <symtab_node *, sem_item *> &ignored_nodes);
>> +  /* Return true if DECL_ARGUMENT types are valid to be merged.  */
>> +  bool compatible_parm_types_p ();
>> +
>>     /* Returns true if tree T can be compared as a handled component.  */
>>     static bool icf_handled_component_p (tree t);
>
> And here should be an empty line above the Return true if ... comment
> too.  Though, I wonder how this can apply cleanly to the current 5 branch,
> because there is an empty newline in between equals_private and the comment
> above icf_handled_component_p.
>
>> diff --git a/gcc/testsuite/g++.dg/ipa/pr65908.C b/gcc/testsuite/g++.dg/ipa/pr65908.C
>> new file mode 100644
>> index 0000000..c1884ab
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/ipa/pr65908.C
>> @@ -0,0 +1,26 @@
>> +// PR ipa/65908
>> +// { dg-options "-O2 -fPIC" }
>> +// { dg-do compile { target { { fpic } } } }
>
> Perhaps
> // PR ipa/65908
> // { dg-do compile }
> // { dg-options "-O2" }
> // { dg-additional-options "-fPIC" { target fpic } }
> instead?
>
> 	Jakub
>

Hi.

Thanks for all nits. I've just bootstrapped reapplied version of the patch, based on
current trunk with respect to all changes observed by Jakub.

Patch can also survive regression tests.

What do you think about it Honza?

Thanks,
Martin

Comments

Jan Hubicka May 15, 2015, 6:52 p.m. UTC | #1 
> +/* Return true if DECL_ARGUMENT types are valid to be merged.  */
Perhaps bettter as

Perform additional check needed to match types function parameters that are
used.  Unlike for normal parameters it matters if type is TYPE_RESTRICT and we
make an assumption that REFERENCE_TYPE parameters are always non-NULL.

> +
> +bool
> +sem_function::compatible_parm_types_p ()
> +{
> +  tree parm1, parm2;
> +  unsigned i = 0;
> +
> +  for (parm1 = DECL_ARGUMENTS (decl),
> +       parm2 = DECL_ARGUMENTS (m_compared_func->decl);
> +       parm1 && parm2;
> +       parm1 = DECL_CHAIN (parm1), parm2 = DECL_CHAIN (parm2), i++)

I think this is still not right.  What you wan to to do is to have

1) comparible_parm_types_p (t1,t2, index) that returns true if T1 and T2 are
   matching with checks bellow:
> +  {
> +    if (!param_used_p (i))
> +      continue;
> +
> +    if (POINTER_TYPE_P (parm1)
> +	&& (TYPE_RESTRICT (parm1) != TYPE_RESTRICT (parm2)))
> +      return return_false_with_msg ("argument restrict flag mismatch");
> +    /* nonnull_arg_p implies non-zero range to REFERENCE types.  */
> +    if (POINTER_TYPE_P (parm1)
> +	&& TREE_CODE (parm1) != TREE_CODE (parm2)
> +	&& opt_for_fn (decl, flag_delete_null_pointer_checks))
> +      return return_false_with_msg ("pointer wrt reference mismatch");
> +  }
   withtout actually walking the chain.

2) make equals_wpa to walk TYPE_ARG_TYPES of the function type and match them.
   This is because DECL_ARGUMENTS are part of function body and before you
   read it into memory, these are NULL

   Walking DECL_ARGUMENTS here may cause ipa-icf to give up in case one body is
   read (and thus have some arguments) and other is not.

3) make equals_private walk DECL_ARGUMENTS and match them
   (well at the time you populate the map.)
   You probalby can skip matching PARM_DECLS that are !parm_used_p (i)
   for anything else than types_compatible_p.

   We only care they are passed the same way by ABI. Everything else is not
   relevant.

Honza
diff mbox

Patch

From 22ee6b7aaae73db83663e8b5c12054b2f126a53e Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Fri, 15 May 2015 13:23:33 +0200
Subject: [PATCH] Fix PR ipa/65908.

gcc/testsuite/ChangeLog:

2015-05-12  Martin Liska  <mliska@suse.cz>

	* g++.dg/ipa/pr65908.C: New test.

gcc/ChangeLog:

2015-05-12  Martin Liska  <mliska@suse.cz>

	PR ipa/65908
	* ipa-icf.c (sem_function::equals_private): Always compare argument
	type got from TYPE_ARG_TYPES.
	(sem_function::compatible_parm_types_p): New function.
	(sem_function::equals_wpa): Use the function.
	(sem_function::equals_private): Likewise.
	(sem_function::parse_tree_args): Handle case where we have a different
	number of arguments.
	* ipa-icf.h (sem_function::compatible_parm_types_p): Declare new
	function.
---
 gcc/ipa-icf.c                      | 78 +++++++++++++++++++++-----------------
 gcc/ipa-icf.h                      |  3 ++
 gcc/testsuite/g++.dg/ipa/pr65908.C | 27 +++++++++++++
 3 files changed, 74 insertions(+), 34 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr65908.C

diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index 3c4ac05..5d1881e 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -594,6 +594,38 @@  sem_function::param_used_p (unsigned int i)
   return ipa_is_param_used (IPA_NODE_REF (get_node ()), i);
 }
 
+/* Return true if DECL_ARGUMENT types are valid to be merged.  */
+
+bool
+sem_function::compatible_parm_types_p ()
+{
+  tree parm1, parm2;
+  unsigned i = 0;
+
+  for (parm1 = DECL_ARGUMENTS (decl),
+       parm2 = DECL_ARGUMENTS (m_compared_func->decl);
+       parm1 && parm2;
+       parm1 = DECL_CHAIN (parm1), parm2 = DECL_CHAIN (parm2), i++)
+  {
+    if (!param_used_p (i))
+      continue;
+
+    if (POINTER_TYPE_P (parm1)
+	&& (TYPE_RESTRICT (parm1) != TYPE_RESTRICT (parm2)))
+      return return_false_with_msg ("argument restrict flag mismatch");
+    /* nonnull_arg_p implies non-zero range to REFERENCE types.  */
+    if (POINTER_TYPE_P (parm1)
+	&& TREE_CODE (parm1) != TREE_CODE (parm2)
+	&& opt_for_fn (decl, flag_delete_null_pointer_checks))
+      return return_false_with_msg ("pointer wrt reference mismatch");
+  }
+
+  if (parm1 || parm2)
+    return return_false ();
+
+  return true;
+}
+
 /* Fast equality function based on knowledge known in WPA.  */
 
 bool
@@ -713,22 +745,12 @@  sem_function::equals_wpa (sem_item *item,
       if (!func_checker::compatible_types_p (arg_types[i],
 					     m_compared_func->arg_types[i]))
 	return return_false_with_msg ("argument type is different");
-
-      /* On used arguments we need to do a bit more of work.  */
-      if (!param_used_p (i))
-	continue;
-      if (POINTER_TYPE_P (arg_types[i])
-	  && (TYPE_RESTRICT (arg_types[i])
-	      != TYPE_RESTRICT (m_compared_func->arg_types[i])))
-	return return_false_with_msg ("argument restrict flag mismatch");
-      /* nonnull_arg_p implies non-zero range to REFERENCE types.  */
-      if (POINTER_TYPE_P (arg_types[i])
-	  && TREE_CODE (arg_types[i])
-	     != TREE_CODE (m_compared_func->arg_types[i])
-	  && opt_for_fn (decl, flag_delete_null_pointer_checks))
-	return return_false_with_msg ("pointer wrt reference mismatch");
     }
 
+  /* For functions with !prototype_p, we have to compare DECL_ARGUMENTS.  */
+  if (!compatible_parm_types_p ())
+    return return_false ();
+
   if (node->num_references () != item->node->num_references ())
     return return_false_with_msg ("different number of references");
 
@@ -938,9 +960,12 @@  sem_function::equals_private (sem_item *item)
   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))
+    if (!arg2 || !m_checker->compare_decl (arg1, arg2))
       return return_false ();
 
+  if (!compatible_parm_types_p ())
+    return return_false ();
+
   if (!dyn_cast <cgraph_node *> (node)->has_gimple_body_p ())
     return true;
 
@@ -1709,30 +1734,15 @@  sem_function::parse (cgraph_node *node, bitmap_obstack *stack)
 void
 sem_function::parse_tree_args (void)
 {
-  tree result;
-
   if (arg_types.exists ())
     arg_types.release ();
 
   arg_types.create (4);
-  tree fnargs = DECL_ARGUMENTS (decl);
+  tree type = TYPE_ARG_TYPES (TREE_TYPE (decl));
+  for (tree parm = type; parm; parm = TREE_CHAIN (parm))
+    arg_types.safe_push (TREE_VALUE (parm));
 
-  for (tree parm = fnargs; parm; parm = DECL_CHAIN (parm))
-    arg_types.safe_push (DECL_ARG_TYPE (parm));
-
-  /* Function result type.  */
-  result = DECL_RESULT (decl);
-  result_type = result ? TREE_TYPE (result) : NULL;
-
-  /* During WPA, we can get arguments by following method.  */
-  if (!fnargs)
-    {
-      tree type = TYPE_ARG_TYPES (TREE_TYPE (decl));
-      for (tree parm = type; parm; parm = TREE_CHAIN (parm))
-	arg_types.safe_push (TYPE_CANONICAL (TREE_VALUE (parm)));
-
-      result_type = TREE_TYPE (TREE_TYPE (decl));
-    }
+  result_type = TREE_TYPE (TREE_TYPE (decl));
 }
 
 /* For given basic blocks BB1 and BB2 (from functions FUNC1 and FUNC),
diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
index a3b9ab9..e6953a4 100644
--- a/gcc/ipa-icf.h
+++ b/gcc/ipa-icf.h
@@ -382,6 +382,9 @@  private:
   /* Processes function equality comparison.  */
   bool equals_private (sem_item *item);
 
+  /* Return true if DECL_ARGUMENT types are valid to be merged.  */
+  bool compatible_parm_types_p ();
+
   /* Returns true if tree T can be compared as a handled component.  */
   static bool icf_handled_component_p (tree t);
 
diff --git a/gcc/testsuite/g++.dg/ipa/pr65908.C b/gcc/testsuite/g++.dg/ipa/pr65908.C
new file mode 100644
index 0000000..38730bd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr65908.C
@@ -0,0 +1,27 @@ 
+// PR ipa/65908
+// { dg-do compile }
+// { dg-options "-O2" }
+// { dg-additional-options "-fPIC" { target fpic } }
+
+class A
+{
+  A (A &);
+};
+class B
+{
+  const A &m_fn1 () const;
+};
+class C
+{
+  A m_fn2 () const;
+};
+A
+C::m_fn2 () const
+{
+  throw 0;
+}
+const A &
+B::m_fn1 () const
+{
+  throw 0;
+}
-- 
2.1.4