public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] IPA ICF: refactoring + fix for PR ipa/63569
@ 2014-12-10 12:19 Martin Liška
  2014-12-11 12:37 ` Richard Biener
  0 siblings, 1 reply; 10+ messages in thread
From: Martin Liška @ 2014-12-10 12:19 UTC (permalink / raw)
  To: GCC Patches; +Cc: hubi >> Jan Hubicka

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]

Hello.

As suggested by Richard, I split compare_operand functions to various functions
related to a specific comparison. Apart from that I added fast check for
volatility flag that caused miscompilation mentioned in PR63569.

Patch can bootstrap on x86_64-linux-pc without any regression seen and I was
able to build Firefox with LTO.

Ready for trunk?
Thanks,
Martin

[-- Attachment #2: 0001-IPA-ICF-compare_operand-is-split-to-multiple-functio.patch --]
[-- Type: text/x-patch, Size: 11591 bytes --]

From 773308af2d2f93a3fca17f3c07030ec9762accc7 Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Wed, 10 Dec 2014 12:56:48 +0100
Subject: [PATCH] IPA ICF: compare_operand is split to multiple functions.

gcc/testsuite/ChangeLog:

2014-12-10  Martin Liska  <mliska@suse.cz>

	* gcc.dg/ipa/pr63569.c: New test.

gcc/ChangeLog:

2014-12-10  Martin Liska  <mliska@suse.cz>

	PR ipa/63569
	* ipa-icf-gimple.c (func_checker::compare_ssa_name): More complex
	comparison is moved to this function.
	(func_checker::compare_memory_operand): New function is responsible
	for comparison of a given memory operands.
	(func_checker::compare_operand): Global switch is reduced and more
	specific comparison functions are called.
	(func_checker::compare_cst_or_decl): New function compares declarations
	and contant types.
	* ipa-icf-gimple.h: Declaration of new function is added.
---
 gcc/ipa-icf-gimple.c               | 271 ++++++++++++++++++++-----------------
 gcc/ipa-icf-gimple.h               |   9 +-
 gcc/testsuite/gcc.dg/ipa/pr63569.c |  32 +++++
 3 files changed, 190 insertions(+), 122 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr63569.c

diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c
index 8f2a438..4ee343d 100644
--- a/gcc/ipa-icf-gimple.c
+++ b/gcc/ipa-icf-gimple.c
@@ -110,6 +110,9 @@ func_checker::~func_checker ()
 bool
 func_checker::compare_ssa_name (tree t1, tree t2)
 {
+  gcc_assert (TREE_CODE (t1) == SSA_NAME);
+  gcc_assert (TREE_CODE (t2) == SSA_NAME);
+
   unsigned i1 = SSA_NAME_VERSION (t1);
   unsigned i2 = SSA_NAME_VERSION (t2);
 
@@ -123,6 +126,20 @@ func_checker::compare_ssa_name (tree t1, tree t2)
   else if (m_target_ssa_names[i2] != (int) i1)
     return false;
 
+  if (SSA_NAME_IS_DEFAULT_DEF (t1))
+    {
+      tree b1 = SSA_NAME_VAR (t1);
+      tree b2 = SSA_NAME_VAR (t2);
+
+      if (b1 == NULL && b2 == NULL)
+	return true;
+
+      if (b1 == NULL || b2 == NULL || TREE_CODE (b1) != TREE_CODE (b2))
+	return return_false ();
+
+      return compare_cst_or_decl (b1, b2);
+    }
+
   return true;
 }
 
@@ -178,9 +195,10 @@ func_checker::compare_decl (tree t1, tree t2)
 }
 
 /* Return true if types are compatible from perspective of ICF.  */
-bool func_checker::compatible_types_p (tree t1, tree t2,
-				       bool compare_polymorphic,
-				       bool first_argument)
+bool
+func_checker::compatible_types_p (tree t1, tree t2,
+				  bool compare_polymorphic,
+				  bool first_argument)
 {
   if (TREE_CODE (t1) != TREE_CODE (t2))
     return return_false_with_msg ("different tree types");
@@ -211,76 +229,17 @@ bool func_checker::compatible_types_p (tree t1, tree t2,
   return true;
 }
 
-/* Function responsible for comparison of handled components T1 and T2.
-   If these components, from functions FUNC1 and FUNC2, are equal, true
-   is returned.  */
+/* Function compare for equality given memory operands T1 and T2.  */
 
 bool
-func_checker::compare_operand (tree t1, tree t2)
+func_checker::compare_memory_operand (tree t1, tree t2)
 {
-  tree base1, base2, x1, x2, y1, y2, z1, z2;
-  HOST_WIDE_INT offset1 = 0, offset2 = 0;
-  bool ret;
-
-  if (!t1 && !t2)
-    return true;
-  else if (!t1 || !t2)
-    return false;
-
-  tree tt1 = TREE_TYPE (t1);
-  tree tt2 = TREE_TYPE (t2);
-
-  if (!func_checker::compatible_types_p (tt1, tt2))
-    return false;
-
-  base1 = get_addr_base_and_unit_offset (t1, &offset1);
-  base2 = get_addr_base_and_unit_offset (t2, &offset2);
-
-  if (base1 && base2)
-    {
-      if (offset1 != offset2)
-	return return_false_with_msg ("base offsets are different");
-
-      t1 = base1;
-      t2 = base2;
-    }
+  bool ret = false;
 
-  if (TREE_CODE (t1) != TREE_CODE (t2))
-    return return_false ();
+  tree x1, x2, y1, y2;
 
   switch (TREE_CODE (t1))
     {
-    case CONSTRUCTOR:
-      {
-	unsigned length1 = vec_safe_length (CONSTRUCTOR_ELTS (t1));
-	unsigned length2 = vec_safe_length (CONSTRUCTOR_ELTS (t2));
-
-	if (length1 != length2)
-	  return return_false ();
-
-	for (unsigned i = 0; i < length1; i++)
-	  if (!compare_operand (CONSTRUCTOR_ELT (t1, i)->value,
-				CONSTRUCTOR_ELT (t2, i)->value))
-	    return return_false();
-
-	return true;
-      }
-    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);
     case MEM_REF:
       {
 	x1 = TREE_OPERAND (t1, 0);
@@ -323,67 +282,25 @@ func_checker::compare_operand (tree t1, tree t2)
 	y2 = TREE_OPERAND (t2, 1);
 
 	ret = compare_operand (x1, x2)
-	      && compare_operand (y1, y2);
+	      && compare_cst_or_decl (y1, y2);
 
 	return return_with_debug (ret);
       }
-    /* Virtual table call.  */
-    case OBJ_TYPE_REF:
-      {
-	x1 = TREE_OPERAND (t1, 0);
-	x2 = TREE_OPERAND (t2, 0);
-	y1 = TREE_OPERAND (t1, 1);
-	y2 = TREE_OPERAND (t2, 1);
-	z1 = TREE_OPERAND (t1, 2);
-	z2 = TREE_OPERAND (t2, 2);
+   default:
+     gcc_unreachable ();
+  }
+}
 
-	ret = compare_operand (x1, x2)
-	      && compare_operand (y1, y2)
-	      && compare_operand (z1, z2);
+/* Function compare for equality given trees T1 and T2 which
+   can be either a constant or a declaration type.  */
 
-	return return_with_debug (ret);
-      }
-    case ADDR_EXPR:
-      {
-	x1 = TREE_OPERAND (t1, 0);
-	x2 = TREE_OPERAND (t2, 0);
+bool
+func_checker::compare_cst_or_decl (tree t1, tree t2)
+{
+  bool ret;
 
-	ret = compare_operand (x1, x2);
-	return return_with_debug (ret);
-      }
-    case SSA_NAME:
-      {
-	ret = compare_ssa_name (t1, t2);
-
-	if (!ret)
-	  return return_with_debug (ret);
-
-	if (SSA_NAME_IS_DEFAULT_DEF (t1))
-	  {
-	    tree b1 = SSA_NAME_VAR (t1);
-	    tree b2 = SSA_NAME_VAR (t2);
-
-	    if (b1 == NULL && b2 == NULL)
-	      return true;
-
-	    if (b1 == NULL || b2 == NULL || TREE_CODE (b1) != TREE_CODE (b2))
-	      return return_false ();
-
-	    switch (TREE_CODE (b1))
-	      {
-	      case VAR_DECL:
-		return return_with_debug (compare_variable_decl (t1, t2));
-	      case PARM_DECL:
-	      case RESULT_DECL:
-		ret = compare_decl (b1, b2);
-		return return_with_debug (ret);
-	      default:
-		return return_false_with_msg ("Unknown TREE code reached");
-	      }
-	  }
-	else
-	  return true;
-      }
+  switch (TREE_CODE (t1))
+    {
     case INTEGER_CST:
       {
 	ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
@@ -435,6 +352,118 @@ func_checker::compare_operand (tree t1, tree t2)
 	return return_with_debug (ret);
       }
     default:
+      gcc_unreachable ();
+    }
+}
+
+/* Function responsible for comparison of various operands T1 and T2.
+   If these components, from functions FUNC1 and FUNC2, are equal, true
+   is returned.  */
+
+bool
+func_checker::compare_operand (tree t1, tree t2)
+{
+  tree x1, x2, y1, y2, z1, z2;
+  bool ret;
+
+  if (!t1 && !t2)
+    return true;
+  else if (!t1 || !t2)
+    return false;
+
+  tree tt1 = TREE_TYPE (t1);
+  tree tt2 = TREE_TYPE (t2);
+
+  if (!func_checker::compatible_types_p (tt1, tt2))
+    return false;
+
+  if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2))
+    return return_false_with_msg ("different operand volatility");
+
+  if (TREE_CODE (t1) != TREE_CODE (t2))
+    return return_false ();
+
+  switch (TREE_CODE (t1))
+    {
+    case CONSTRUCTOR:
+      {
+	unsigned length1 = vec_safe_length (CONSTRUCTOR_ELTS (t1));
+	unsigned length2 = vec_safe_length (CONSTRUCTOR_ELTS (t2));
+
+	if (length1 != length2)
+	  return return_false ();
+
+	for (unsigned i = 0; i < length1; i++)
+	{
+	  if (!compare_operand (CONSTRUCTOR_ELT (t1, i)->value,
+				CONSTRUCTOR_ELT (t2, i)->value))
+	    return return_false();
+	}
+
+	return true;
+      }
+    case ARRAY_REF:
+    case ARRAY_RANGE_REF:
+      /* First argument is the array, second is the index.  */
+      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);
+    case MEM_REF:
+    case COMPONENT_REF:
+      return compare_memory_operand (t1, t2);
+    /* Virtual table call.  */
+    case OBJ_TYPE_REF:
+      {
+	x1 = TREE_OPERAND (t1, 0);
+	x2 = TREE_OPERAND (t2, 0);
+	y1 = TREE_OPERAND (t1, 1);
+	y2 = TREE_OPERAND (t2, 1);
+	z1 = TREE_OPERAND (t1, 2);
+	z2 = TREE_OPERAND (t2, 2);
+
+	ret = compare_ssa_name (x1, x2)
+	      && compare_ssa_name (y1, y2)
+	      && compare_cst_or_decl (z1, z2);
+
+	return return_with_debug (ret);
+      }
+    case ADDR_EXPR:
+      {
+	x1 = TREE_OPERAND (t1, 0);
+	x2 = TREE_OPERAND (t2, 0);
+
+	ret = compare_operand (x1, x2);
+	return return_with_debug (ret);
+      }
+    case SSA_NAME:
+	return compare_ssa_name (t1, t2);
+    case INTEGER_CST:
+    case COMPLEX_CST:
+    case VECTOR_CST:
+    case STRING_CST:
+    case REAL_CST:
+    case FUNCTION_DECL:
+    case VAR_DECL:
+    case FIELD_DECL:
+    case LABEL_DECL:
+    case PARM_DECL:
+    case RESULT_DECL:
+    case CONST_DECL:
+    case BIT_FIELD_REF:
+      return compare_cst_or_decl (t1, t2);
+    default:
       return return_false_with_msg ("Unknown TREE code reached");
     }
 }
diff --git a/gcc/ipa-icf-gimple.h b/gcc/ipa-icf-gimple.h
index cb9b1fc..b2d670d 100644
--- a/gcc/ipa-icf-gimple.h
+++ b/gcc/ipa-icf-gimple.h
@@ -203,7 +203,14 @@ public:
   /* Verifies that tree labels T1 and T2 correspond.  */
   bool compare_tree_ssa_label (tree t1, tree t2);
 
-  /* Function responsible for comparison of handled components T1 and T2.
+  /* Function compare for equality given memory operands T1 and T2.  */
+  bool compare_memory_operand (tree t1, tree t2);
+
+  /* Function compare for equality given trees T1 and T2 which
+     can be either a constant or a declaration type.  */
+  bool compare_cst_or_decl (tree t1, tree t2);
+
+  /* Function responsible for comparison of various operands T1 and T2.
      If these components, from functions FUNC1 and FUNC2, are equal, true
      is returned.  */
   bool compare_operand (tree t1, tree t2);
diff --git a/gcc/testsuite/gcc.dg/ipa/pr63569.c b/gcc/testsuite/gcc.dg/ipa/pr63569.c
new file mode 100644
index 0000000..8bd5c1f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr63569.c
@@ -0,0 +1,32 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-icf-details"  } */
+
+static int f(int t, int *a) __attribute__((noinline));
+
+static int g(int t, volatile int *a) __attribute__((noinline));
+static int g(int t, volatile int *a)
+{
+  int i;
+  int tt = 0;
+  for(i=0;i<t;i++)
+    tt += *a;
+  return tt;
+}
+static int f(int t, int *a)
+{
+  int i;
+  int tt = 0;
+  for(i=0;i<t;i++)
+    tt += *a;
+  return tt;
+}
+
+
+int h(int t, int *a)
+{
+  return f(t, a) + g(t, a);
+}
+
+/* { dg-final { scan-ipa-dump "different operand volatility" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Equal symbols: 0" "icf"  } } */
+/* { dg-final { cleanup-ipa-dump "icf" } } */
-- 
2.1.2


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] IPA ICF: refactoring + fix for PR ipa/63569
  2014-12-10 12:19 [PATCH] IPA ICF: refactoring + fix for PR ipa/63569 Martin Liška
@ 2014-12-11 12:37 ` Richard Biener
  2014-12-17 11:29   ` Martin Liška
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Biener @ 2014-12-11 12:37 UTC (permalink / raw)
  To: Martin Liška; +Cc: GCC Patches, hubi >> Jan Hubicka

On Wed, Dec 10, 2014 at 1:18 PM, Martin Liška <mliska@suse.cz> wrote:
> Hello.
>
> As suggested by Richard, I split compare_operand functions to various
> functions
> related to a specific comparison. Apart from that I added fast check for
> volatility flag that caused miscompilation mentioned in PR63569.
>
> Patch can bootstrap on x86_64-linux-pc without any regression seen and I was
> able to build Firefox with LTO.
>
> Ready for trunk?

Hmm, I don't think the dispatch to compare_memory_operand is at the
correct place.  It should be called from places where currently
compare_operand is called and it should recurse to compare_operand.
That is, it is more "high-level".

Can you please fix the volatile issue separately?  It's also not necessary
to do that check on every operand but just on memory operands.

Thanks,
Richard.

> Thanks,
> Martin

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] IPA ICF: refactoring + fix for PR ipa/63569
  2014-12-11 12:37 ` Richard Biener
