| Message ID | 54F2554D.5080109@suse.cz |
|---|---|
| State | New |
| Headers | show |
> > Half of FAILs are gone, but the rest is correctly merged (alignment matches). Thus I would omit ICF > in this testcase. I'm going to install the patch. > > Thanks, > Martin > >From 60c5fdc5d5bab2d26a43813ffebda247c8dd1fce Mon Sep 17 00:00:00 2001 > From: mliska <mliska@suse.cz> > Date: Fri, 27 Feb 2015 21:49:46 +0100 > Subject: [PATCH 1/4] ICF is more strict about non-common function and var > attributes. > > gcc/ChangeLog: > > 2015-02-28 Martin Liska <mliska@suse.cz> > Jan Hubicka <hubicka@ucw.cz> > > * ipa-icf-gimple.c (func_checker::compare_variable_decl): > Validate variable alignment. > * ipa-icf.c (sem_function::equals_private): Be more precise > about non-common function attributes. > (sem_variable::equals): Likewise. > > gcc/testsuite/ChangeLog: > > 2015-03-01 Martin Liska <mliska@suse.cz> > Jan Hubicka <hubicka@ucw.cz> > > * gcc.target/i386/stackalign/longlong-2.c: Omit ICF. OK then. Thanks! Are we ready to close the PR? Honza
> > > > Half of FAILs are gone, but the rest is correctly merged (alignment matches). Thus I would omit ICF > > in this testcase. I'm going to install the patch. Richard, I wonder what happens with TYPE_ALIGN at LTO. It is not part of canonical type definition and thus we get random alignments on CANONICAL_TYPE_HASH. func_checker::types_compatible leads to type_compatible_p which will eventually do /* If we know the canonical types, compare them. */ if (TYPE_CANONICAL (inner_type) && TYPE_CANONICAL (inner_type) == TYPE_CANONICAL (outer_type)) return true; and thus we will hapilly merge types with different TYPE_ALIGN. Should func_checker::types_compatible be extended to compare these? Clearly TYPE_ALIGN matters for vectorizer and other plaes... Any chance -malign-double can work and mix with -mno-align-double? I think we will mix the alignments because double is one of nodes we do not stream, right? Honza
On March 1, 2015 1:09:50 AM CET, Jan Hubicka <hubicka@ucw.cz> wrote: >> > >> > Half of FAILs are gone, but the rest is correctly merged (alignment >matches). Thus I would omit ICF >> > in this testcase. I'm going to install the patch. > >Richard, I wonder what happens with TYPE_ALIGN at LTO. It is not part >of canonical type >definition and thus we get random alignments on CANONICAL_TYPE_HASH. >func_checker::types_compatible leads to type_compatible_p which will >eventually do >/* If we know the canonical types, compare them. */ > >if (TYPE_CANONICAL (inner_type) > >&& TYPE_CANONICAL (inner_type) == TYPE_CANONICAL (outer_type)) > >return true; > >and thus we will hapilly merge types with different TYPE_ALIGN. >Should func_checker::types_compatible be extended to compare these? >Clearly TYPE_ALIGN matters for vectorizer and other plaes... But it matters on MEM_REFs only, and you can't use canonical types for that. And we don't. Tree merging correctly takes TYPE_ALING into account. >Any chance -malign-double can work and mix with -mno-align-double? I >think we >will mix the alignments because double is one of nodes we do not >stream, right? Right. Richard. > >Honza
> >and thus we will hapilly merge types with different TYPE_ALIGN. > >Should func_checker::types_compatible be extended to compare these? > >Clearly TYPE_ALIGN matters for vectorizer and other plaes... > > But it matters on MEM_REFs only, and you can't use canonical types for that. > And we don't. Tree merging correctly takes TYPE_ALING into account. Yep, I wonder about ICF that will match the different MEM_REFs based on useless type conversion assumptions. In fact here is a wrong code testcase: $ more t.c struct a { int a[100000]; } __attribute__ ((aligned (32))); struct b { int a[100000]; }; t2(struct a *a) { int i; for (i=0;i<100000;i++) a->a[i]++; } t(struct b *a) { int i; for (i=0;i<100000;i++) a->a[i]++; } $ ./xgcc -B ./ -O3 t.c -flto -c -fno-inline t.c:10:1: warning: return type defaults to �int� [-Wimplicit-int] t2(struct a *a) ^ t.c:16:1: warning: return type defaults to �int� [-Wimplicit-int] t(struct b *a) ^ $ ./xgcc -B ./ -O3 t.o -flto -r -nostdlib $ objdump a.out -d a.out: file format elf64-x86-64 Disassembly of section .text: 0000000000000000 <t2>: 0: 48 8d 87 80 1a 06 00 lea 0x61a80(%rdi),%rax 7: 66 0f 6f 0d 00 00 00 movdqa 0x0(%rip),%xmm1 # f <t2+0xf> e: 00 f: 90 nop 10: 48 83 c7 10 add $0x10,%rdi 14: 66 0f 6f 47 f0 movdqa -0x10(%rdi),%xmm0 19: 66 0f fe c1 paddd %xmm1,%xmm0 1d: 0f 29 47 f0 movaps %xmm0,-0x10(%rdi) 21: 48 39 c7 cmp %rax,%rdi 24: 75 ea jne 10 <t2+0x10> 26: f3 c3 repz retq 28: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1) 2f: 00 0000000000000030 <t>: 30: e9 00 00 00 00 jmpq 35 <t+0x5> Function t should have alignment prologue. We need to match additional MEMREF properties I suppose, including alignment etc. Honza
> > >and thus we will hapilly merge types with different TYPE_ALIGN. > > >Should func_checker::types_compatible be extended to compare these? > > >Clearly TYPE_ALIGN matters for vectorizer and other plaes... > > > > But it matters on MEM_REFs only, and you can't use canonical types for that. > > And we don't. Tree merging correctly takes TYPE_ALING into account. > > Yep, I wonder about ICF that will match the different MEM_REFs based on useless > type conversion assumptions. In fact here is a wrong code testcase: Now tracked as PR65270 Honza
From 60c5fdc5d5bab2d26a43813ffebda247c8dd1fce Mon Sep 17 00:00:00 2001 From: mliska <mliska@suse.cz> Date: Fri, 27 Feb 2015 21:49:46 +0100 Subject: [PATCH 1/4] ICF is more strict about non-common function and var attributes. gcc/ChangeLog: 2015-02-28 Martin Liska <mliska@suse.cz> Jan Hubicka <hubicka@ucw.cz> * ipa-icf-gimple.c (func_checker::compare_variable_decl): Validate variable alignment. * ipa-icf.c (sem_function::equals_private): Be more precise about non-common function attributes. (sem_variable::equals): Likewise. gcc/testsuite/ChangeLog: 2015-03-01 Martin Liska <mliska@suse.cz> Jan Hubicka <hubicka@ucw.cz> * gcc.target/i386/stackalign/longlong-2.c: Omit ICF. --- gcc/ipa-icf-gimple.c | 3 ++ gcc/ipa-icf.c | 35 ++++++++++++++++++++++ .../gcc.target/i386/stackalign/longlong-2.c | 2 +- 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c index 53d2c38..cbeb795 100644 --- a/gcc/ipa-icf-gimple.c +++ b/gcc/ipa-icf-gimple.c @@ -575,6 +575,9 @@ func_checker::compare_variable_decl (tree t1, tree t2) if (t1 == t2) return true; + if (DECL_ALIGN (t1) != DECL_ALIGN (t2)) + return return_false_with_msg ("alignments are different"); + if (DECL_HARD_REGISTER (t1) != DECL_HARD_REGISTER (t2)) return return_false_with_msg ("DECL_HARD_REGISTER are different"); diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c index 5d50b6f..92133fc 100644 --- a/gcc/ipa-icf.c +++ b/gcc/ipa-icf.c @@ -619,6 +619,30 @@ sem_function::equals_private (sem_item *item, if (!compare_phi_node (bb_sorted[i]->bb, m_compared_func->bb_sorted[i]->bb)) return return_false_with_msg ("PHI node comparison returns false"); + /* Compare special function DECL attributes. */ + if (DECL_FUNCTION_PERSONALITY (decl) != DECL_FUNCTION_PERSONALITY (item->decl)) + return return_false_with_msg ("function personalities are different"); + + if (DECL_DISREGARD_INLINE_LIMITS (decl) != DECL_DISREGARD_INLINE_LIMITS (item->decl)) + return return_false_with_msg ("DECL_DISREGARD_INLINE_LIMITS are different"); + + if (DECL_DECLARED_INLINE_P (decl) != DECL_DECLARED_INLINE_P (item->decl)) + return return_false_with_msg ("inline attributes are different"); + + if (DECL_IS_OPERATOR_NEW (decl) != DECL_IS_OPERATOR_NEW (item->decl)) + return return_false_with_msg ("operator new flags are different"); + + if (DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (decl) + != DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (item->decl)) + return return_false_with_msg ("intrument function entry exit " + "attributes are different"); + + if (DECL_NO_LIMIT_STACK (decl) != DECL_NO_LIMIT_STACK (item->decl)) + return return_false_with_msg ("no stack limit attributes are different"); + + if (flags_from_decl_or_type (decl) != flags_from_decl_or_type (item->decl)) + return return_false_with_msg ("decl_or_type flags are different"); + return result; } @@ -1280,6 +1304,17 @@ sem_variable::equals (sem_item *item, if (!ctor || !v->ctor) return return_false_with_msg ("ctor is missing for semantic variable"); + if (DECL_IN_CONSTANT_POOL (decl) + && (DECL_IN_CONSTANT_POOL (item->decl) + || item->node->address_matters_p ())) + return return_false_with_msg ("constant pool"); + + if (DECL_IN_TEXT_SECTION (decl) != DECL_IN_TEXT_SECTION (item->decl)) + return return_false_with_msg ("text section"); + + if (DECL_TLS_MODEL (decl) || DECL_TLS_MODEL (item->decl)) + return return_false_with_msg ("TLS model"); + return sem_variable::equals (ctor, v->ctor); } diff --git a/gcc/testsuite/gcc.target/i386/stackalign/longlong-2.c b/gcc/testsuite/gcc.target/i386/stackalign/longlong-2.c index 6ea83f9..d52b9d1 100644 --- a/gcc/testsuite/gcc.target/i386/stackalign/longlong-2.c +++ b/gcc/testsuite/gcc.target/i386/stackalign/longlong-2.c @@ -1,6 +1,6 @@ /* { dg-do compile { target { ! *-*-darwin* } } } */ /* { dg-require-effective-target ia32 } */ -/* { dg-options "-O2 -mpreferred-stack-boundary=2" } */ +/* { dg-options "-O2 -mpreferred-stack-boundary=2 -fno-ipa-icf" } */ /* { dg-final { scan-assembler-times "and\[lq\]?\[^\\n\]*-8,\[^\\n\]*sp" 2 } } */ /* { dg-final { scan-assembler-times "and\[lq\]?\[^\\n\]*-16,\[^\\n\]*sp" 2 } } */ -- 2.1.2