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>, "hubi >> Jan Hubicka" <hubicka at ucw dot cz>
- Date: Wed, 17 Dec 2014 12:17:36 +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>
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
>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
>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