@ 2014-12-17 11:29   ` Martin Liška
  2014-12-17 15:41     ` Richard Biener
  0 siblings, 1 reply; 10+ messages in thread
From: Martin Liška @ 2014-12-17 11:29 UTC (permalink / raw)
  To: Richard Biener; +Cc: GCC Patches, hubi >> Jan Hubicka

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

On 12/11/2014 01:37 PM, Richard Biener wrote:
> On Wed, Dec 10, 2014 at 1:18 PM, Martin Liška <mliska@suse.cz> wrote:
>> Hello.
>>
>> As suggested by Richard, I split compare_operand functions to various
>> functions
>> related to a specific comparison. Apart from that I added fast check for
>> volatility flag that caused miscompilation mentioned in PR63569.
>>
>> Patch can bootstrap on x86_64-linux-pc without any regression seen and I was
>> able to build Firefox with LTO.
>>
>> Ready for trunk?
>
> Hmm, I don't think the dispatch to compare_memory_operand is at the
> correct place.  It should be called from places where currently
> compare_operand is called and it should recurse to compare_operand.
> That is, it is more "high-level".
>
> Can you please fix the volatile issue separately?  It's also not necessary
> to do that check on every operand but just on memory operands.

Hello Richard.

I hope the following patch is the finally the right approach we want to do ;)
In the first patch, I did just refactoring for high-level memory operand
comparison and the second of fixes problem connected to missing volatile
attribute comparison.

Patch was retested and can bootstrap.

What do you think about it?
Thank you,
Martin

>
> Thanks,
> Richard.
>
>> Thanks,
>> Martin


[-- Attachment #2: 0001-IPA-ICF-compare_operand-is-split-to-multiple-functio.patch --]
[-- Type: text/x-patch, Size: 13160 bytes --]

From 422dadd7913bf8cce479a035e4680c8ed625d8ef Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Wed, 10 Dec 2014 12:56:48 +0100
Subject: [PATCH 1/2] IPA ICF: compare_operand is split to multiple functions.

gcc/ChangeLog:

2014-12-12  Martin Liska  <mliska@suse.cz>

	PR ipa/63569
	* gimple-walk.h (get_base_loadstore): Declare a new global
	function.
	* gimple-walk.c (get_base_loadstore): Define a new global
	function.
	* ipa-icf-gimple.c (func_checker::compare_ssa_name): Enhance SSA
	name comparison.
	(func_checker::compare_memory_operand): New function.
	(func_checker::compare_operand): Split case to newly
	added functions.
	(func_checker::compare_cst_or_decl): New function.
	(func_checker::compare_gimple_call): Identify
	memory operands.
	(func_checker::compare_gimple_assign): Likewise.
	* ipa-icf-gimple.h: New function.
---
 gcc/gimple-walk.c    |   6 +-
 gcc/gimple-walk.h    |   2 +
 gcc/ipa-icf-gimple.c | 232 +++++++++++++++++++++++++++++----------------------
 gcc/ipa-icf-gimple.h |   9 +-
 4 files changed, 144 insertions(+), 105 deletions(-)

diff --git a/gcc/gimple-walk.c b/gcc/gimple-walk.c
index 959d68e..cdd2631 100644
--- a/gcc/gimple-walk.c
+++ b/gcc/gimple-walk.c
@@ -672,10 +672,10 @@ walk_gimple_stmt (gimple_stmt_iterator *gsi, walk_stmt_fn callback_stmt,
 /* From a tree operand OP return the base of a load or store operation
    or NULL_TREE if OP is not a load or a store.  */
 
-static tree
-get_base_loadstore (tree op)
+tree
+get_base_loadstore (tree op, bool unpack_handled_component)
 {
-  while (handled_component_p (op))
+  while (handled_component_p (op) && unpack_handled_component)
     op = TREE_OPERAND (op, 0);
   if (DECL_P (op)
       || INDIRECT_REF_P (op)
diff --git a/gcc/gimple-walk.h b/gcc/gimple-walk.h
index 5b75fdc..fdc3b1f 100644
--- a/gcc/gimple-walk.h
+++ b/gcc/gimple-walk.h
@@ -97,4 +97,6 @@ extern bool walk_stmt_load_store_addr_ops (gimple, void *,
 extern bool walk_stmt_load_store_ops (gimple, void *,
 				      walk_stmt_load_store_addr_fn,
 				      walk_stmt_load_store_addr_fn);
+extern tree get_base_loadstore (tree op, bool unpack_handled_component = true);
+
 #endif /* GCC_GIMPLE_WALK_H */
diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c
index ec0290a..9f365af 100644
--- a/gcc/ipa-icf-gimple.c
+++ b/gcc/ipa-icf-gimple.c
@@ -46,6 +46,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-dfa.h"
 #include "tree-pass.h"
 #include "gimple-pretty-print.h"
+#include "gimple-walk.h"
 #include "cfgloop.h"
 #include "except.h"
 #include "hash-map.h"
@@ -110,6 +111,9 @@ func_checker::~func_checker ()
 bool
 func_checker::compare_ssa_name (tree t1, tree t2)
 {
+  gcc_assert (TREE_CODE (t1) == SSA_NAME);
+  gcc_assert (TREE_CODE (t2) == SSA_NAME);
+
   unsigned i1 = SSA_NAME_VERSION (t1);
   unsigned i2 = SSA_NAME_VERSION (t2);
 
@@ -123,6 +127,20 @@ func_checker::compare_ssa_name (tree t1, tree t2)
   else if (m_target_ssa_names[i2] != (int) i1)
     return false;
 
+  if (SSA_NAME_IS_DEFAULT_DEF (t1))
+    {
+      tree b1 = SSA_NAME_VAR (t1);
+      tree b2 = SSA_NAME_VAR (t2);
+
+      if (b1 == NULL && b2 == NULL)
+	return true;
+
+      if (b1 == NULL || b2 == NULL || TREE_CODE (b1) != TREE_CODE (b2))
+	return return_false ();
+
+      return compare_cst_or_decl (b1, b2);
+    }
+
   return true;
 }
 
@@ -178,9 +196,10 @@ func_checker::compare_decl (tree t1, tree t2)
 }
 
 /* Return true if types are compatible from perspective of ICF.  */
-bool func_checker::compatible_types_p (tree t1, tree t2,
-				       bool compare_polymorphic,
-				       bool first_argument)
+bool
+func_checker::compatible_types_p (tree t1, tree t2,
+				  bool compare_polymorphic,
+				  bool first_argument)
 {
   if (TREE_CODE (t1) != TREE_CODE (t2))
     return return_false_with_msg ("different tree types");
@@ -211,15 +230,94 @@ bool func_checker::compatible_types_p (tree t1, tree t2,
   return true;
 }
 
-/* Function responsible for comparison of handled components T1 and T2.
+/* Function compare for equality given memory operands T1 and T2.  */
+
+bool
+func_checker::compare_memory_operand (tree t1, tree t2)
+{
+  ao_ref r1, r2;
+  ao_ref_init (&r1, t1);
+  ao_ref_init (&r2, t2);
+  if (ao_ref_alias_set (&r1) != ao_ref_alias_set (&r2)
+      || ao_ref_base_alias_set (&r1) != ao_ref_base_alias_set (&r2))
+    return return_false_with_msg ("ao alias sets are different");
+
+  return compare_operand (t1, t2);
+}
+
+/* Function compare for equality given trees T1 and T2 which
+   can be either a constant or a declaration type.  */
+
+bool
+func_checker::compare_cst_or_decl (tree t1, tree t2)
+{
+  bool ret;
+
+  switch (TREE_CODE (t1))
+    {
+    case INTEGER_CST:
+      {
+	ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
+	      && wi::to_offset  (t1) == wi::to_offset  (t2);
+
+	return return_with_debug (ret);
+      }
+    case COMPLEX_CST:
+    case VECTOR_CST:
+    case STRING_CST:
+    case REAL_CST:
+      {
+	ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
+	return return_with_debug (ret);
+      }
+    case FUNCTION_DECL:
+      {
+	ret = compare_function_decl (t1, t2);
+	return return_with_debug (ret);
+      }
+    case VAR_DECL:
+      return return_with_debug (compare_variable_decl (t1, t2));
+    case FIELD_DECL:
+      {
+	tree offset1 = DECL_FIELD_OFFSET (t1);
+	tree offset2 = DECL_FIELD_OFFSET (t2);
+
+	tree bit_offset1 = DECL_FIELD_BIT_OFFSET (t1);
+	tree bit_offset2 = DECL_FIELD_BIT_OFFSET (t2);
+
+	ret = compare_operand (offset1, offset2)
+	      && compare_operand (bit_offset1, bit_offset2);
+
+	return return_with_debug (ret);
+      }
+    case LABEL_DECL:
+      {
+	int *bb1 = m_label_bb_map.get (t1);
+	int *bb2 = m_label_bb_map.get (t2);
+
+	return return_with_debug (*bb1 == *bb2);
+      }
+    case PARM_DECL:
+    case RESULT_DECL:
+    case CONST_DECL:
+    case BIT_FIELD_REF:
+      {
+	ret = compare_decl (t1, t2);
+	return return_with_debug (ret);
+      }
+    default:
+      gcc_unreachable ();
+    }
+}
+
+/* Function responsible for comparison of various operands T1 and T2.
    If these components, from functions FUNC1 and FUNC2, are equal, true
    is returned.  */
 
 bool
 func_checker::compare_operand (tree t1, tree t2)
 {
-  tree base1, base2, x1, x2, y1, y2, z1, z2;
-  HOST_WIDE_INT offset1 = 0, offset2 = 0;
+  tree x1, x2, y1, y2, z1, z2;
   bool ret;
 
   if (!t1 && !t2)
@@ -233,18 +331,6 @@ func_checker::compare_operand (tree t1, tree t2)
   if (!func_checker::compatible_types_p (tt1, tt2))
     return false;
 
-  base1 = get_addr_base_and_unit_offset (t1, &offset1);
-  base2 = get_addr_base_and_unit_offset (t2, &offset2);
-
-  if (base1 && base2)
-    {
-      if (offset1 != offset2)
-	return return_false_with_msg ("base offsets are different");
-
-      t1 = base1;
-      t2 = base2;
-    }
-
   if (TREE_CODE (t1) != TREE_CODE (t2))
     return return_false ();
 
@@ -267,6 +353,7 @@ func_checker::compare_operand (tree t1, tree t2)
       }
     case ARRAY_REF:
     case ARRAY_RANGE_REF:
+      /* First argument is the array, second is the index.  */
       x1 = TREE_OPERAND (t1, 0);
       x2 = TREE_OPERAND (t2, 0);
       y1 = TREE_OPERAND (t1, 1);
@@ -278,6 +365,7 @@ func_checker::compare_operand (tree t1, tree t2)
       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);
@@ -302,16 +390,6 @@ func_checker::compare_operand (tree t1, tree t2)
 	if (!compare_operand (x1, x2))
 	  return return_false_with_msg ("");
 
-	if (get_alias_set (TREE_TYPE (y1)) != get_alias_set (TREE_TYPE (y2)))
-	  return return_false_with_msg ("alias set for MEM_REF offsets are different");
-
-	ao_ref r1, r2;
-	ao_ref_init (&r1, t1);
-	ao_ref_init (&r2, t2);
-	if (ao_ref_alias_set (&r1) != ao_ref_alias_set (&r2)
-	    || ao_ref_base_alias_set (&r1) != ao_ref_base_alias_set (&r2))
-	  return return_false_with_msg ("ao alias sets are different");
-
 	/* Type of the offset on MEM_REF does not matter.  */
 	return wi::to_offset  (y1) == wi::to_offset  (y2);
       }
@@ -323,7 +401,7 @@ func_checker::compare_operand (tree t1, tree t2)
 	y2 = TREE_OPERAND (t2, 1);
 
 	ret = compare_operand (x1, x2)
-	      && compare_operand (y1, y2);
+	      && compare_cst_or_decl (y1, y2);
 
 	return return_with_debug (ret);
       }
@@ -337,9 +415,9 @@ func_checker::compare_operand (tree t1, tree t2)
 	z1 = TREE_OPERAND (t1, 2);
 	z2 = TREE_OPERAND (t2, 2);
 
-	ret = compare_operand (x1, x2)
-	      && compare_operand (y1, y2)
-	      && compare_operand (z1, z2);
+	ret = compare_ssa_name (x1, x2)
+	      && compare_ssa_name (y1, y2)
+	      && compare_cst_or_decl (z1, z2);
 
 	return return_with_debug (ret);
       }
@@ -352,88 +430,21 @@ func_checker::compare_operand (tree t1, tree t2)
 	return return_with_debug (ret);
       }
     case SSA_NAME:
