This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] IPA ICF: refactoring + fix for PR ipa/63569
- From: Martin LiÅka <mliska at suse dot cz>
- To: gcc-patches at gcc dot gnu dot org
- Date: Thu, 18 Dec 2014 18:38:10 +0100
- Subject: Re: [PATCH] IPA ICF: refactoring + fix for PR ipa/63569
- Authentication-results: sourceware.org; auth=none
- References: <54883A2C dot 4050301 at suse dot cz> <CAFiYyc2sJsdrj8gaVEmqx8kjW8ODA0T7M0k8kCP9Ry7RxLm4+g at mail dot gmail dot com> <54916650 dot 5060707 at suse dot cz> <CAFiYyc1EUEQrkRNTUhQddWWUMiP_3324tqQacw9WejSmBXxtcQ at mail dot gmail dot com>
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
>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