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: Richard Biener <richard dot guenther at gmail dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 05 Jan 2015 14:12:26 +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> <54931102 dot 9000404 at suse dot cz> <CAFiYyc1dvsHmRSiRrEDE3dR79di8b4HTVpCnECx17HPkdaGQ6Q at mail dot gmail dot com>
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
>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