-      {
-	ret = compare_ssa_name (t1, t2);
-
-	if (!ret)
-	  return return_with_debug (ret);
-
-	if (SSA_NAME_IS_DEFAULT_DEF (t1))
-	  {
-	    tree b1 = SSA_NAME_VAR (t1);
-	    tree b2 = SSA_NAME_VAR (t2);
-
-	    if (b1 == NULL && b2 == NULL)
-	      return true;
-
-	    if (b1 == NULL || b2 == NULL || TREE_CODE (b1) != TREE_CODE (b2))
-	      return return_false ();
-
-	    switch (TREE_CODE (b1))
-	      {
-	      case VAR_DECL:
-		return return_with_debug (compare_variable_decl (t1, t2));
-	      case PARM_DECL:
-	      case RESULT_DECL:
-		ret = compare_decl (b1, b2);
-		return return_with_debug (ret);
-	      default:
-		return return_false_with_msg ("Unknown TREE code reached");
-	      }
-	  }
-	else
-	  return true;
-      }
+	return compare_ssa_name (t1, t2);
     case INTEGER_CST:
-      {
-	ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
-	      && wi::to_offset  (t1) == wi::to_offset  (t2);
-
-	return return_with_debug (ret);
-      }
     case COMPLEX_CST:
     case VECTOR_CST:
     case STRING_CST:
     case REAL_CST:
-      {
-	ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
-	return return_with_debug (ret);
-      }
     case FUNCTION_DECL:
-      {
-	ret = compare_function_decl (t1, t2);
-	return return_with_debug (ret);
-      }
     case VAR_DECL:
-      return return_with_debug (compare_variable_decl (t1, t2));
     case FIELD_DECL:
-      {
-	tree offset1 = DECL_FIELD_OFFSET (t1);
-	tree offset2 = DECL_FIELD_OFFSET (t2);
-
-	tree bit_offset1 = DECL_FIELD_BIT_OFFSET (t1);
-	tree bit_offset2 = DECL_FIELD_BIT_OFFSET (t2);
-
-	ret = compare_operand (offset1, offset2)
-	      && compare_operand (bit_offset1, bit_offset2);
-
-	return return_with_debug (ret);
-      }
     case LABEL_DECL:
-      {
-	int *bb1 = m_label_bb_map.get (t1);
-	int *bb2 = m_label_bb_map.get (t2);
-
-	return return_with_debug (*bb1 == *bb2);
-      }
     case PARM_DECL:
     case RESULT_DECL:
     case CONST_DECL:
     case BIT_FIELD_REF:
-      {
-	ret = compare_decl (t1, t2);
-	return return_with_debug (ret);
-      }
+      return compare_cst_or_decl (t1, t2);
     default:
       return return_false_with_msg ("Unknown TREE code reached");
     }
@@ -700,6 +711,14 @@ func_checker::compare_gimple_call (gcall *s1, gcall *s2)
       t1 = gimple_call_arg (s1, i);
       t2 = gimple_call_arg (s2, i);
 
+      tree load1 = get_base_loadstore (t1, false);
+      tree load2 = get_base_loadstore (t2, false);
+
+      if (load1 && load2 && !compare_memory_operand (t1, t2))
+	return return_false_with_msg ("memory operands are different");
+      else if ((load1 && !load2) || (!load1 && load2))
+	return return_false_with_msg ("");
+
       if (!compare_operand (t1, t2))
 	return false;
     }
@@ -739,6 +758,17 @@ func_checker::compare_gimple_assign (gimple s1, gimple s2)
       arg1 = gimple_op (s1, i);
       arg2 = gimple_op (s2, i);
 
+      if (i == 1)
+        {
+	  tree rhs1 = get_base_loadstore (arg1, false);
+	  tree rhs2 = get_base_loadstore (arg2, false);
+
+	  if (rhs1 && rhs2 && !compare_memory_operand (rhs1, rhs2))
+	    return return_false_with_msg ("memory operands are different");
+	  else if ((rhs1 && !rhs2) || (!rhs1 && rhs2))
+	    return return_false_with_msg ("");
+        }
+
       if (!compare_operand (arg1, arg2))
 	return false;
     }
diff --git a/gcc/ipa-icf-gimple.h b/gcc/ipa-icf-gimple.h
index cb9b1fc..b2d670d 100644
--- a/gcc/ipa-icf-gimple.h
+++ b/gcc/ipa-icf-gimple.h
@@ -203,7 +203,14 @@ public:
   /* Verifies that tree labels T1 and T2 correspond.  */
   bool compare_tree_ssa_label (tree t1, tree t2);
 
-  /* Function responsible for comparison of handled components T1 and T2.
+  /* Function compare for equality given memory operands T1 and T2.  */
+  bool compare_memory_operand (tree t1, tree t2);
+
+  /* Function compare for equality given trees T1 and T2 which
+     can be either a constant or a declaration type.  */
+  bool compare_cst_or_decl (tree t1, tree t2);
+
+  /* Function responsible for comparison of various operands T1 and T2.
      If these components, from functions FUNC1 and FUNC2, are equal, true
      is returned.  */
   bool compare_operand (tree t1, tree t2);
-- 
2.1.2


[-- Attachment #3: 0002-Fix-for-PR-ipa-63569.patch --]
[-- Type: text/x-patch, Size: 2017 bytes --]

From 33736601c62fdef33614f055e8f0e2ef7dc476f0 Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Tue, 16 Dec 2014 16:09:40 +0100
Subject: [PATCH 2/2] Fix for PR ipa/63569.

gcc/testsuite/ChangeLog:

2014-12-16  Martin Liska  <mliska@suse.cz>

	* gcc.dg/ipa/pr63569.c: New test.

gcc/ChangeLog:

2014-12-16  Martin Liska  <mliska@suse.cz>

	* ipa-icf-gimple.c (func_checker::compare_memory_operand):
	Validate volatile flag.
---
 gcc/ipa-icf-gimple.c               |  3 +++
 gcc/testsuite/gcc.dg/ipa/pr63569.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr63569.c

diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c
index 9f365af..ebfa9eb 100644
--- a/gcc/ipa-icf-gimple.c
+++ b/gcc/ipa-icf-gimple.c
@@ -235,6 +235,9 @@ func_checker::compatible_types_p (tree t1, tree t2,
 bool
 func_checker::compare_memory_operand (tree t1, tree t2)
 {
+  if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2))
+    return return_false_with_msg ("different operand volatility");
+
   ao_ref r1, r2;
   ao_ref_init (&r1, t1);
   ao_ref_init (&r2, t2);
diff --git a/gcc/testsuite/gcc.dg/ipa/pr63569.c b/gcc/testsuite/gcc.dg/ipa/pr63569.c
new file mode 100644
index 0000000..8bd5c1f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr63569.c
@@ -0,0 +1,32 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-icf-details"  } */
+
+static int f(int t, int *a) __attribute__((noinline));
+
+static int g(int t, volatile int *a) __attribute__((noinline));
+static int g(int t, volatile int *a)
+{
+  int i;
+  int tt = 0;
+  for(i=0;i<t;i++)
+    tt += *a;
+  return tt;
+}
+static int f(int t, int *a)
+{
+  int i;
+  int tt = 0;
+  for(i=0;i<t;i++)
+    tt += *a;
+  return tt;
+}
+
+
+int h(int t, int *a)
+{
+  return f(t, a) + g(t, a);
+}
+
+/* { dg-final { scan-ipa-dump "different operand volatility" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Equal symbols: 0" "icf"  } } */
+/* { dg-final { cleanup-ipa-dump "icf" } } */
-- 
2.1.2


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] IPA ICF: refactoring + fix for PR ipa/63569
  2014-12-17 11:29   ` Martin Liška
@ 2014-12-17 15:41     ` Richard Biener
  2014-12-18 13:45       ` Martin Liška
  2014-12-18 17:41       ` Martin Liška
  0 siblings, 2 replies; 10+ messages in thread
From: Richard Biener @ 2014-12-17 15:41 UTC (permalink / raw)
  To: Martin Liška; +Cc: GCC Patches, hubi >> Jan Hubicka

On Wed, Dec 17, 2014 at 12:17 PM, Martin Liška <mliska@suse.cz> wrote:
> On 12/11/2014 01:37 PM, Richard Biener wrote:
>>
>> On Wed, Dec 10, 2014 at 1:18 PM, Martin Liška <mliska@suse.cz> wrote:
>>>
>>> Hello.
>>>
>>> As suggested by Richard, I split compare_operand functions to various
>>> functions
>>> related to a specific comparison. Apart from that I added fast check for
>>> volatility flag that caused miscompilation mentioned in PR63569.
>>>
>>> Patch can bootstrap on x86_64-linux-pc without any regression seen and I
>>> was
>>> able to build Firefox with LTO.
>>>
>>> Ready for trunk?
>>
>>
>> Hmm, I don't think the dispatch to compare_memory_operand is at the
>> correct place.  It should be called from places where currently
>> compare_operand is called and it should recurse to compare_operand.
>> That is, it is more "high-level".
>>
>> Can you please fix the volatile issue separately?  It's also not necessary
>> to do that check on every operand but just on memory operands.
>
>
> Hello Richard.
>
> I hope the following patch is the finally the right approach we want to do
> ;)
> In the first patch, I did just refactoring for high-level memory operand
> comparison and the second of fixes problem connected to missing volatile
> attribute comparison.
>
> Patch was retested and can bootstrap.
>
> What do you think about it?

