On 12/17/2014 04:23 PM, Richard Biener wrote:
On Wed, Dec 17, 2014 at 12:17 PM, Martin Liška <mli...@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 <mli...@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
>From b984eaae263eedc8beb2413f4739d3574f46d63d Mon Sep 17 00:00:00 2001
From: mliska <mli...@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 <mli...@suse.cz>
PR ipa/63569
* gcc.dg/ipa/pr63569.c: New test.
gcc/ChangeLog:
2014-12-18 Martin Liska <mli...@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
