| Message ID | 54916650.5060707@suse.cz |
|---|---|
| State | New |
| Headers | show |
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. > Thank you, > Martin > >> >> Thanks, >> Richard. >> >>> Thanks, >>> Martin > >
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