+bool
+func_checker::compare_cst_or_decl (tree t1, tree t2)
+{
...
+    case INTEGER_CST:
+      {
+       ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
+             && wi::to_offset  (t1) == wi::to_offset  (t2);
+
+       return return_with_debug (ret);
+      }
+    case COMPLEX_CST:
+    case VECTOR_CST:
+    case STRING_CST:
+    case REAL_CST:
+      {
+       ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
+       return return_with_debug (ret);

why does the type matter for INTEGER_CSTs but not for other constants?
Also comparing INTEGER_CSTs via to_offset is certainly wrong.  Please
use tree_int_cst_equal_p (t1, t2) instead, or operand_equal_p which would
end up calling tree_int_cst_equal_p.  That said, I'd have commonized all
_CST nodes by

  ret = compatible_types_p (....) && operand_equal_p (...);

+    case CONST_DECL:
+    case BIT_FIELD_REF:
+      {

I'm quite sure BIT_FIELD_REF is spurious here.

Now to the "meat" ...

+      tree load1 = get_base_loadstore (t1, false);
+      tree load2 = get_base_loadstore (t2, false);
+
+      if (load1 && load2 && !compare_memory_operand (t1, t2))
+       return return_false_with_msg ("memory operands are different");
+      else if ((load1 && !load2) || (!load1 && load2))
+       return return_false_with_msg ("");
+

and the similar code for assigns.  The way you introduce the
unpack_handled_component flag to get_base_loadstore makes
it treat x.field as non-memory operand.  That's wrong.  I wonder
why you think you needed that.  Why does

  tree load1= get_base_loadstore (t1);

not work?  Btw, I'd have avoided get_base_loadstore and simply did

  ao_ref_init (&r1, t1);
  ao_ref_init (&r2, t2);
  tree base1 = ao_ref_base (&r1);
  tree base2 = ao_ref_base (&r2);
  if ((DECL_P (base1) || (TREE_CODE (base1) == MEM_REF || TREE_CODE
(base1) == TARGET_MEM_REF))
     && (DECL_P (base2) || (...))
   ...

or rather put this into compare_memory_operand and call that on all operands
that may be memory (TREE_CODE () != SSA_NAME && !is_gimple_min_invariant ()).

I also miss where you handle memory operands on the LHS for calls
and for assignments.

The compare_ssa_name refactoring part is ok to install.

Can you please fix the volatile bug before the refactoring as it looks like
we're going to iterate some more here unless I can find the time to give
it a quick try myself.

Thanks,
Richard.

> Thank you,
> Martin
>
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> Martin
>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] IPA ICF: refactoring + fix for PR ipa/63569
  2014-12-17 15:41     ` Richard Biener
@ 2014-12-18 13:45       ` Martin Liška
  2014-12-19 11:01         ` Richard Biener
  2014-12-18 17:41       ` Martin Liška
  1 sibling, 1 reply; 10+ messages in thread
From: Martin Liška @ 2014-12-18 13:45 UTC (permalink / raw)
  To: Richard Biener; +Cc: GCC Patches, hubi >> Jan Hubicka

[-- Attachment #1: Type: text/plain, Size: 4235 bytes --]

On 12/17/2014 04:23 PM, Richard Biener wrote:
> On Wed, Dec 17, 2014 at 12:17 PM, Martin Liška <mliska@suse.cz> wrote:
>> On 12/11/2014 01:37 PM, Richard Biener wrote:
>>>
>>> On Wed, Dec 10, 2014 at 1:18 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>> Hello.
>>>>
>>>> As suggested by Richard, I split compare_operand functions to various
>>>> functions
>>>> related to a specific comparison. Apart from that I added fast check for
>>>> volatility flag that caused miscompilation mentioned in PR63569.
>>>>
>>>> Patch can bootstrap on x86_64-linux-pc without any regression seen and I
>>>> was
>>>> able to build Firefox with LTO.
>>>>
>>>> Ready for trunk?
>>>
>>>
>>> Hmm, I don't think the dispatch to compare_memory_operand is at the
>>> correct place.  It should be called from places where currently
>>> compare_operand is called and it should recurse to compare_operand.
>>> That is, it is more "high-level".
>>>
>>> Can you please fix the volatile issue separately?  It's also not necessary
>>> to do that check on every operand but just on memory operands.
>>
>>
>> Hello Richard.
>>
>> I hope the following patch is the finally the right approach we want to do
>> ;)
>> In the first patch, I did just refactoring for high-level memory operand
>> comparison and the second of fixes problem connected to missing volatile
>> attribute comparison.
>>
>> Patch was retested and can bootstrap.
>>
>> What do you think about it?
>
> +bool
> +func_checker::compare_cst_or_decl (tree t1, tree t2)
> +{
> ...
> +    case INTEGER_CST:
> +      {
> +       ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
> +             && wi::to_offset  (t1) == wi::to_offset  (t2);
> +
> +       return return_with_debug (ret);
> +      }
> +    case COMPLEX_CST:
> +    case VECTOR_CST:
> +    case STRING_CST:
> +    case REAL_CST:
> +      {
> +       ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
> +       return return_with_debug (ret);
>
> why does the type matter for INTEGER_CSTs but not for other constants?
> Also comparing INTEGER_CSTs via to_offset is certainly wrong.  Please
> use tree_int_cst_equal_p (t1, t2) instead, or operand_equal_p which would
> end up calling tree_int_cst_equal_p.  That said, I'd have commonized all
> _CST nodes by
>
>    ret = compatible_types_p (....) && operand_equal_p (...);
>
> +    case CONST_DECL:
> +    case BIT_FIELD_REF:
> +      {
>
> I'm quite sure BIT_FIELD_REF is spurious here.
>
> Now to the "meat" ...
>
> +      tree load1 = get_base_loadstore (t1, false);
> +      tree load2 = get_base_loadstore (t2, false);
> +
> +      if (load1 && load2 && !compare_memory_operand (t1, t2))
> +       return return_false_with_msg ("memory operands are different");
> +      else if ((load1 && !load2) || (!load1 && load2))
> +       return return_false_with_msg ("");
> +
>
> and the similar code for assigns.  The way you introduce the
> unpack_handled_component flag to get_base_loadstore makes
> it treat x.field as non-memory operand.  That's wrong.  I wonder
> why you think you needed that.  Why does
>
>    tree load1= get_base_loadstore (t1);
>
> not work?  Btw, I'd have avoided get_base_loadstore and simply did
>
>    ao_ref_init (&r1, t1);
>    ao_ref_init (&r2, t2);
>    tree base1 = ao_ref_base (&r1);
>    tree base2 = ao_ref_base (&r2);
>    if ((DECL_P (base1) || (TREE_CODE (base1) == MEM_REF || TREE_CODE
> (base1) == TARGET_MEM_REF))
>       && (DECL_P (base2) || (...))
>     ...
>
> or rather put this into compare_memory_operand and call that on all operands
> that may be memory (TREE_CODE () != SSA_NAME && !is_gimple_min_invariant ()).
>
> I also miss where you handle memory operands on the LHS for calls
> and for assignments.
>
> The compare_ssa_name refactoring part is ok to install.
>
> Can you please fix the volatile bug before the refactoring as it looks like
> we're going to iterate some more here unless I can find the time to give
> it a quick try myself.

Hi.

Sure, there's minimalistic patch which fixes the PR.
Can I install this change?

I'm going to apply your notes that are orthogonal to the PR.

Thanks,
Martin

>
> Thanks,
> Richard.
>
>> Thank you,
>> Martin
>>
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Thanks,
>>>> Martin
>>
>>


[-- Attachment #2: 0001-Fix-for-PR-ipa-63569.patch --]
[-- Type: text/x-patch, Size: 2053 bytes --]

From b984eaae263eedc8beb2413f4739d3574f46d63d Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Thu, 18 Dec 2014 14:16:12 +0100
Subject: [PATCH 1/2] Fix for PR ipa/63569.

gcc/testsuite/ChangeLog:

2014-12-18  Martin Liska  <mliska@suse.cz>

	PR ipa/63569
	* gcc.dg/ipa/pr63569.c: New test.

gcc/ChangeLog:

2014-12-18  Martin Liska  <mliska@suse.cz>

	PR ipa/63569
	* ipa-icf-gimple.c (func_checker::compare_operand): Add missing
	comparison for volatile flag.
---
 gcc/ipa-icf-gimple.c               |  3 +++
 gcc/testsuite/gcc.dg/ipa/pr63569.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr63569.c

diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c
index ec0290a..fa2c353 100644
--- a/gcc/ipa-icf-gimple.c
+++ b/gcc/ipa-icf-gimple.c
@@ -230,6 +230,9 @@ func_checker::compare_operand (tree t1, tree t2)
   tree tt1 = TREE_TYPE (t1);
   tree tt2 = TREE_TYPE (t2);
 
+  if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2))
+    return return_false_with_msg ("different operand volatility");
+
   if (!func_checker::compatible_types_p (tt1, tt2))
     return false;
 
diff --git a/gcc/testsuite/gcc.dg/ipa/pr63569.c b/gcc/testsuite/gcc.dg/ipa/pr63569.c
new file mode 100644
index 0000000..8bd5c1f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr63569.c
@@ -0,0 +1,32 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-icf-details"  } */
+
+static int f(int t, int *a) __attribute__((noinline));
+
+static int g(int t, volatile int *a) __attribute__((noinline));
+static int g(int t, volatile int *a)
+{
+  int i;
+  int tt = 0;
+  for(i=0;i<t;i++)
+    tt += *a;
+  return tt;
+}
+static int f(int t, int *a)
+{
+  int i;
+  int tt = 0;
+  for(i=0;i<t;i++)
+    tt += *a;
+  return tt;
+}
+
+
+int h(int t, int *a)
+{
+  return f(t, a) + g(t, a);
+}
+
+/* { dg-final { scan-ipa-dump "different operand volatility" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Equal symbols: 0" "icf"  } } */
+/* { dg-final { cleanup-ipa-dump "icf" } } */
-- 
2.1.2


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] IPA ICF: refactoring + fix for PR ipa/63569
  2014-12-17 15:41     ` Richard Biener
  2014-12-18 13:45       ` Martin Liška
@ 2014-12-18 17:41       ` Martin Liška
  2014-12-19 11:11         ` Richard Biener
  1 sibling, 1 reply; 10+ messages in thread
From: Martin Liška @ 2014-12-18 17:41 UTC (permalink / raw)
  To: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 4131 bytes --]

On 12/17/2014 04:23 PM, Richard Biener wrote:
> On Wed, Dec 17, 2014 at 12:17 PM, Martin Liška <mliska@suse.cz> wrote:
>> On 12/11/2014 01:37 PM, Richard Biener wrote:
>>>
>>> On Wed, Dec 10, 2014 at 1:18 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>> Hello.
>>>>
>>>> As suggested by Richard, I split compare_operand functions to various
>>>> functions
>>>> related to a specific comparison. Apart from that I added fast check for
>>>> volatility flag that caused miscompilation mentioned in PR63569.
>>>>
>>>> Patch can bootstrap on x86_64-linux-pc without any regression seen and I
>>>> was
>>>> able to build Firefox with LTO.
>>>>
>>>> Ready for trunk?
>>>
>>>
>>> Hmm, I don't think the dispatch to compare_memory_operand is at the
>>> correct place.  It should be called from places where currently
>>> compare_operand is called and it should recurse to compare_operand.
>>> That is, it is more "high-level".
>>>
>>> Can you please fix the volatile issue separately?  It's also not necessary
>>> to do that check on every operand but just on memory operands.
>>
>>
>> Hello Richard.
>>
>> I hope the following patch is the finally the right approach we want to do
>> ;)
>> In the first patch, I did just refactoring for high-level memory operand
>> comparison and the second of fixes problem connected to missing volatile
>> attribute comparison.
>>
>> Patch was retested and can bootstrap.
>>
>> What do you think about it?
>
> +bool
> +func_checker::compare_cst_or_decl (tree t1, tree t2)
> +{
> ...
> +    case INTEGER_CST:
> +      {
> +       ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
> +             && wi::to_offset  (t1) == wi::to_offset  (t2);
> +
> +       return return_with_debug (ret);
> +      }
> +    case COMPLEX_CST:
> +    case VECTOR_CST:
> +    case STRING_CST:
> +    case REAL_CST:
> +      {
> +       ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
> +       return return_with_debug (ret);
>
> why does the type matter for INTEGER_CSTs but not for other constants?
> Also comparing INTEGER_CSTs via to_offset is certainly wrong.  Please
> use tree_int_cst_equal_p (t1, t2) instead, or operand_equal_p which would
> end up calling tree_int_cst_equal_p.  That said, I'd have commonized all
> _CST nodes by
>
>    ret = compatible_types_p (....) && operand_equal_p (...);
>
> +    case CONST_DECL:
> +    case BIT_FIELD_REF:
> +      {
>
> I'm quite sure BIT_FIELD_REF is spurious here.
>
> Now to the "meat" ...
>
> +      tree load1 = get_base_loadstore (t1, false);
> +      tree load2 = get_base_loadstore (t2, false);
> +
> +      if (load1 && load2 && !compare_memory_operand (t1, t2))
> +       return return_false_with_msg ("memory operands are different");
> +      else if ((load1 && !load2) || (!load1 && load2))
> +       return return_false_with_msg ("");
> +
>
> and the similar code for assigns.  The way you introduce the
> unpack_handled_component flag to get_base_loadstore makes
> it treat x.field as non-memory operand.  That's wrong.  I wonder
> why you think you needed that.  Why does
>
>    tree load1= get_base_loadstore (t1);
>
> not work?  Btw, I'd have avoided get_base_loadstore and simply did
>
>    ao_ref_init (&r1, t1);
>    ao_ref_init (&r2, t2);
>    tree base1 = ao_ref_base (&r1);
>    tree base2 = ao_ref_base (&r2);
>    if ((DECL_P (base1) || (TREE_CODE (base1) == MEM_REF || TREE_CODE
> (base1) == TARGET_MEM_REF))
>       && (DECL_P (base2) || (...))
>     ...
>
> or rather put this into compare_memory_operand and call that on all operands
> that may be memory (TREE_CODE () != SSA_NAME && !is_gimple_min_invariant ()).
>
> I also miss where you handle memory operands on the LHS for calls
> and for assignments.
>
> The compare_ssa_name refactoring part is ok to install.
>
> Can you please fix the volatile bug before the refactoring as it looks like
> we're going to iterate some more here unless I can find the time to give
> it a quick try myself.
>
> Thanks,
> Richard.
>

Hi.

There's second part of the patch set.

Thanks,
Martin

>> Thank you,
>> Martin
>>
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Thanks,
>>>> Martin
>>
>>


[-- Attachment #2: 0002-IPA-ICF-compare_operand-is-split-to-multiple-functio.patch --]
[-- Type: text/x-patch, Size: 12111 bytes --]

From 288fb5fa6c78cf5a323c7667b180aed2b4d7e346 Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Thu, 18 Dec 2014 14:22:51 +0100
Subject: [PATCH 2/2] IPA ICF: compare_operand is split to multiple functions.

gcc/ChangeLog

2014-12-12  Martin Liska  <mliska@suse.cz>

	* ipa-icf-gimple.c (func_checker::compare_ssa_name): Enhance SSA
	name comparison.
	(func_checker::compare_memory_operand): New function.
	(func_checker::compare_operand): Split case to newly
	added functions.
	(func_checker::compare_cst_or_decl): New function.
	(func_checker::compare_gimple_call): Identify
	memory operands.
	(func_checker::compare_gimple_assign): Likewise.
	* ipa-icf-gimple.h: New function.
---
 gcc/ipa-icf-gimple.c | 243 ++++++++++++++++++++++++++++-----------------------
 gcc/ipa-icf-gimple.h |   9 +-
 2 files changed, 142 insertions(+), 110 deletions(-)

diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c
index fa2c353..2530a19 100644
--- a/gcc/ipa-icf-gimple.c
+++ b/gcc/ipa-icf-gimple.c
@@ -110,6 +110,9 @@ func_checker::~func_checker ()
 bool
 func_checker::compare_ssa_name (tree t1, tree t2)
 {
+  gcc_assert (TREE_CODE (t1) == SSA_NAME);
+  gcc_assert (TREE_CODE (t2) == SSA_NAME);
+
   unsigned i1 = SSA_NAME_VERSION (t1);
   unsigned i2 = SSA_NAME_VERSION (t2);
 
@@ -123,6 +126,20 @@ func_checker::compare_ssa_name (tree t1, tree t2)
   else if (m_target_ssa_names[i2] != (int) i1)
     return false;
 
+  if (SSA_NAME_IS_DEFAULT_DEF (t1))
+    {
+      tree b1 = SSA_NAME_VAR (t1);
+      tree b2 = SSA_NAME_VAR (t2);
+
+      if (b1 == NULL && b2 == NULL)
+	return true;
+
+      if (b1 == NULL || b2 == NULL || TREE_CODE (b1) != TREE_CODE (b2))
+	return return_false ();
+
+      return compare_cst_or_decl (b1, b2);
+    }
+
   return true;
 }
 
@@ -178,9 +195,10 @@ func_checker::compare_decl (tree t1, tree t2)
 }
 
 /* Return true if types are compatible from perspective of ICF.  */
-bool func_checker::compatible_types_p (tree t1, tree t2,
-				       bool compare_polymorphic,
-				       bool first_argument)
+bool
+func_checker::compatible_types_p (tree t1, tree t2,
+				  bool compare_polymorphic,
+				  bool first_argument)
 {
   if (TREE_CODE (t1) != TREE_CODE (t2))
     return return_false_with_msg ("different tree types");
@@ -211,15 +229,108 @@ bool func_checker::compatible_types_p (tree t1, tree t2,
   return true;
 }
 
-/* Function responsible for comparison of handled components T1 and T2.
+/* Function compare for equality given memory operands T1 and T2.  */
+
+bool
+func_checker::compare_memory_operand (tree t1, tree t2)
+{
+  if (!t1 && !t2)
+    return true;
+  else if (!t1 || !t2)
+    return false;
+
+  if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2))
+    return return_false_with_msg ("different operand volatility");
+
+  bool source_is_memop = DECL_P (t1) || INDIRECT_REF_P (t1)
+			 || TREE_CODE (t1) == MEM_REF
+			 || TREE_CODE (t1) == TARGET_MEM_REF;
+
+  bool target_is_memop = DECL_P (t2) || INDIRECT_REF_P (t2)
+			 || TREE_CODE (t2) == MEM_REF
+			 || TREE_CODE (t2) == TARGET_MEM_REF;
+
+  /* Compare alias sets for memory operands.  */
+  if (source_is_memop && target_is_memop)
+    {
+      ao_ref r1, r2;
+      ao_ref_init (&r1, t1);
+      ao_ref_init (&r2, t2);
+      if (ao_ref_alias_set (&r1) != ao_ref_alias_set (&r2)
+	  || ao_ref_base_alias_set (&r1) != ao_ref_base_alias_set (&r2))
+	return return_false_with_msg ("ao alias sets are different");
+    }
+
+  return compare_operand (t1, t2);
+}
+
+/* Function compare for equality given trees T1 and T2 which
+   can be either a constant or a declaration type.  */
+
+bool
+func_checker::compare_cst_or_decl (tree t1, tree t2)
+{
+  bool ret;
+
+  switch (TREE_CODE (t1))
+    {
+    case INTEGER_CST:
+    case COMPLEX_CST:
+    case VECTOR_CST:
+    case STRING_CST:
+    case REAL_CST:
+      {
+	ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
+	      && operand_equal_p (t1, t2, OEP_ONLY_CONST);
+	return return_with_debug (ret);
+      }
+    case FUNCTION_DECL:
+      {
+	ret = compare_function_decl (t1, t2);
+	return return_with_debug (ret);
+      }
+    case VAR_DECL:
+      return return_with_debug (compare_variable_decl (t1, t2));
+    case FIELD_DECL:
+      {
+	tree offset1 = DECL_FIELD_OFFSET (t1);
+	tree offset2 = DECL_FIELD_OFFSET (t2);
+
+	tree bit_offset1 = DECL_FIELD_BIT_OFFSET (t1);
+	tree bit_offset2 = DECL_FIELD_BIT_OFFSET (t2);
+
+	ret = compare_operand (offset1, offset2)
+	      && compare_operand (bit_offset1, bit_offset2);
+
+	return return_with_debug (ret);
+      }
+    case LABEL_DECL:
+      {
+	int *bb1 = m_label_bb_map.get (t1);
+	int *bb2 = m_label_bb_map.get (t2);
+
+	return return_with_debug (*bb1 == *bb2);
+      }
+    case PARM_DECL:
+    case RESULT_DECL:
+    case CONST_DECL:
+      {
+	ret = compare_decl (t1, t2);
+	return return_with_debug (ret);
+      }
+    default:
+      gcc_unreachable ();
+    }
+}
+
+/* Function responsible for comparison of various operands T1 and T2.
    If these components, from functions FUNC1 and FUNC2, are equal, true
    is returned.  */
 
 bool
 func_checker::compare_operand (tree t1, tree t2)
 {
-  tree base1, base2, x1, x2, y1, y2, z1, z2;
-  HOST_WIDE_INT offset1 = 0, offset2 = 0;
+  tree x1, x2, y1, y2, z1, z2;
   bool ret;
 
   if (!t1 && !t2)
@@ -230,24 +341,9 @@ func_checker::compare_operand (tree t1, tree t2)
   tree tt1 = TREE_TYPE (t1);
   tree tt2 = TREE_TYPE (t2);
 
-  if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2))
-    return return_false_with_msg ("different operand volatility");
-
   if (!func_checker::compatible_types_p (tt1, tt2))
     return false;
 
-  base1 = get_addr_base_and_unit_offset (t1, &offset1);
-  base2 = get_addr_base_and_unit_offset (t2, &offset2);
-
-  if (base1 && base2)
-    {
-      if (offset1 != offset2)
-	return return_false_with_msg ("base offsets are different");
-
-      t1 = base1;
-      t2 = base2;
-    }
-
   if (TREE_CODE (t1) != TREE_CODE (t2))
     return return_false ();
 
@@ -270,6 +366,7 @@ func_checker::compare_operand (tree t1, tree t2)
       }
     case ARRAY_REF:
     case ARRAY_RANGE_REF:
+      /* First argument is the array, second is the index.  */
       x1 = TREE_OPERAND (t1, 0);
       x2 = TREE_OPERAND (t2, 0);
       y1 = TREE_OPERAND (t1, 1);
@@ -281,6 +378,7 @@ func_checker::compare_operand (tree t1, tree t2)
       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);
@@ -305,16 +403,6 @@ func_checker::compare_operand (tree t1, tree t2)
 	if (!compare_operand (x1, x2))
 	  return return_false_with_msg ("");
 
-	if (get_alias_set (TREE_TYPE (y1)) != get_alias_set (TREE_TYPE (y2)))
-	  return return_false_with_msg ("alias set for MEM_REF offsets are different");
-
-	ao_ref r1, r2;
-	ao_ref_init (&r1, t1);
-	ao_ref_init (&r2, t2);
-	if (ao_ref_alias_set (&r1) != ao_ref_alias_set (&r2)
-	    || ao_ref_base_alias_set (&r1) != ao_ref_base_alias_set (&r2))
-	  return return_false_with_msg ("ao alias sets are different");
-
 	/* Type of the offset on MEM_REF does not matter.  */
 	return wi::to_offset  (y1) == wi::to_offset  (y2);
       }
@@ -326,7 +414,7 @@ func_checker::compare_operand (tree t1, tree t2)
 	y2 = TREE_OPERAND (t2, 1);
 
 	ret = compare_operand (x1, x2)
-	      && compare_operand (y1, y2);
+	      && compare_cst_or_decl (y1, y2);
 
 	return return_with_debug (ret);
       }
@@ -340,9 +428,9 @@ func_checker::compare_operand (tree t1, tree t2)
 	z1 = TREE_OPERAND (t1, 2);
 	z2 = TREE_OPERAND (t2, 2);
 
-	ret = compare_operand (x1, x2)
-	      && compare_operand (y1, y2)
-	      && compare_operand (z1, z2);
+	ret = compare_ssa_name (x1, x2)
+	      && compare_ssa_name (y1, y2)
+	      && compare_cst_or_decl (z1, z2);
 
 	return return_with_debug (ret);
       }
