On November 10, 2014 9:45:27 PM CET, Jakub Jelinek <ja...@redhat.com> 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 <ja...@redhat.com>
> Martin Liska <mli...@suse.cz>
>
> * 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.000000000 +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 (0, f3, f4) != 2 || f1 (1, f3, f4) != 1 || f2 (0, f3, f4) !=
>1
>+ || f2 (1, f3, f4) != 2)
>+ __builtin_abort ();
>+ return 0;
>+}
>
> Jakub
