diff mbox

IPA ICF: refactoring + fix for PR ipa/63569

Message ID 54AA8DBA.6030909@suse.cz
State New
Headers show

Commit Message

Martin Liška Jan. 5, 2015, 1:12 p.m. UTC 
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

Comments

Richard Biener Jan. 9, 2015, 9:50 a.m. UTC | #1 
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
>
diff mbox

Patch

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