@@ -354,89 +442,26 @@ func_checker::compare_operand (tree t1, tree t2)
 	ret = compare_operand (x1, x2);
 	return return_with_debug (ret);
       }
-    case SSA_NAME:
-      {
-	ret = compare_ssa_name (t1, t2);
-
-	if (!ret)
-	  return return_with_debug (ret);
-
-	if (SSA_NAME_IS_DEFAULT_DEF (t1))
-	  {
-	    tree b1 = SSA_NAME_VAR (t1);
-	    tree b2 = SSA_NAME_VAR (t2);
-
-	    if (b1 == NULL && b2 == NULL)
-	      return true;
-
-	    if (b1 == NULL || b2 == NULL || TREE_CODE (b1) != TREE_CODE (b2))
-	      return return_false ();
-
-	    switch (TREE_CODE (b1))
-	      {
-	      case VAR_DECL:
-		return return_with_debug (compare_variable_decl (t1, t2));
-	      case PARM_DECL:
-	      case RESULT_DECL:
-		ret = compare_decl (b1, b2);
-		return return_with_debug (ret);
-	      default:
-		return return_false_with_msg ("Unknown TREE code reached");
-	      }
-	  }
-	else
-	  return true;
-      }
-    case INTEGER_CST:
+    case BIT_FIELD_REF:
       {
-	ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
-	      && wi::to_offset  (t1) == wi::to_offset  (t2);
-
+	ret = compare_decl (t1, t2);
 	return return_with_debug (ret);
       }
+    case SSA_NAME:
+	return compare_ssa_name (t1, t2);
+    case INTEGER_CST:
     case COMPLEX_CST:
     case VECTOR_CST:
     case STRING_CST:
     case REAL_CST:
-      {
-	ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
-	return return_with_debug (ret);
-      }
     case FUNCTION_DECL:
-      {
-	ret = compare_function_decl (t1, t2);
-	return return_with_debug (ret);
-      }
     case VAR_DECL:
-      return return_with_debug (compare_variable_decl (t1, t2));
     case FIELD_DECL:
-      {
-	tree offset1 = DECL_FIELD_OFFSET (t1);
-	tree offset2 = DECL_FIELD_OFFSET (t2);
-
-	tree bit_offset1 = DECL_FIELD_BIT_OFFSET (t1);
-	tree bit_offset2 = DECL_FIELD_BIT_OFFSET (t2);
-
-	ret = compare_operand (offset1, offset2)
-	      && compare_operand (bit_offset1, bit_offset2);
-
-	return return_with_debug (ret);
-      }
     case LABEL_DECL:
-      {
-	int *bb1 = m_label_bb_map.get (t1);
-	int *bb2 = m_label_bb_map.get (t2);
-
-	return return_with_debug (*bb1 == *bb2);
-      }
     case PARM_DECL:
     case RESULT_DECL:
     case CONST_DECL:
-    case BIT_FIELD_REF:
-      {
-	ret = compare_decl (t1, t2);
-	return return_with_debug (ret);
-      }
+      return compare_cst_or_decl (t1, t2);
     default:
       return return_false_with_msg ("Unknown TREE code reached");
     }
@@ -703,15 +728,15 @@ func_checker::compare_gimple_call (gcall *s1, gcall *s2)
       t1 = gimple_call_arg (s1, i);
       t2 = gimple_call_arg (s2, i);
 
-      if (!compare_operand (t1, t2))
-	return false;
+      if (!compare_memory_operand (t1, t2))
+	return return_false_with_msg ("memory operands are different");
     }
 
   /* Return value checking.  */
   t1 = gimple_get_lhs (s1);
   t2 = gimple_get_lhs (s2);
 
-  return compare_operand (t1, t2);
+  return compare_memory_operand (t1, t2);
 }
 
 
@@ -742,8 +767,8 @@ func_checker::compare_gimple_assign (gimple s1, gimple s2)
       arg1 = gimple_op (s1, i);
       arg2 = gimple_op (s2, i);
 
-      if (!compare_operand (arg1, arg2))
-	return false;
+      if (!compare_memory_operand (arg1, arg2))
+	return return_false_with_msg ("memory operands are different");
     }
 
 
diff --git a/gcc/ipa-icf-gimple.h b/gcc/ipa-icf-gimple.h
index cb9b1fc..b2d670d 100644
--- a/gcc/ipa-icf-gimple.h
+++ b/gcc/ipa-icf-gimple.h
@@ -203,7 +203,14 @@ public:
   /* Verifies that tree labels T1 and T2 correspond.  */
   bool compare_tree_ssa_label (tree t1, tree t2);
 
-  /* Function responsible for comparison of handled components T1 and T2.
+  /* Function compare for equality given memory operands T1 and T2.  */
+  bool compare_memory_operand (tree t1, tree t2);
+
+  /* Function compare for equality given trees T1 and T2 which
+     can be either a constant or a declaration type.  */
+  bool compare_cst_or_decl (tree t1, tree t2);
+
+  /* Function responsible for comparison of various operands T1 and T2.
      If these components, from functions FUNC1 and FUNC2, are equal, true
      is returned.  */
   bool compare_operand (tree t1, tree t2);
-- 
2.1.2


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] IPA ICF: refactoring + fix for PR ipa/63569
  2014-12-18 13:45       ` Martin Liška
@ 2014-12-19 11:01         ` Richard Biener
  0 siblings, 0 replies; 10+ messages in thread
From: Richard Biener @ 2014-12-19 11:01 UTC (permalink / raw)
  To: Martin Liška; +Cc: GCC Patches, hubi >> Jan Hubicka

