Re: [PATCH] Fix some ICF gimple_call handling issues
On 11/11/2014 12:11 AM, Jakub Jelinek wrote: On Mon, Nov 10, 2014 at 10:08:54PM +0100, Richard Biener wrote: @@ -662,9 +662,49 @@ func_checker::compare_gimple_call (gimpl t1 = gimple_call_fndecl (s1); t2 = gimple_call_fndecl (s2); Just drop these and compare gimple_call_fn only. + tree chain1 = gimple_call_chain (s1); + tree chain2 = gimple_call_chain (s2); + + if ((chain1 && !chain2) || (!chain1 && chain2)) +return return_false_with_msg ("Tree call chains are different"); I miss a compare_operands for the call chain. Otherwise OK. Here is what I've committed after another bootstrap/regtest. Note, I've tried: __attribute__ ((noinline, noclone)) int f1 (int x) { int y = 3, z = 4; __attribute__ ((noinline, noclone)) int f2 (int a) { return a + x + y + z; } return f2 (5); } __attribute__ ((noinline, noclone)) int f3 (int x) { int y = 3, z = 4; __attribute__ ((noinline, noclone)) int f4 (int a) { return a + x + y + z; } return f4 (5); } int main () { if (f1 (9) != 21 || f3 (9) != 21) __builtin_abort (); return 0; } but ICF doesn't optimize this with or without the patch, as the structs aren't the same type (supposedly different alias set?), even when they have the same members laid out the same. Hello Jakub. You are right, more precisely types_compatible_p return false for these two structures. I'll write this situation to my TODO list. Thank you for sending the patch. Martin 2014-11-11 Jakub Jelinek Martin Liska * ipa-icf-gimple.c (func_checker::compare_bb): Fix comment typo. (func_checker::compare_gimple_call): Compare gimple_call_fn, gimple_call_chain, gimple_call_fntype and call flags. testsuite/ * gcc.dg/ubsan/ipa-icf-1.c: New test. * gcc.dg/ipa/ipa-icf-31.c: New test. --- gcc/ipa-icf-gimple.c.jj 2014-10-30 14:42:20.0 +0100 +++ gcc/ipa-icf-gimple.c2014-11-10 19:08:38.339986360 +0100 @@ -554,7 +554,7 @@ func_checker::parse_labels (sem_bb *bb) In general, a collection of equivalence dictionaries is built for types like SSA names, declarations (VAR_DECL, PARM_DECL, ..). This infrastructure - is utilized by every statement-by-stament comparison function. */ + is utilized by every statement-by-statement comparison function. */ bool func_checker::compare_bb (sem_bb *bb1, sem_bb *bb2) @@ -659,12 +659,39 @@ func_checker::compare_gimple_call (gimpl if (gimple_call_num_args (s1) != gimple_call_num_args (s2)) return false; - t1 = gimple_call_fndecl (s1); - t2 = gimple_call_fndecl (s2); - - /* Function pointer variables are not supported yet. */ + t1 = gimple_call_fn (s1); + t2 = gimple_call_fn (s2); if (!compare_operand (t1, t2)) -return return_false(); +return return_false (); + + /* Compare flags. */ + if (gimple_call_internal_p (s1) != gimple_call_internal_p (s2) + || gimple_call_ctrl_altering_p (s1) != gimple_call_ctrl_altering_p (s2) + || gimple_call_tail_p (s1) != gimple_call_tail_p (s2) + || gimple_call_return_slot_opt_p (s1) != gimple_call_return_slot_opt_p (s2) + || gimple_call_from_thunk_p (s1) != gimple_call_from_thunk_p (s2) + || gimple_call_va_arg_pack_p (s1) != gimple_call_va_arg_pack_p (s2) + || gimple_call_alloca_for_var_p (s1) != gimple_call_alloca_for_var_p (s2) + || gimple_call_with_bounds_p (s1) != gimple_call_with_bounds_p (s2)) +return false; + + if (gimple_call_internal_p (s1) + && gimple_call_internal_fn (s1) != gimple_call_internal_fn (s2)) +return false; + + tree fntype1 = gimple_call_fntype (s1); + tree fntype2 = gimple_call_fntype (s2); + if ((fntype1 && !fntype2) + || (!fntype1 && fntype2) + || (fntype1 && !types_compatible_p (fntype1, fntype2))) +return return_false_with_msg ("call function types are not compatible"); + + tree chain1 = gimple_call_chain (s1); + tree chain2 = gimple_call_chain (s2); + if ((chain1 && !chain2) + || (!chain1 && chain2) + || !compare_operand (chain1, chain2)) +return return_false_with_msg ("static call chains are different"); /* Checking of argument. */ for (i = 0; i < gimple_call_num_args (s1); ++i) --- gcc/testsuite/gcc.dg/ubsan/ipa-icf-1.c.jj 2014-11-10 19:00:53.509525071 +0100 +++ gcc/testsuite/gcc.dg/ubsan/ipa-icf-1.c 2014-11-10 19:02:21.836925806 +0100 @@ -0,0 +1,23 @@ +/* { dg-do run } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */ +/* { dg-options "-fsanitize=undefined -fipa-icf" } */ + +__attribute__ ((noinline, noclone)) +int f1 (int x, int y) +{ + return x + y; +} + +__attribute__ ((noinline, noclone)) +int f2 (int x, int y) +{ + return x - y; +} + +int +main () +{ + if (f1 (5, 6) != 11 || f2 (5, 6) != -1) +__builtin_abort (); + return 0; +} --- gcc/testsuite/gcc.dg/ipa/ipa-icf-31.c.jj2014-11-10 18:59:16.604294652 +0100 +++ gcc/testsuite/gcc.dg/ipa/ipa-icf-31.c 2014-11-10 18:59:59.690519616 +0100 @@ -0,0 +1,41 @@ +/* {
Re: [PATCH] Fix some ICF gimple_call handling issues
On Mon, Nov 10, 2014 at 10:08:54PM +0100, Richard Biener wrote: > >@@ -662,9 +662,49 @@ func_checker::compare_gimple_call (gimpl > > t1 = gimple_call_fndecl (s1); > > t2 = gimple_call_fndecl (s2); > > Just drop these and compare gimple_call_fn only. > > >+ tree chain1 = gimple_call_chain (s1); > >+ tree chain2 = gimple_call_chain (s2); > >+ > >+ if ((chain1 && !chain2) || (!chain1 && chain2)) > >+return return_false_with_msg ("Tree call chains are different"); > > I miss a compare_operands for the call chain. > > Otherwise OK. Here is what I've committed after another bootstrap/regtest. Note, I've tried: __attribute__ ((noinline, noclone)) int f1 (int x) { int y = 3, z = 4; __attribute__ ((noinline, noclone)) int f2 (int a) { return a + x + y + z; } return f2 (5); } __attribute__ ((noinline, noclone)) int f3 (int x) { int y = 3, z = 4; __attribute__ ((noinline, noclone)) int f4 (int a) { return a + x + y + z; } return f4 (5); } int main () { if (f1 (9) != 21 || f3 (9) != 21) __builtin_abort (); return 0; } but ICF doesn't optimize this with or without the patch, as the structs aren't the same type (supposedly different alias set?), even when they have the same members laid out the same. 2014-11-11 Jakub Jelinek Martin Liska * ipa-icf-gimple.c (func_checker::compare_bb): Fix comment typo. (func_checker::compare_gimple_call): Compare gimple_call_fn, gimple_call_chain, gimple_call_fntype and call flags. testsuite/ * gcc.dg/ubsan/ipa-icf-1.c: New test. * gcc.dg/ipa/ipa-icf-31.c: New test. --- gcc/ipa-icf-gimple.c.jj 2014-10-30 14:42:20.0 +0100 +++ gcc/ipa-icf-gimple.c2014-11-10 19:08:38.339986360 +0100 @@ -554,7 +554,7 @@ func_checker::parse_labels (sem_bb *bb) In general, a collection of equivalence dictionaries is built for types like SSA names, declarations (VAR_DECL, PARM_DECL, ..). This infrastructure - is utilized by every statement-by-stament comparison function. */ + is utilized by every statement-by-statement comparison function. */ bool func_checker::compare_bb (sem_bb *bb1, sem_bb *bb2) @@ -659,12 +659,39 @@ func_checker::compare_gimple_call (gimpl if (gimple_call_num_args (s1) != gimple_call_num_args (s2)) return false; - t1 = gimple_call_fndecl (s1); - t2 = gimple_call_fndecl (s2); - - /* Function pointer variables are not supported yet. */ + t1 = gimple_call_fn (s1); + t2 = gimple_call_fn (s2); if (!compare_operand (t1, t2)) -return return_false(); +return return_false (); + + /* Compare flags. */ + if (gimple_call_internal_p (s1) != gimple_call_internal_p (s2) + || gimple_call_ctrl_altering_p (s1) != gimple_call_ctrl_altering_p (s2) + || gimple_call_tail_p (s1) != gimple_call_tail_p (s2) + || gimple_call_return_slot_opt_p (s1) != gimple_call_return_slot_opt_p (s2) + || gimple_call_from_thunk_p (s1) != gimple_call_from_thunk_p (s2) + || gimple_call_va_arg_pack_p (s1) != gimple_call_va_arg_pack_p (s2) + || gimple_call_alloca_for_var_p (s1) != gimple_call_alloca_for_var_p (s2) + || gimple_call_with_bounds_p (s1) != gimple_call_with_bounds_p (s2)) +return false; + + if (gimple_call_internal_p (s1) + && gimple_call_internal_fn (s1) != gimple_call_internal_fn (s2)) +return false; + + tree fntype1 = gimple_call_fntype (s1); + tree fntype2 = gimple_call_fntype (s2); + if ((fntype1 && !fntype2) + || (!fntype1 && fntype2) + || (fntype1 && !types_compatible_p (fntype1, fntype2))) +return return_false_with_msg ("call function types are not compatible"); + + tree chain1 = gimple_call_chain (s1); + tree chain2 = gimple_call_chain (s2); + if ((chain1 && !chain2) + || (!chain1 && chain2) + || !compare_operand (chain1, chain2)) +return return_false_with_msg ("static call chains are different"); /* Checking of argument. */ for (i = 0; i < gimple_call_num_args (s1); ++i) --- gcc/testsuite/gcc.dg/ubsan/ipa-icf-1.c.jj 2014-11-10 19:00:53.509525071 +0100 +++ gcc/testsuite/gcc.dg/ubsan/ipa-icf-1.c 2014-11-10 19:02:21.836925806 +0100 @@ -0,0 +1,23 @@ +/* { dg-do run } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */ +/* { dg-options "-fsanitize=undefined -fipa-icf" } */ + +__attribute__ ((noinline, noclone)) +int f1 (int x, int y) +{ + return x + y; +} + +__attribute__ ((noinline, noclone)) +int f2 (int x, int y) +{ + return x - y; +} + +int +main () +{ + if (f1 (5, 6) != 11 || f2 (5, 6) != -1) +__builtin_abort (); + return 0; +} --- gcc/testsuite/gcc.dg/ipa/ipa-icf-31.c.jj2014-11-10 18:59:16.604294652 +0100 +++ gcc/testsuite/gcc.dg/ipa/ipa-icf-31.c 2014-11-10 18:59:59.690519616 +0100 @@ -0,0 +1,41 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -fipa-icf" } */ + +__attribute__ ((noinline, noclone)) +int f1 (int x, int (*p1) (void), int (*p2) (void)) +{ + if (x) +return p1 (); + else +return p2 (); +} + +__attribute
Re: [PATCH] Fix some ICF gimple_call handling issues
> Hi! > > As the following two testcases shows, there are lots of issues in > ICF compare_gimple_call, in particular, it doesn't handle indirect calls > properly (see the ipa-icf-31.c testcase), doesn't handle internal calls > properly (see ubsan/ipa-icf-1.c), didn't check gimple_call flags at all. > > As discussed with Honza, the call chain test (from Martin) is probably > insufficient, I'm open with leaving it out from the patch, but perhaps > what the patch has is better than nothing at all for now. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2014-11-10 Jakub Jelinek > Martin Liska > > * ipa-icf-gimple.c (func_checker::compare_bb): Fix comment typo. > (func_checker::compare_gimple_call): Compare gimple_call_fn, > gimple_call_chain, gimple_call_fntype and call flags. > testsuite/ > * gcc.dg/ubsan/ipa-icf-1.c: New test. > * gcc.dg/ipa/ipa-icf-31.c: New test. OK. > + > + tree chain1 = gimple_call_chain (s1); > + tree chain2 = gimple_call_chain (s2); > + > + if ((chain1 && !chain2) || (!chain1 && chain2)) > +return return_false_with_msg ("Tree call chains are different"); Since gimple_call_chain is actually gimple_val, I think it needs to be compared as operand. Honza
Re: [PATCH] Fix some ICF gimple_call handling issues
On November 10, 2014 9:45:27 PM CET, Jakub Jelinek wrote: >Hi! > >As the following two testcases shows, there are lots of issues in >ICF compare_gimple_call, in particular, it doesn't handle indirect >calls >properly (see the ipa-icf-31.c testcase), doesn't handle internal calls >properly (see ubsan/ipa-icf-1.c), didn't check gimple_call flags at >all. > >As discussed with Honza, the call chain test (from Martin) is probably >insufficient, I'm open with leaving it out from the patch, but perhaps >what the patch has is better than nothing at all for now. > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > >2014-11-10 Jakub Jelinek > Martin Liska > > * ipa-icf-gimple.c (func_checker::compare_bb): Fix comment typo. > (func_checker::compare_gimple_call): Compare gimple_call_fn, > gimple_call_chain, gimple_call_fntype and call flags. >testsuite/ > * gcc.dg/ubsan/ipa-icf-1.c: New test. > * gcc.dg/ipa/ipa-icf-31.c: New test. > >--- gcc/ipa-icf-gimple.c.jj2014-10-30 14:42:20.0 +0100 >+++ gcc/ipa-icf-gimple.c 2014-11-10 19:08:38.339986360 +0100 >@@ -554,7 +554,7 @@ func_checker::parse_labels (sem_bb *bb) > >In general, a collection of equivalence dictionaries is built for types >like SSA names, declarations (VAR_DECL, PARM_DECL, ..). This >infrastructure >- is utilized by every statement-by-stament comparison function. */ >+ is utilized by every statement-by-statement comparison function. >*/ > > bool > func_checker::compare_bb (sem_bb *bb1, sem_bb *bb2) >@@ -662,9 +662,49 @@ func_checker::compare_gimple_call (gimpl > t1 = gimple_call_fndecl (s1); > t2 = gimple_call_fndecl (s2); Just drop these and compare gimple_call_fn only. >- /* Function pointer variables are not supported yet. */ > if (!compare_operand (t1, t2)) >-return return_false(); >+return return_false (); >+ >+ if (t1 == NULL_TREE) >+{ >+ t1 = gimple_call_fn (s1); >+ t2 = gimple_call_fn (s2); >+ if (!compare_operand (t1, t2)) >+ return return_false (); >+} >+ >+ /* Compare flags. */ >+ if (gimple_call_internal_p (s1) != gimple_call_internal_p (s2) >+ || gimple_call_ctrl_altering_p (s1) != >gimple_call_ctrl_altering_p (s2) >+ || gimple_call_tail_p (s1) != gimple_call_tail_p (s2) >+ || gimple_call_return_slot_opt_p (s1) != >gimple_call_return_slot_opt_p (s2) >+ || gimple_call_from_thunk_p (s1) != gimple_call_from_thunk_p >(s2) >+ || gimple_call_va_arg_pack_p (s1) != gimple_call_va_arg_pack_p >(s2) >+ || gimple_call_alloca_for_var_p (s1) != >gimple_call_alloca_for_var_p (s2) >+ || gimple_call_with_bounds_p (s1) != gimple_call_with_bounds_p >(s2)) >+return false; >+ >+ if (gimple_call_internal_p (s1) >+ && gimple_call_internal_fn (s1) != gimple_call_internal_fn (s2)) >+return false; >+ >+ tree fntype1 = gimple_call_fntype (s1); >+ tree fntype2 = gimple_call_fntype (s2); >+ if (fntype1 == NULL_TREE) >+{ >+ if (fntype2) >+ return false; >+} >+ else if (fntype2 == NULL_TREE) >+return false; >+ else if (!types_compatible_p (fntype1, fntype2)) >+return return_false_with_msg ("call function types are not >compatible"); >+ >+ tree chain1 = gimple_call_chain (s1); >+ tree chain2 = gimple_call_chain (s2); >+ >+ if ((chain1 && !chain2) || (!chain1 && chain2)) >+return return_false_with_msg ("Tree call chains are different"); I miss a compare_operands for the call chain. Otherwise OK. Thanks, Richard. > /* Checking of argument. */ > for (i = 0; i < gimple_call_num_args (s1); ++i) >--- gcc/testsuite/gcc.dg/ubsan/ipa-icf-1.c.jj 2014-11-10 >19:00:53.509525071 +0100 >+++ gcc/testsuite/gcc.dg/ubsan/ipa-icf-1.c 2014-11-10 >19:02:21.836925806 +0100 >@@ -0,0 +1,23 @@ >+/* { dg-do run } */ >+/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */ >+/* { dg-options "-fsanitize=undefined -fipa-icf" } */ >+ >+__attribute__ ((noinline, noclone)) >+int f1 (int x, int y) >+{ >+ return x + y; >+} >+ >+__attribute__ ((noinline, noclone)) >+int f2 (int x, int y) >+{ >+ return x - y; >+} >+ >+int >+main () >+{ >+ if (f1 (5, 6) != 11 || f2 (5, 6) != -1) >+__builtin_abort (); >+ return 0; >+} >--- gcc/testsuite/gcc.dg/ipa/ipa-icf-31.c.jj 2014-11-10 >18:59:16.604294652 +0100 >+++ gcc/testsuite/gcc.dg/ipa/ipa-icf-31.c 2014-11-10 18:59:59.690519616 >+0100 >@@ -0,0 +1,41 @@ >+/* { dg-do run } */ >+/* { dg-options "-O2 -fipa-icf" } */ >+ >+__attribute__ ((noinline, noclone)) >+int f1 (int x, int (*p1) (void), int (*p2) (void)) >+{ >+ if (x) >+return p1 (); >+ else >+return p2 (); >+} >+ >+__attribute__ ((noinline, noclone)) >+int f2 (int x, int (*p1) (void), int (*p2) (void)) >+{ >+ if (x) >+return p2 (); >+ else >+return p1 (); >+} >+ >+__attribute__ ((noinline, noclone)) >+int f3 (void) >+{ >+ return 1; >+} >+ >+__attribute__ ((noinline, noclone)) >+int f4 (void) >+{ >+ return 2; >+} >+ >+int >+main () >+{ >+ if (f1 (
[PATCH] Fix some ICF gimple_call handling issues
Hi! As the following two testcases shows, there are lots of issues in ICF compare_gimple_call, in particular, it doesn't handle indirect calls properly (see the ipa-icf-31.c testcase), doesn't handle internal calls properly (see ubsan/ipa-icf-1.c), didn't check gimple_call flags at all. As discussed with Honza, the call chain test (from Martin) is probably insufficient, I'm open with leaving it out from the patch, but perhaps what the patch has is better than nothing at all for now. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2014-11-10 Jakub Jelinek Martin Liska * ipa-icf-gimple.c (func_checker::compare_bb): Fix comment typo. (func_checker::compare_gimple_call): Compare gimple_call_fn, gimple_call_chain, gimple_call_fntype and call flags. testsuite/ * gcc.dg/ubsan/ipa-icf-1.c: New test. * gcc.dg/ipa/ipa-icf-31.c: New test. --- gcc/ipa-icf-gimple.c.jj 2014-10-30 14:42:20.0 +0100 +++ gcc/ipa-icf-gimple.c2014-11-10 19:08:38.339986360 +0100 @@ -554,7 +554,7 @@ func_checker::parse_labels (sem_bb *bb) In general, a collection of equivalence dictionaries is built for types like SSA names, declarations (VAR_DECL, PARM_DECL, ..). This infrastructure - is utilized by every statement-by-stament comparison function. */ + is utilized by every statement-by-statement comparison function. */ bool func_checker::compare_bb (sem_bb *bb1, sem_bb *bb2) @@ -662,9 +662,49 @@ func_checker::compare_gimple_call (gimpl t1 = gimple_call_fndecl (s1); t2 = gimple_call_fndecl (s2); - /* Function pointer variables are not supported yet. */ if (!compare_operand (t1, t2)) -return return_false(); +return return_false (); + + if (t1 == NULL_TREE) +{ + t1 = gimple_call_fn (s1); + t2 = gimple_call_fn (s2); + if (!compare_operand (t1, t2)) + return return_false (); +} + + /* Compare flags. */ + if (gimple_call_internal_p (s1) != gimple_call_internal_p (s2) + || gimple_call_ctrl_altering_p (s1) != gimple_call_ctrl_altering_p (s2) + || gimple_call_tail_p (s1) != gimple_call_tail_p (s2) + || gimple_call_return_slot_opt_p (s1) != gimple_call_return_slot_opt_p (s2) + || gimple_call_from_thunk_p (s1) != gimple_call_from_thunk_p (s2) + || gimple_call_va_arg_pack_p (s1) != gimple_call_va_arg_pack_p (s2) + || gimple_call_alloca_for_var_p (s1) != gimple_call_alloca_for_var_p (s2) + || gimple_call_with_bounds_p (s1) != gimple_call_with_bounds_p (s2)) +return false; + + if (gimple_call_internal_p (s1) + && gimple_call_internal_fn (s1) != gimple_call_internal_fn (s2)) +return false; + + tree fntype1 = gimple_call_fntype (s1); + tree fntype2 = gimple_call_fntype (s2); + if (fntype1 == NULL_TREE) +{ + if (fntype2) + return false; +} + else if (fntype2 == NULL_TREE) +return false; + else if (!types_compatible_p (fntype1, fntype2)) +return return_false_with_msg ("call function types are not compatible"); + + tree chain1 = gimple_call_chain (s1); + tree chain2 = gimple_call_chain (s2); + + if ((chain1 && !chain2) || (!chain1 && chain2)) +return return_false_with_msg ("Tree call chains are different"); /* Checking of argument. */ for (i = 0; i < gimple_call_num_args (s1); ++i) --- gcc/testsuite/gcc.dg/ubsan/ipa-icf-1.c.jj 2014-11-10 19:00:53.509525071 +0100 +++ gcc/testsuite/gcc.dg/ubsan/ipa-icf-1.c 2014-11-10 19:02:21.836925806 +0100 @@ -0,0 +1,23 @@ +/* { dg-do run } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */ +/* { dg-options "-fsanitize=undefined -fipa-icf" } */ + +__attribute__ ((noinline, noclone)) +int f1 (int x, int y) +{ + return x + y; +} + +__attribute__ ((noinline, noclone)) +int f2 (int x, int y) +{ + return x - y; +} + +int +main () +{ + if (f1 (5, 6) != 11 || f2 (5, 6) != -1) +__builtin_abort (); + return 0; +} --- gcc/testsuite/gcc.dg/ipa/ipa-icf-31.c.jj2014-11-10 18:59:16.604294652 +0100 +++ gcc/testsuite/gcc.dg/ipa/ipa-icf-31.c 2014-11-10 18:59:59.690519616 +0100 @@ -0,0 +1,41 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -fipa-icf" } */ + +__attribute__ ((noinline, noclone)) +int f1 (int x, int (*p1) (void), int (*p2) (void)) +{ + if (x) +return p1 (); + else +return p2 (); +} + +__attribute__ ((noinline, noclone)) +int f2 (int x, int (*p1) (void), int (*p2) (void)) +{ + if (x) +return p2 (); + else +return p1 (); +} + +__attribute__ ((noinline, noclone)) +int f3 (void) +{ + return 1; +} + +__attribute__ ((noinline, noclone)) +int f4 (void) +{ + return 2; +} + +int +main () +{ + if (f1 (0, f3, f4) != 2 || f1 (1, f3, f4) != 1 || f2 (0, f3, f4) != 1 + || f2 (1, f3, f4) != 2) +__builtin_abort (); + return 0; +} Jakub