On Fri, 31 Jan 2025, Jakub Jelinek wrote:
> Hi!
>
> compare_operand uses operand_equal_p under the hood, which e.g. for
> INTEGER_CSTs will just match the values rather regardless of their types.
> Now, in many comparing the type is redundant, if we have
> x_2 = y_3 + 1;
> we've already compared the type for the lhs and also for rhs1, there won't
> be any surprises on rhs2.
> As noted in the PR, there are cases where the type of the operand is the
> sole place of information and we don't want to ICF merge functions if the
> types differ.
> One case is stdarg functions, arguments passed to ..., it is different
> if we pass 1, 1L, 1LL.
> Another case are the K&R unprototyped functions (sure, gone in C23).
> And yet another case are inline asm operands, "r" (1) is different from "r"
> (1L) from "r" (1LL).
>
> So, the following patch determines based on lack of fntype (e.g. for
> internal functions), or on !prototype_p, or on stdarg_p (in that case
> using number of named arguments) which arguments need to have type checked
> and does that, plus compares types on inline asm operands (maybe it would be
> enough to do that just for input operands but we have just a routine to
> handle both and I didn't feel we need to differentiate).
>
> Fuirthermore, I've noticed fntype{1,2} isn't actually compared if it is a
> direct call (gimple_call_fndecl is non-NULL). That is wrong too, we could
> have
> void (*fn) (int, long long) = (void (*) (int, long long)) foo;
> fn (1, 1LL);
> in one case and
> void (*fn) (long long, int) = (void (*) (long long, int)) foo;
> fn (1LL, 1);
> in another, both folded into a direct call of foo with different
> gimple_call_fntype. Sure, one of them would be UB at runtime (or both), but
> what if we ICF merge it into something that into the one UB at runtime
> and the program actually calls the correct one only?
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2025-01-31 Jakub Jelinek <ja...@redhat.com>
>
> PR ipa/117432
> * ipa-icf-gimple.cc (func_checker::compare_asm_inputs_outputs):
> Also return_false if operands have incompatible types.
> (func_checker::compare_gimple_call): Also check fntype1 vs. fntype2
> compatibility if at least one of the calls has different
> gimple_call_fntype from the FUNCTION_TYPE of the called decl. For
> calls to non-prototyped calls or for stdarg_p functions after the
> last named argument (if any) check type compatibility of call
> arguments.
>
> * gcc.c-torture/execute/pr117432.c: New test.
> * gcc.target/i386/pr117432.c: New test.
>
> --- gcc/ipa-icf-gimple.cc.jj 2025-01-02 11:23:16.334519404 +0100
> +++ gcc/ipa-icf-gimple.cc 2025-01-30 16:21:05.782127011 +0100
> @@ -459,7 +459,9 @@ func_checker::compare_asm_inputs_outputs
> return false;
>
> if (!compare_operand (TREE_VALUE (t1), TREE_VALUE (t2),
> - get_operand_access_type (map, t1)))
> + get_operand_access_type (map, t1))
> + || !types_compatible_p (TREE_TYPE (TREE_VALUE (t1)),
> + TREE_TYPE (TREE_VALUE (t2))))
> return return_false ();
>
> tree p1 = TREE_PURPOSE (t1);
> @@ -718,8 +720,11 @@ func_checker::compare_gimple_call (gcall
>
> /* For direct calls we verify that types are compatible so if we matched
> callees, callers must match, too. For indirect calls however verify
> - function type. */
> - if (!gimple_call_fndecl (s1))
> + function type. And also verify it for direct calls with some different
> + fntype. */
> + if (!gimple_call_fndecl (s1)
> + || TREE_TYPE (TREE_TYPE (t1)) != fntype1
> + || TREE_TYPE (TREE_TYPE (t2)) != fntype2)
I think we want to always compare the ABI relevant fntypes. It seems
we can arrive here with internal function calls where t1/t2 are
"somthing" (NULL?). I guess doing this as else {} of the
if (gimple_call_internal_p (s1) (with gimple_call_internal_fn compare
in a conditiona if) would be a lot clearer?
> {
> if ((fntype1 && !fntype2)
> || (!fntype1 && fntype2)
> @@ -738,6 +743,24 @@ func_checker::compare_gimple_call (gcall
> get_operand_access_type (&map, chain1)))
> return return_false_with_msg ("static call chains are different");
>
> + unsigned check_arg_types_from = gimple_call_num_args (s1);
> + if (!fntype1
> + || !fntype2
> + || !prototype_p (fntype1)
> + || !prototype_p (fntype2))
> + check_arg_types_from = 0;
> + else if (stdarg_p (fntype1))
> + {
> + check_arg_types_from = list_length (TYPE_ARG_TYPES (fntype1));
> + if (stdarg_p (fntype2))
> + {
> + unsigned n = list_length (TYPE_ARG_TYPES (fntype2));
> + check_arg_types_from = MIN (check_arg_types_from, n);
> + }
> + }
> + else if (stdarg_p (fntype2))
> + check_arg_types_from = list_length (TYPE_ARG_TYPES (fntype2));
> +
> /* Checking of argument. */
> for (i = 0; i < gimple_call_num_args (s1); ++i)
> {
> @@ -746,6 +769,10 @@ func_checker::compare_gimple_call (gcall
>
> if (!compare_operand (t1, t2, get_operand_access_type (&map, t1)))
> return return_false_with_msg ("GIMPLE call operands are different");
> + if (i >= check_arg_types_from
> + && !types_compatible_p (TREE_TYPE (t1), TREE_TYPE (t2)))
> + return return_false_with_msg ("GIMPLE call operand types are "
> + "different");
> }
The rest looks OK to me.
Richard.
> /* Return value checking. */
> --- gcc/testsuite/gcc.c-torture/execute/pr117432.c.jj 2025-01-30
> 17:34:51.745191701 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr117432.c 2025-01-30
> 16:48:45.050300148 +0100
> @@ -0,0 +1,71 @@
> +/* PR ipa/117432 */
> +
> +#include <stdarg.h>
> +
> +long long r;
> +
> +__attribute__((noipa)) void
> +baz (int tag, ...)
> +{
> + va_list ap;
> + va_start (ap, tag);
> + if (!r)
> + r = va_arg (ap, long long);
> + else
> + r = va_arg (ap, int);
> + va_end (ap);
> +}
> +
> +void
> +foo (void)
> +{
> + baz (1, -1, 0);
> +}
> +
> +void
> +bar (void)
> +{
> + baz (1, -1LL, 0);
> +}
> +
> +__attribute__((noipa)) void
> +qux (...)
> +{
> + va_list ap;
> + va_start (ap);
> + if (!r)
> + r = va_arg (ap, long long);
> + else
> + r = va_arg (ap, int);
> + va_end (ap);
> +}
> +
> +void
> +corge (void)
> +{
> + qux (-2, 0);
> +}
> +
> +void
> +fred (void)
> +{
> + qux (-2LL, 0);
> +}
> +
> +int
> +main ()
> +{
> + bar ();
> + if (r != -1LL)
> + __builtin_abort ();
> + foo ();
> + if (r != -1)
> + __builtin_abort ();
> + r = 0;
> + fred ();
> + if (r != -2LL)
> + __builtin_abort ();
> + corge ();
> + if (r != -2)
> + __builtin_abort ();
> +}
> --- gcc/testsuite/gcc.target/i386/pr117432.c.jj 2025-01-30
> 17:34:32.427457117 +0100
> +++ gcc/testsuite/gcc.target/i386/pr117432.c 2025-01-30 17:33:38.937192052
> +0100
> @@ -0,0 +1,17 @@
> +/* PR ipa/117432 */
> +/* { dg-do compile { target lp64 } } */
> +/* { dg-options "-O2" } */
> +/* { dg-final { scan-assembler "# myinsn %rax" } } */
> +/* { dg-final { scan-assembler "# myinsn %eax" } } */
> +
> +void
> +foo (void)
> +{
> + asm volatile ("# myinsn %0" : : "r" (-42L));
> +}
> +
> +void
> +bar (void)
> +{
> + asm volatile ("# myinsn %0" : : "r" (-42));
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