On Thu, Dec 18, 2014 at 2:39 PM, Martin Liška <mliska@suse.cz> wrote:
> On 12/17/2014 04:23 PM, Richard Biener wrote:
>>
>> On Wed, Dec 17, 2014 at 12:17 PM, Martin Liška <mliska@suse.cz> wrote:
>>>
>>> On 12/11/2014 01:37 PM, Richard Biener wrote:
>>>>
>>>>
>>>> On Wed, Dec 10, 2014 at 1:18 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>>
>>>>>
>>>>> Hello.
>>>>>
>>>>> As suggested by Richard, I split compare_operand functions to various
>>>>> functions
>>>>> related to a specific comparison. Apart from that I added fast check
>>>>> for
>>>>> volatility flag that caused miscompilation mentioned in PR63569.
>>>>>
>>>>> Patch can bootstrap on x86_64-linux-pc without any regression seen and
>>>>> I
>>>>> was
>>>>> able to build Firefox with LTO.
>>>>>
>>>>> Ready for trunk?
>>>>
>>>>
>>>>
>>>> Hmm, I don't think the dispatch to compare_memory_operand is at the
>>>> correct place.  It should be called from places where currently
>>>> compare_operand is called and it should recurse to compare_operand.
>>>> That is, it is more "high-level".
>>>>
>>>> Can you please fix the volatile issue separately?  It's also not
>>>> necessary
>>>> to do that check on every operand but just on memory operands.
>>>
>>>
>>>
>>> Hello Richard.
>>>
>>> I hope the following patch is the finally the right approach we want to
>>> do
>>> ;)
>>> In the first patch, I did just refactoring for high-level memory operand
>>> comparison and the second of fixes problem connected to missing volatile
>>> attribute comparison.
>>>
>>> Patch was retested and can bootstrap.
>>>
>>> What do you think about it?
>>
>>
>> +bool
>> +func_checker::compare_cst_or_decl (tree t1, tree t2)
>> +{
>> ...
>> +    case INTEGER_CST:
>> +      {
>> +       ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
>> +             && wi::to_offset  (t1) == wi::to_offset  (t2);
>> +
>> +       return return_with_debug (ret);
>> +      }
>> +    case COMPLEX_CST:
>> +    case VECTOR_CST:
>> +    case STRING_CST:
>> +    case REAL_CST:
>> +      {
>> +       ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
>> +       return return_with_debug (ret);
>>
>> why does the type matter for INTEGER_CSTs but not for other constants?
>> Also comparing INTEGER_CSTs via to_offset is certainly wrong.  Please
>> use tree_int_cst_equal_p (t1, t2) instead, or operand_equal_p which would
>> end up calling tree_int_cst_equal_p.  That said, I'd have commonized all
>> _CST nodes by
>>
>>    ret = compatible_types_p (....) && operand_equal_p (...);
>>
>> +    case CONST_DECL:
>> +    case BIT_FIELD_REF:
>> +      {
>>
>> I'm quite sure BIT_FIELD_REF is spurious here.
>>
>> Now to the "meat" ...
>>
>> +      tree load1 = get_base_loadstore (t1, false);
>> +      tree load2 = get_base_loadstore (t2, false);
>> +
>> +      if (load1 && load2 && !compare_memory_operand (t1, t2))
>> +       return return_false_with_msg ("memory operands are different");
>> +      else if ((load1 && !load2) || (!load1 && load2))
>> +       return return_false_with_msg ("");
>> +
>>
>> and the similar code for assigns.  The way you introduce the
>> unpack_handled_component flag to get_base_loadstore makes
>> it treat x.field as non-memory operand.  That's wrong.  I wonder
>> why you think you needed that.  Why does
>>
>>    tree load1= get_base_loadstore (t1);
>>
>> not work?  Btw, I'd have avoided get_base_loadstore and simply did
>>
>>    ao_ref_init (&r1, t1);
>>    ao_ref_init (&r2, t2);
>>    tree base1 = ao_ref_base (&r1);
>>    tree base2 = ao_ref_base (&r2);
>>    if ((DECL_P (base1) || (TREE_CODE (base1) == MEM_REF || TREE_CODE
>> (base1) == TARGET_MEM_REF))
>>       && (DECL_P (base2) || (...))
>>     ...
>>
>> or rather put this into compare_memory_operand and call that on all
>> operands
>> that may be memory (TREE_CODE () != SSA_NAME && !is_gimple_min_invariant
>> ()).
>>
>> I also miss where you handle memory operands on the LHS for calls
>> and for assignments.
>>
>> The compare_ssa_name refactoring part is ok to install.
>>
>> Can you please fix the volatile bug before the refactoring as it looks
>> like
>> we're going to iterate some more here unless I can find the time to give
>> it a quick try myself.
>
>
> Hi.
>
> Sure, there's minimalistic patch which fixes the PR.
> Can I install this change?

Ok.

Thanks,
Richard.

> I'm going to apply your notes that are orthogonal to the PR.
>
> Thanks,
> Martin
>
>
>>
>> Thanks,
>> Richard.
>>
>>> Thank you,
>>> Martin
>>>
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> Thanks,
>>>>> Martin
>>>
>>>
>>>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] IPA ICF: refactoring + fix for PR ipa/63569
  2014-12-18 17:41       ` Martin Liška
@ 2014-12-19 11:11         ` Richard Biener
  2015-01-05 13:12           ` Martin Liška
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Biener @ 2014-12-19 11:11 UTC (permalink / raw)
  To: Martin Liška; +Cc: GCC Patches

On Thu, Dec 18, 2014 at 6:38 PM, Martin Liška <mliska@suse.cz> wrote:
> On 12/17/2014 04:23 PM, Richard Biener wrote:
>>
>> On Wed, Dec 17, 2014 at 12:17 PM, Martin Liška <mliska@suse.cz> wrote:
>>>
>>> On 12/11/2014 01:37 PM, Richard Biener wrote:
>>>>
>>>>
>>>> On Wed, Dec 10, 2014 at 1:18 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>>
>>>>>
>>>>> Hello.
>>>>>
>>>>> As suggested by Richard, I split compare_operand functions to various
>>>>> functions
>>>>> related to a specific comparison. Apart from that I added fast check
>>>>> for
>>>>> volatility flag that caused miscompilation mentioned in PR63569.
>>>>>
>>>>> Patch can bootstrap on x86_64-linux-pc without any regression seen and
>>>>> I
>>>>> was
>>>>> able to build Firefox with LTO.
>>>>>
>>>>> Ready for trunk?
>>>>
>>>>
>>>>
>>>> Hmm, I don't think the dispatch to compare_memory_operand is at the
>>>> correct place.  It should be called from places where currently
>>>> compare_operand is called and it should recurse to compare_operand.
>>>> That is, it is more "high-level".
>>>>
>>>> Can you please fix the volatile issue separately?  It's also not
>>>> necessary
>>>> to do that check on every operand but just on memory operands.
>>>
>>>
>>>
>>> Hello Richard.
>>>
>>> I hope the following patch is the finally the right approach we want to
>>> do
>>> ;)
>>> In the first patch, I did just refactoring for high-level memory operand
>>> comparison and the second of fixes problem connected to missing volatile
>>> attribute comparison.
>>>
>>> Patch was retested and can bootstrap.
>>>
>>> What do you think about it?
>>
>>
>> +bool
>> +func_checker::compare_cst_or_decl (tree t1, tree t2)
>> +{
>> ...
>> +    case INTEGER_CST:
>> +      {
>> +       ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
>> +             && wi::to_offset  (t1) == wi::to_offset  (t2);
>> +
>> +       return return_with_debug (ret);
>> +      }
>> +    case COMPLEX_CST:
>> +    case VECTOR_CST:
>> +    case STRING_CST:
>> +    case REAL_CST:
>> +      {
>> +       ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
>> +       return return_with_debug (ret);
>>
>> why does the type matter for INTEGER_CSTs but not for other constants?
>> Also comparing INTEGER_CSTs via to_offset is certainly wrong.  Please
>> use tree_int_cst_equal_p (t1, t2) instead, or operand_equal_p which would
>> end up calling tree_int_cst_equal_p.  That said, I'd have commonized all
>> _CST nodes by
>>
>>    ret = compatible_types_p (....) && operand_equal_p (...);
>>
>> +    case CONST_DECL:
>> +    case BIT_FIELD_REF:
>> +      {
>>
>> I'm quite sure BIT_FIELD_REF is spurious here.
>>
>> Now to the "meat" ...
>>
>> +      tree load1 = get_base_loadstore (t1, false);
>> +      tree load2 = get_base_loadstore (t2, false);
>> +
>> +      if (load1 && load2 && !compare_memory_operand (t1, t2))
>> +       return return_false_with_msg ("memory operands are different");
>> +      else if ((load1 && !load2) || (!load1 && load2))
>> +       return return_false_with_msg ("");
>> +
>>
>> and the similar code for assigns.  The way you introduce the
>> unpack_handled_component flag to get_base_loadstore makes
>> it treat x.field as non-memory operand.  That's wrong.  I wonder
>> why you think you needed that.  Why does
>>
>>    tree load1= get_base_loadstore (t1);
>>
>> not work?  Btw, I'd have avoided get_base_loadstore and simply did
>>
>>    ao_ref_init (&r1, t1);
>>    ao_ref_init (&r2, t2);
>>    tree base1 = ao_ref_base (&r1);
>>    tree base2 = ao_ref_base (&r2);
>>    if ((DECL_P (base1) || (TREE_CODE (base1) == MEM_REF || TREE_CODE
>> (base1) == TARGET_MEM_REF))
>>       && (DECL_P (base2) || (...))
>>     ...
>>
>> or rather put this into compare_memory_operand and call that on all
>> operands
>> that may be memory (TREE_CODE () != SSA_NAME && !is_gimple_min_invariant
>> ()).
>>
>> I also miss where you handle memory operands on the LHS for calls
>> and for assignments.
>>
>> The compare_ssa_name refactoring part is ok to install.
>>
>> Can you please fix the volatile bug before the refactoring as it looks
>> like
>> we're going to iterate some more here unless I can find the time to give
>> it a quick try myself.
>>
>> Thanks,
>> Richard.
>>
>
> Hi.
>
> There's second part of the patch set.

It still has similar issues:

+/* Function compare for equality given memory operands T1 and T2.  */
+
+bool
+func_checker::compare_memory_operand (tree t1, tree t2)
+{
+  if (!t1 && !t2)
+    return true;
+  else if (!t1 || !t2)
+    return false;
+
+  if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2))
+    return return_false_with_msg ("different operand volatility");
+
+  bool source_is_memop = DECL_P (t1) || INDIRECT_REF_P (t1)
+                        || TREE_CODE (t1) == MEM_REF
+                        || TREE_CODE (t1) == TARGET_MEM_REF;
+
+  bool target_is_memop = DECL_P (t2) || INDIRECT_REF_P (t2)
+                        || TREE_CODE (t2) == MEM_REF
+                        || TREE_CODE (t2) == TARGET_MEM_REF;
+
+  /* Compare alias sets for memory operands.  */
+  if (source_is_memop && target_is_memop)
+    {
+      ao_ref r1, r2;
+      ao_ref_init (&r1, t1);
+      ao_ref_init (&r2, t2);

the *_is_memop tests need to work on the memory reference base.  Thus
simply move the ao_ref_init's before those checks and do them on
the result of ao_ref_base ().

Similarly the volatile check can be put into the if (source_is_memop
&& target_is_memop) block, volatileness doesn't matter for non-memory
operands.

Ok with that changes.

Thanks,
Richard.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] IPA ICF: refactoring + fix for PR ipa/63569
  2014-12-19 11:11         ` Richard Biener
@ 2015-01-05 13:12           ` Martin Liška
  2015-01-09  9:55             ` Richard Biener
  0 siblings, 1 reply; 10+ messages in thread
From: Martin Liška @ 2015-01-05 13:12 UTC (permalink / raw)
  To: Richard Biener; +Cc: GCC Patches

[-- Attachment #1: Type: text/plain, Size: 5983 bytes --]

On 12/19/2014 12:04 PM, Richard Biener wrote:
> On Thu, Dec 18, 2014 at 6:38 PM, Martin Liška <mliska@suse.cz> wrote:
>> On 12/17/2014 04:23 PM, Richard Biener wrote:
>>>
>>> On Wed, Dec 17, 2014 at 12:17 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>> On 12/11/2014 01:37 PM, Richard Biener wrote:
>>>>>
>>>>>
>>>>> On Wed, Dec 10, 2014 at 1:18 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>>>
>>>>>>
>>>>>> Hello.
>>>>>>
>>>>>> As suggested by Richard, I split compare_operand functions to various
>>>>>> functions
>>>>>> related to a specific comparison. Apart from that I added fast check
>>>>>> for
>>>>>> volatility flag that caused miscompilation mentioned in PR63569.
>>>>>>
>>>>>> Patch can bootstrap on x86_64-linux-pc without any regression seen and
>>>>>> I
>>>>>> was
>>>>>> able to build Firefox with LTO.
>>>>>>
>>>>>> Ready for trunk?
>>>>>
>>>>>
>>>>>
>>>>> Hmm, I don't think the dispatch to compare_memory_operand is at the
>>>>> correct place.  It should be called from places where currently
>>>>> compare_operand is called and it should recurse to compare_operand.
>>>>> That is, it is more "high-level".
>>>>>
>>>>> Can you please fix the volatile issue separately?  It's also not
>>>>> necessary
>>>>> to do that check on every operand but just on memory operands.
>>>>
>>>>
>>>>
>>>> Hello Richard.
>>>>
>>>> I hope the following patch is the finally the right approach we want to
>>>> do
>>>> ;)
>>>> In the first patch, I did just refactoring for high-level memory operand
>>>> comparison and the second of fixes problem connected to missing volatile
>>>> attribute comparison.
>>>>
>>>> Patch was retested and can bootstrap.
>>>>
>>>> What do you think about it?
>>>
>>>
>>> +bool
>>> +func_checker::compare_cst_or_decl (tree t1, tree t2)
>>> +{
>>> ...
>>> +    case INTEGER_CST:
>>> +      {
>>> +       ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
>>> +             && wi::to_offset  (t1) == wi::to_offset  (t2);
>>> +
>>> +       return return_with_debug (ret);
>>> +      }
>>> +    case COMPLEX_CST:
>>> +    case VECTOR_CST:
>>> +    case STRING_CST:
>>> +    case REAL_CST:
>>> +      {
>>> +       ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
>>> +       return return_with_debug (ret);
>>>
>>> why does the type matter for INTEGER_CSTs but not for other constants?
>>> Also comparing INTEGER_CSTs via to_offset is certainly wrong.  Please
>>> use tree_int_cst_equal_p (t1, t2) instead, or operand_equal_p which would
>>> end up calling tree_int_cst_equal_p.  That said, I'd have commonized all
>>> _CST nodes by
>>>
>>>     ret = compatible_types_p (....) && operand_equal_p (...);
>>>
>>> +    case CONST_DECL:
>>> +    case BIT_FIELD_REF:
>>> +      {
>>>
>>> I'm quite sure BIT_FIELD_REF is spurious here.
>>>
>>> Now to the "meat" ...
>>>
>>> +      tree load1 = get_base_loadstore (t1, false);
>>> +      tree load2 = get_base_loadstore (t2, false);
>>> +
>>> +      if (load1 && load2 && !compare_memory_operand (t1, t2))
>>> +       return return_false_with_msg ("memory operands are different");
>>> +      else if ((load1 && !load2) || (!load1 && load2))
>>> +       return return_false_with_msg ("");
>>> +
>>>
>>> and the similar code for assigns.  The way you introduce the
>>> unpack_handled_component flag to get_base_loadstore makes
>>> it treat x.field as non-memory operand.  That's wrong.  I wonder
>>> why you think you needed that.  Why does
>>>
>>>     tree load1= get_base_loadstore (t1);
>>>
>>> not work?  Btw, I'd have avoided get_base_loadstore and simply did
>>>
>>>     ao_ref_init (&r1, t1);
>>>     ao_ref_init (&r2, t2);
>>>     tree base1 = ao_ref_base (&r1);
>>>     tree base2 = ao_ref_base (&r2);
>>>     if ((DECL_P (base1) || (TREE_CODE (base1) == MEM_REF || TREE_CODE
>>> (base1) == TARGET_MEM_REF))
>>>        && (DECL_P (base2) || (...))
>>>      ...
>>>
>>> or rather put this into compare_memory_operand and call that on all
>>> operands
>>> that may be memory (TREE_CODE () != SSA_NAME && !is_gimple_min_invariant
>>> ()).
>>>
>>> I also miss where you handle memory operands on the LHS for calls
>>> and for assignments.
>>>
>>> The compare_ssa_name refactoring part is ok to install.
>>>
>>> Can you please fix the volatile bug before the refactoring as it looks
>>> like
>>> we're going to iterate some more here unless I can find the time to give
>>> it a quick try myself.
>>>
>>> Thanks,
>>> Richard.
>>>
>>
>> Hi.
>>
>> There's second part of the patch set.
>
> It still has similar issues:
>
> +/* Function compare for equality given memory operands T1 and T2.  */
> +
> +bool
> +func_checker::compare_memory_operand (tree t1, tree t2)
> +{
> +  if (!t1 && !t2)
> +    return true;
> +  else if (!t1 || !t2)
> +    return false;
> +
> +  if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2))
> +    return return_false_with_msg ("different operand volatility");
> +
> +  bool source_is_memop = DECL_P (t1) || INDIRECT_REF_P (t1)
> +                        || TREE_CODE (t1) == MEM_REF
> +                        || TREE_CODE (t1) == TARGET_MEM_REF;
> +
> +  bool target_is_memop = DECL_P (t2) || INDIRECT_REF_P (t2)
> +                        || TREE_CODE (t2) == MEM_REF
> +                        || TREE_CODE (t2) == TARGET_MEM_REF;
> +
> +  /* Compare alias sets for memory operands.  */
> +  if (source_is_memop && target_is_memop)
> +    {
> +      ao_ref r1, r2;
> +      ao_ref_init (&r1, t1);
> +      ao_ref_init (&r2, t2);
>
> the *_is_memop tests need to work on the memory reference base.  Thus
> simply move the ao_ref_init's before those checks and do them on
> the result of ao_ref_base ().
>
> Similarly the volatile check can be put into the if (source_is_memop
> && target_is_memop) block, volatileness doesn't matter for non-memory
> operands.
>
> Ok with that changes.
>
> Thanks,
> Richard.

Hello Richard.

Just for being sure, there's final version of the patch that passes
regression tests.

Ready for trunk in this shape?

Thanks,
Martin


[-- Attachment #2: 0001-IPA-ICF-compare_operand-is-split-to-multiple-functio.patch --]
[-- Type: text/x-patch, Size: 12164 bytes --]

From 49c7701110e7812568ad4cd0a6e08827bb878448 Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Thu, 18 Dec 2014 14:22:51 +0100
Subject: [PATCH] IPA ICF: compare_operand is split to multiple functions.

gcc/ChangeLog

2014-12-12  Martin Liska  <mliska@suse.cz>

	* ipa-icf-gimple.c (func_checker::compare_ssa_name): Enhance SSA
	name comparison.
	(func_checker::compare_memory_operand): New function.
	(func_checker::compare_operand): Split case to newly
	added functions.
	(func_checker::compare_cst_or_decl): New function.
	(func_checker::compare_gimple_call): Identify
	memory operands.
	(func_checker::compare_gimple_assign): Likewise.
	* ipa-icf-gimple.h: New function.
---
 gcc/ipa-icf-gimple.c | 247 ++++++++++++++++++++++++++++-----------------------
 gcc/ipa-icf-gimple.h |   9 +-
 2 files changed, 146 insertions(+), 110 deletions(-)

diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c
index 6689463..b6b81ca 100644
--- a/gcc/ipa-icf-gimple.c
+++ b/gcc/ipa-icf-gimple.c
@@ -110,6 +110,9 @@ func_checker::~func_checker ()
 bool
 func_checker::compare_ssa_name (tree t1, tree t2)
 {
+  gcc_assert (TREE_CODE (t1) == SSA_NAME);
+  gcc_assert (TREE_CODE (t2) == SSA_NAME);
+
   unsigned i1 = SSA_NAME_VERSION (t1);
   unsigned i2 = SSA_NAME_VERSION (t2);
 
@@ -123,6 +126,20 @@ func_checker::compare_ssa_name (tree t1, tree t2)
   else if (m_target_ssa_names[i2] != (int) i1)
     return false;
 
+  if (SSA_NAME_IS_DEFAULT_DEF (t1))
+    {
+      tree b1 = SSA_NAME_VAR (t1);
+      tree b2 = SSA_NAME_VAR (t2);
+
+      if (b1 == NULL && b2 == NULL)
+	return true;
+
+      if (b1 == NULL || b2 == NULL || TREE_CODE (b1) != TREE_CODE (b2))
+	return return_false ();
+
+      return compare_cst_or_decl (b1, b2);
+    }
+
   return true;
 }
 
@@ -178,9 +195,10 @@ func_checker::compare_decl (tree t1, tree t2)
 }
 
 /* Return true if types are compatible from perspective of ICF.  */
-bool func_checker::compatible_types_p (tree t1, tree t2,
-				       bool compare_polymorphic,
-				       bool first_argument)
+bool
+func_checker::compatible_types_p (tree t1, tree t2,
+				  bool compare_polymorphic,
+				  bool first_argument)
 {
   if (TREE_CODE (t1) != TREE_CODE (t2))
     return return_false_with_msg ("different tree types");
@@ -214,15 +232,112 @@ bool func_checker::compatible_types_p (tree t1, tree t2,
   return true;
 }
 
-/* Function responsible for comparison of handled components T1 and T2.
+/* Function compare for equality given memory operands T1 and T2.  */
+
+bool
+func_checker::compare_memory_operand (tree t1, tree t2)
+{
+  if (!t1 && !t2)
+    return true;
+  else if (!t1 || !t2)
+    return false;
+
+  ao_ref r1, r2;
+  ao_ref_init (&r1, t1);
+  ao_ref_init (&r2, t2);
+
+  tree b1 = ao_ref_base (&r1);
+  tree b2 = ao_ref_base (&r2);
+
+  bool source_is_memop = DECL_P (b1) || INDIRECT_REF_P (b1)
+			 || TREE_CODE (b1) == MEM_REF
+			 || TREE_CODE (b1) == TARGET_MEM_REF;
+
+  bool target_is_memop = DECL_P (b2) || INDIRECT_REF_P (b2)
+			 || TREE_CODE (b2) == MEM_REF
+			 || TREE_CODE (b2) == TARGET_MEM_REF;
+
+  /* Compare alias sets for memory operands.  */
+  if (source_is_memop && target_is_memop)
+    {
+      if (TREE_THIS_VOLATILE (b1) != TREE_THIS_VOLATILE (b2))
+	return return_false_with_msg ("different operand volatility");
+
+      if (ao_ref_alias_set (&r1) != ao_ref_alias_set (&r2)
+	  || ao_ref_base_alias_set (&r1) != ao_ref_base_alias_set (&r2))
+	return return_false_with_msg ("ao alias sets are different");
+    }
+
+  return compare_operand (t1, t2);
+}
+
+/* Function compare for equality given trees T1 and T2 which
+   can be either a constant or a declaration type.  */
+
+bool
+func_checker::compare_cst_or_decl (tree t1, tree t2)
+{
+  bool ret;
+
+  switch (TREE_CODE (t1))
+    {
+    case INTEGER_CST:
+    case COMPLEX_CST:
+    case VECTOR_CST:
+    case STRING_CST:
+    case REAL_CST:
+      {
+	ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
+	      && operand_equal_p (t1, t2, OEP_ONLY_CONST);
+	return return_with_debug (ret);
+      }
+    case FUNCTION_DECL:
+      {
+	ret = compare_function_decl (t1, t2);
+	return return_with_debug (ret);
+      }
+    case VAR_DECL:
+      return return_with_debug (compare_variable_decl (t1, t2));
+    case FIELD_DECL:
+      {
+	tree offset1 = DECL_FIELD_OFFSET (t1);
+	tree offset2 = DECL_FIELD_OFFSET (t2);
+
+	tree bit_offset1 = DECL_FIELD_BIT_OFFSET (t1);
+	tree bit_offset2 = DECL_FIELD_BIT_OFFSET (t2);
+
+	ret = compare_operand (offset1, offset2)
+	      && compare_operand (bit_offset1, bit_offset2);
+
+	return return_with_debug (ret);
+      }
+    case LABEL_DECL:
+      {
+	int *bb1 = m_label_bb_map.get (t1);
+	int *bb2 = m_label_bb_map.get (t2);
+
+	return return_with_debug (*bb1 == *bb2);
+      }
+    case PARM_DECL:
+    case RESULT_DECL:
+    case CONST_DECL:
+      {
+	ret = compare_decl (t1, t2);
+	return return_with_debug (ret);
+      }
+    default:
+      gcc_unreachable ();
+    }
+}
+
+/* Function responsible for comparison of various operands T1 and T2.
    If these components, from functions FUNC1 and FUNC2, are equal, true
    is returned.  */
 
 bool
 func_checker::compare_operand (tree t1, tree t2)
 {
-  tree base1, base2, x1, x2, y1, y2, z1, z2;
-  HOST_WIDE_INT offset1 = 0, offset2 = 0;
+  tree x1, x2, y1, y2, z1, z2;
   bool ret;
 
   if (!t1 && !t2)
@@ -233,24 +348,9 @@ func_checker::compare_operand (tree t1, tree t2)
   tree tt1 = TREE_TYPE (t1);
   tree tt2 = TREE_TYPE (t2);
 
-  if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2))
-    return return_false_with_msg ("different operand volatility");
-
   if (!func_checker::compatible_types_p (tt1, tt2))
     return false;
 
-  base1 = get_addr_base_and_unit_offset (t1, &offset1);
-  base2 = get_addr_base_and_unit_offset (t2, &offset2);
-
-  if (base1 && base2)
-    {
-      if (offset1 != offset2)
-	return return_false_with_msg ("base offsets are different");
-
-      t1 = base1;
-      t2 = base2;
-    }
-
   if (TREE_CODE (t1) != TREE_CODE (t2))
     return return_false ();
 
@@ -273,6 +373,7 @@ func_checker::compare_operand (tree t1, tree t2)
       }
     case ARRAY_REF:
     case ARRAY_RANGE_REF:
+      /* First argument is the array, second is the index.  */
       x1 = TREE_OPERAND (t1, 0);
       x2 = TREE_OPERAND (t2, 0);
       y1 = TREE_OPERAND (t1, 1);
@@ -284,6 +385,7 @@ func_checker::compare_operand (tree t1, tree t2)
       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);
@@ -308,16 +410,6 @@ func_checker::compare_operand (tree t1, tree t2)
 	if (!compare_operand (x1, x2))
 	  return return_false_with_msg ("");
 
-	if (get_alias_set (TREE_TYPE (y1)) != get_alias_set (TREE_TYPE (y2)))
-	  return return_false_with_msg ("alias set for MEM_REF offsets are different");
-
-	ao_ref r1, r2;
-	ao_ref_init (&r1, t1);
-	ao_ref_init (&r2, t2);
-	if (ao_ref_alias_set (&r1) != ao_ref_alias_set (&r2)
-	    || ao_ref_base_alias_set (&r1) != ao_ref_base_alias_set (&r2))
-	  return return_false_with_msg ("ao alias sets are different");
-
 	/* Type of the offset on MEM_REF does not matter.  */
 	return wi::to_offset  (y1) == wi::to_offset  (y2);
       }
@@ -329,7 +421,7 @@ func_checker::compare_operand (tree t1, tree t2)
 	y2 = TREE_OPERAND (t2, 1);
 
 	ret = compare_operand (x1, x2)
-	      && compare_operand (y1, y2);
+	      && compare_cst_or_decl (y1, y2);
 
 	return return_with_debug (ret);
       }
@@ -343,9 +435,9 @@ func_checker::compare_operand (tree t1, tree t2)
 	z1 = TREE_OPERAND (t1, 2);
 	z2 = TREE_OPERAND (t2, 2);
 
-	ret = compare_operand (x1, x2)
-	      && compare_operand (y1, y2)
-	      && compare_operand (z1, z2);
+	ret = compare_ssa_name (x1, x2)
+	      && compare_ssa_name (y1, y2)
+	      && compare_cst_or_decl (z1, z2);
 
 	return return_with_debug (ret);
       }
@@ -357,89 +449,26 @@ func_checker::compare_operand (tree t1, tree t2)
 	ret = compare_operand (x1, x2);
 	return return_with_debug (ret);
       }
-    case SSA_NAME:
-      {
-	ret = compare_ssa_name (t1, t2);
-
-	if (!ret)
-	  return return_with_debug (ret);
-
-	if (SSA_NAME_IS_DEFAULT_DEF (t1))
-	  {
-	    tree b1 = SSA_NAME_VAR (t1);
-	    tree b2 = SSA_NAME_VAR (t2);
-
-	    if (b1 == NULL && b2 == NULL)
-	      return true;
-
-	    if (b1 == NULL || b2 == NULL || TREE_CODE (b1) != TREE_CODE (b2))
-	      return return_false ();
-
-	    switch (TREE_CODE (b1))
-	      {
-	      case VAR_DECL:
-		return return_with_debug (compare_variable_decl (t1, t2));
-	      case PARM_DECL:
-	      case RESULT_DECL:
-		ret = compare_decl (b1, b2);
-		return return_with_debug (ret);
-	      default:
-		return return_false_with_msg ("Unknown TREE code reached");
-	      }
-	  }
-	else
-	  return true;
-      }
-    case INTEGER_CST:
+    case BIT_FIELD_REF:
       {
-	ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
-	      && wi::to_offset  (t1) == wi::to_offset  (t2);
-
+	ret = compare_decl (t1, t2);
 	return return_with_debug (ret);
       }
+    case SSA_NAME:
+	return compare_ssa_name (t1, t2);
+    case INTEGER_CST:
     case COMPLEX_CST:
     case VECTOR_CST:
     case STRING_CST:
     case REAL_CST:
-      {
-	ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
-	return return_with_debug (ret);
-      }
     case FUNCTION_DECL:
-      {
-	ret = compare_function_decl (t1, t2);
-	return return_with_debug (ret);
-      }
     case VAR_DECL:
-      return return_with_debug (compare_variable_decl (t1, t2));
     case FIELD_DECL:
-      {
-	tree offset1 = DECL_FIELD_OFFSET (t1);
-	tree offset2 = DECL_FIELD_OFFSET (t2);
-
-	tree bit_offset1 = DECL_FIELD_BIT_OFFSET (t1);
-	tree bit_offset2 = DECL_FIELD_BIT_OFFSET (t2);
-
-	ret = compare_operand (offset1, offset2)
-	      && compare_operand (bit_offset1, bit_offset2);
-
-	return return_with_debug (ret);
-      }
     case LABEL_DECL:
-      {
-	int *bb1 = m_label_bb_map.get (t1);
-	int *bb2 = m_label_bb_map.get (t2);
-
-	return return_with_debug (*bb1 == *bb2);
-      }
     case PARM_DECL:
     case RESULT_DECL:
     case CONST_DECL:
-    case BIT_FIELD_REF:
-      {
-	ret = compare_decl (t1, t2);
-	return return_with_debug (ret);
-      }
+      return compare_cst_or_decl (t1, t2);
     default:
       return return_false_with_msg ("Unknown TREE code reached");
     }
