| Message ID | 3a4fffa0-406b-2c4c-a349-d133465a2e1d@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Verify NOP_EXPR LHS type in IPA ICF. | expand |
On Fri, Nov 15, 2019 at 2:40 PM Martin Liška <mliska@suse.cz> wrote: > > Hi. > > After I removed ::add_type hashing in r278207, we don't hash > types of SSA names. Moreover, compare_gimple_assign just compares > all operands and ICF does not compare types of SSA names in order > to not be too much conservative. So that, I'm suggesting to compare > LHS type of NOP_EXPRs, which is a possible place for conversion. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? Use CONVERT_EXPR_CODE_P (code1) please. Note there are other codes like VIEW_CONVERT_EXPR or BIT_FIELD_REF or MEM_REF which involve conversions so I'm not sure not comparing types of SSA names is a good idea. Richard. > Thanks, > Martin > > gcc/ChangeLog: > > 2019-11-15 Martin Liska <mliska@suse.cz> > > PR ipa/92529 > * ipa-icf-gimple.c (func_checker::compare_gimple_assign): > Compare LHS types of NOP_EXPR. > > gcc/testsuite/ChangeLog: > > 2019-11-15 Martin Liska <mliska@suse.cz> > > PR ipa/92529 > * gcc.dg/ipa/pr92529.c: New test. > --- > gcc/ipa-icf-gimple.c | 7 +++++++ > gcc/testsuite/gcc.dg/ipa/pr92529.c | 28 ++++++++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > create mode 100644 gcc/testsuite/gcc.dg/ipa/pr92529.c > >
On 11/15/19 2:59 PM, Richard Biener wrote: > On Fri, Nov 15, 2019 at 2:40 PM Martin Liška <mliska@suse.cz> wrote: >> >> Hi. >> >> After I removed ::add_type hashing in r278207, we don't hash >> types of SSA names. Moreover, compare_gimple_assign just compares >> all operands and ICF does not compare types of SSA names in order >> to not be too much conservative. So that, I'm suggesting to compare >> LHS type of NOP_EXPRs, which is a possible place for conversion. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? > > Use CONVERT_EXPR_CODE_P (code1) please. Sure. > > Note there are other codes like VIEW_CONVERT_EXPR or > BIT_FIELD_REF or MEM_REF which involve conversions so I'm not > sure not comparing types of SSA names is a good idea. Note that all these go through operand_compare::operand_equal_p which considers types accordingly. I would like to go with this patch and we can eventually come up with a more strict type comparison for SSA NAMEs. Martin > > Richard. > >> Thanks, >> Martin >> >> gcc/ChangeLog: >> >> 2019-11-15 Martin Liska <mliska@suse.cz> >> >> PR ipa/92529 >> * ipa-icf-gimple.c (func_checker::compare_gimple_assign): >> Compare LHS types of NOP_EXPR. >> >> gcc/testsuite/ChangeLog: >> >> 2019-11-15 Martin Liska <mliska@suse.cz> >> >> PR ipa/92529 >> * gcc.dg/ipa/pr92529.c: New test. >> --- >> gcc/ipa-icf-gimple.c | 7 +++++++ >> gcc/testsuite/gcc.dg/ipa/pr92529.c | 28 ++++++++++++++++++++++++++++ >> 2 files changed, 35 insertions(+) >> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr92529.c >> >>
On Mon, Nov 18, 2019 at 10:04 AM Martin Liška <mliska@suse.cz> wrote: > > On 11/15/19 2:59 PM, Richard Biener wrote: > > On Fri, Nov 15, 2019 at 2:40 PM Martin Liška <mliska@suse.cz> wrote: > >> > >> Hi. > >> > >> After I removed ::add_type hashing in r278207, we don't hash > >> types of SSA names. Moreover, compare_gimple_assign just compares > >> all operands and ICF does not compare types of SSA names in order > >> to not be too much conservative. So that, I'm suggesting to compare > >> LHS type of NOP_EXPRs, which is a possible place for conversion. > >> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >> > >> Ready to be installed? > > > > Use CONVERT_EXPR_CODE_P (code1) please. > > Sure. > > > > > Note there are other codes like VIEW_CONVERT_EXPR or > > BIT_FIELD_REF or MEM_REF which involve conversions so I'm not > > sure not comparing types of SSA names is a good idea. > > Note that all these go through operand_compare::operand_equal_p which > considers types accordingly. I would like to go with this patch and > we can eventually come up with a more strict type comparison for SSA > NAMEs. Sure, the fix in itself is OK. Richard. > Martin > > > > > Richard. > > > >> Thanks, > >> Martin > >> > >> gcc/ChangeLog: > >> > >> 2019-11-15 Martin Liska <mliska@suse.cz> > >> > >> PR ipa/92529 > >> * ipa-icf-gimple.c (func_checker::compare_gimple_assign): > >> Compare LHS types of NOP_EXPR. > >> > >> gcc/testsuite/ChangeLog: > >> > >> 2019-11-15 Martin Liska <mliska@suse.cz> > >> > >> PR ipa/92529 > >> * gcc.dg/ipa/pr92529.c: New test. > >> --- > >> gcc/ipa-icf-gimple.c | 7 +++++++ > >> gcc/testsuite/gcc.dg/ipa/pr92529.c | 28 ++++++++++++++++++++++++++++ > >> 2 files changed, 35 insertions(+) > >> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr92529.c > >> > >> >
diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c index ac53a1dfbbf..cf0908b8f12 100644 --- a/gcc/ipa-icf-gimple.c +++ b/gcc/ipa-icf-gimple.c @@ -620,6 +620,13 @@ func_checker::compare_gimple_assign (gimple *s1, gimple *s2) arg1 = gimple_op (s1, i); arg2 = gimple_op (s2, i); + /* LHS types of NOP_EXPR must be compatible. */ + if (code1 == NOP_EXPR && i == 0) + { + if (!compatible_types_p (TREE_TYPE (arg1), TREE_TYPE (arg2))) + return return_false_with_msg ("GIMPLE NOP LHS type mismatch"); + } + if (!compare_operand (arg1, arg2)) return return_false_with_msg ("GIMPLE assignment operands " "are different"); diff --git a/gcc/testsuite/gcc.dg/ipa/pr92529.c b/gcc/testsuite/gcc.dg/ipa/pr92529.c new file mode 100644 index 00000000000..0864f342773 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr92529.c @@ -0,0 +1,28 @@ +/* PR ipa/92529 */ +/* { dg-options "-O2 -fdump-ipa-icf-optimized" } */ + +int +foo(volatile int a) +{ + return (char)a; +} + +int +bar(volatile int a) +{ + return (short)a; +} + +#pragma GCC optimize ("-O0") + +int main(int argc, char **argv) +{ + int r = bar(1000); + __builtin_printf ("global: %d\n", r); + if (r != 1000) + __builtin_abort (); + + return 0; +} + +/* { dg-final { scan-ipa-dump "Equal symbols: 0" "icf" } } */