@@ -706,15 +735,15 @@ func_checker::compare_gimple_call (gcall *s1, gcall *s2)
       t1 = gimple_call_arg (s1, i);
       t2 = gimple_call_arg (s2, i);
 
-      if (!compare_operand (t1, t2))
-	return false;
+      if (!compare_memory_operand (t1, t2))
+	return return_false_with_msg ("memory operands are different");
     }
 
   /* Return value checking.  */
   t1 = gimple_get_lhs (s1);
   t2 = gimple_get_lhs (s2);
 
-  return compare_operand (t1, t2);
+  return compare_memory_operand (t1, t2);
 }
 
 
@@ -745,8 +774,8 @@ func_checker::compare_gimple_assign (gimple s1, gimple s2)
       arg1 = gimple_op (s1, i);
       arg2 = gimple_op (s2, i);
 
-      if (!compare_operand (arg1, arg2))
-	return false;
+      if (!compare_memory_operand (arg1, arg2))
+	return return_false_with_msg ("memory operands are different");
     }
 
 
diff --git a/gcc/ipa-icf-gimple.h b/gcc/ipa-icf-gimple.h
index cb9b1fc..b2d670d 100644
--- a/gcc/ipa-icf-gimple.h
+++ b/gcc/ipa-icf-gimple.h
@@ -203,7 +203,14 @@ public:
   /* Verifies that tree labels T1 and T2 correspond.  */
   bool compare_tree_ssa_label (tree t1, tree t2);
 
-  /* Function responsible for comparison of handled components T1 and T2.
+  /* Function compare for equality given memory operands T1 and T2.  */
+  bool compare_memory_operand (tree t1, tree t2);
+
+  /* Function compare for equality given trees T1 and T2 which
+     can be either a constant or a declaration type.  */
+  bool compare_cst_or_decl (tree t1, tree t2);
+
+  /* Function responsible for comparison of various operands T1 and T2.
      If these components, from functions FUNC1 and FUNC2, are equal, true
      is returned.  */
   bool compare_operand (tree t1, tree t2);
-- 
2.1.2


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] IPA ICF: refactoring + fix for PR ipa/63569
  2015-01-05 13:12           ` Martin Liška
@ 2015-01-09  9:55             ` Richard Biener
  0 siblings, 0 replies; 10+ messages in thread
From: Richard Biener @ 2015-01-09  9:55 UTC (permalink / raw)
  To: Martin Liška; +Cc: GCC Patches

On Mon, Jan 5, 2015 at 2:12 PM, Martin Liška <mliska@suse.cz> wrote:
> On 12/19/2014 12:04 PM, Richard Biener wrote:
>>
>> On Thu, Dec 18, 2014 at 6:38 PM, Martin Liška <mliska@suse.cz> wrote:
>>>
>>> On 12/17/2014 04:23 PM, Richard Biener wrote:
>>>>
>>>>
>>>> On Wed, Dec 17, 2014 at 12:17 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>>
>>>>>
>>>>> On 12/11/2014 01:37 PM, Richard Biener wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Dec 10, 2014 at 1:18 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hello.
>>>>>>>
>>>>>>> As suggested by Richard, I split compare_operand functions to various
>>>>>>> functions
>>>>>>> related to a specific comparison. Apart from that I added fast check
>>>>>>> for
>>>>>>> volatility flag that caused miscompilation mentioned in PR63569.
>>>>>>>
>>>>>>> Patch can bootstrap on x86_64-linux-pc without any regression seen
>>>>>>> and
>>>>>>> I
>>>>>>> was
>>>>>>> able to build Firefox with LTO.
>>>>>>>
>>>>>>> Ready for trunk?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hmm, I don't think the dispatch to compare_memory_operand is at the
>>>>>> correct place.  It should be called from places where currently
>>>>>> compare_operand is called and it should recurse to compare_operand.
>>>>>> That is, it is more "high-level".
>>>>>>
>>>>>> Can you please fix the volatile issue separately?  It's also not
>>>>>> necessary
>>>>>> to do that check on every operand but just on memory operands.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Hello Richard.
>>>>>
>>>>> I hope the following patch is the finally the right approach we want to
>>>>> do
>>>>> ;)
>>>>> In the first patch, I did just refactoring for high-level memory
>>>>> operand
>>>>> comparison and the second of fixes problem connected to missing
>>>>> volatile
>>>>> attribute comparison.
>>>>>
>>>>> Patch was retested and can bootstrap.
>>>>>
>>>>> What do you think about it?
>>>>
>>>>
>>>>
>>>> +bool
>>>> +func_checker::compare_cst_or_decl (tree t1, tree t2)
>>>> +{
>>>> ...
>>>> +    case INTEGER_CST:
>>>> +      {
>>>> +       ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
>>>> +             && wi::to_offset  (t1) == wi::to_offset  (t2);
>>>> +
>>>> +       return return_with_debug (ret);
>>>> +      }
>>>> +    case COMPLEX_CST:
>>>> +    case VECTOR_CST:
>>>> +    case STRING_CST:
>>>> +    case REAL_CST:
>>>> +      {
>>>> +       ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
>>>> +       return return_with_debug (ret);
>>>>
>>>> why does the type matter for INTEGER_CSTs but not for other constants?
>>>> Also comparing INTEGER_CSTs via to_offset is certainly wrong.  Please
>>>> use tree_int_cst_equal_p (t1, t2) instead, or operand_equal_p which
>>>> would
>>>> end up calling tree_int_cst_equal_p.  That said, I'd have commonized all
>>>> _CST nodes by
>>>>
>>>>     ret = compatible_types_p (....) && operand_equal_p (...);
>>>>
>>>> +    case CONST_DECL:
>>>> +    case BIT_FIELD_REF:
>>>> +      {
>>>>
>>>> I'm quite sure BIT_FIELD_REF is spurious here.
>>>>
>>>> Now to the "meat" ...
>>>>
>>>> +      tree load1 = get_base_loadstore (t1, false);
>>>> +      tree load2 = get_base_loadstore (t2, false);
>>>> +
>>>> +      if (load1 && load2 && !compare_memory_operand (t1, t2))
>>>> +       return return_false_with_msg ("memory operands are different");
>>>> +      else if ((load1 && !load2) || (!load1 && load2))
>>>> +       return return_false_with_msg ("");
>>>> +
>>>>
>>>> and the similar code for assigns.  The way you introduce the
>>>> unpack_handled_component flag to get_base_loadstore makes
>>>> it treat x.field as non-memory operand.  That's wrong.  I wonder
>>>> why you think you needed that.  Why does
>>>>
>>>>     tree load1= get_base_loadstore (t1);
>>>>
>>>> not work?  Btw, I'd have avoided get_base_loadstore and simply did
>>>>
>>>>     ao_ref_init (&r1, t1);
>>>>     ao_ref_init (&r2, t2);
>>>>     tree base1 = ao_ref_base (&r1);
>>>>     tree base2 = ao_ref_base (&r2);
>>>>     if ((DECL_P (base1) || (TREE_CODE (base1) == MEM_REF || TREE_CODE
>>>> (base1) == TARGET_MEM_REF))
>>>>        && (DECL_P (base2) || (...))
>>>>      ...
>>>>
>>>> or rather put this into compare_memory_operand and call that on all
>>>> operands
>>>> that may be memory (TREE_CODE () != SSA_NAME && !is_gimple_min_invariant
>>>> ()).
>>>>
>>>> I also miss where you handle memory operands on the LHS for calls
>>>> and for assignments.
>>>>
>>>> The compare_ssa_name refactoring part is ok to install.
>>>>
>>>> Can you please fix the volatile bug before the refactoring as it looks
>>>> like
>>>> we're going to iterate some more here unless I can find the time to give
>>>> it a quick try myself.
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>
>>> Hi.
>>>
>>> There's second part of the patch set.
>>
>>
>> It still has similar issues:
>>
>> +/* Function compare for equality given memory operands T1 and T2.  */
>> +
>> +bool
>> +func_checker::compare_memory_operand (tree t1, tree t2)
>> +{
>> +  if (!t1 && !t2)
>> +    return true;
>> +  else if (!t1 || !t2)
>> +    return false;
>> +
>> +  if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2))
>> +    return return_false_with_msg ("different operand volatility");
>> +
>> +  bool source_is_memop = DECL_P (t1) || INDIRECT_REF_P (t1)
>> +                        || TREE_CODE (t1) == MEM_REF
>> +                        || TREE_CODE (t1) == TARGET_MEM_REF;
>> +
>> +  bool target_is_memop = DECL_P (t2) || INDIRECT_REF_P (t2)
>> +                        || TREE_CODE (t2) == MEM_REF
>> +                        || TREE_CODE (t2) == TARGET_MEM_REF;
>> +
>> +  /* Compare alias sets for memory operands.  */
>> +  if (source_is_memop && target_is_memop)
>> +    {
>> +      ao_ref r1, r2;
>> +      ao_ref_init (&r1, t1);
>> +      ao_ref_init (&r2, t2);
>>
>> the *_is_memop tests need to work on the memory reference base.  Thus
>> simply move the ao_ref_init's before those checks and do them on
>> the result of ao_ref_base ().
>>
>> Similarly the volatile check can be put into the if (source_is_memop
>> && target_is_memop) block, volatileness doesn't matter for non-memory
>> operands.
>>
>> Ok with that changes.
>>
>> Thanks,
>> Richard.
>
>
> Hello Richard.
>
> Just for being sure, there's final version of the patch that passes
> regression tests.
>
> Ready for trunk in this shape?

Ok.

Thanks,
Richard.

> Thanks,
> Martin
>

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-01-09  9:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-10 12:19 [PATCH] IPA ICF: refactoring + fix for PR ipa/63569 Martin Liška
2014-12-11 12:37 ` Richard Biener
2014-12-17 11:29   ` Martin Liška
2014-12-17 15:41     ` Richard Biener
2014-12-18 13:45       ` Martin Liška
2014-12-19 11:01         ` Richard Biener
2014-12-18 17:41       ` Martin Liška
2014-12-19 11:11         ` Richard Biener
2015-01-05 13:12           ` Martin Liška
2015-01-09  9:55             ` Richard Biener

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).