Re: [PATCH, PR 42706] IPA-SRA should check that all callers pass enough arguments

[Richard Guenther <richard dot guenther at gmail dot com>]

On Thu, Jan 14, 2010 at 5:07 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> this patch addresses a fairly big omission on my part, it adds a
> simple check to IPA-SRA to verify that all callers pass enough
> arguments, because if we later on attempt to modify an argument that
> does not exist we ICE. ?On a related note, I switched off some IPA-SRA
> specific checks in intra-SRA in the analysis part because I added
> another, possibly slightly more expensive one to find recursive calls
> and check them too (though I do not know how to make a testcase to
> trigger that check). ?Since this adds a test for recursive calls in
> the analysis stage anyway, I have also added a variable to remember
> whether there are any, so that we can avoid a third walk through the
> function if there are none.
>
> Bootstrapped and regression tested on x86_64-linux, OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
>
> Martin
>
>
> 2010-01-14 ?Martin Jambor ?<mjambor@suse.cz>
>
> ? ? ? ?PR tree-optimization/42706
> ? ? ? ?* tree-sra.c (encountered_recursive_call): New variable.
> ? ? ? ?(encountered_unchangable_recursive_call): Likewise.
> ? ? ? ?(sra_initialize): Initialize both new variables.
> ? ? ? ?(callsite_has_enough_arguments_p): New function.
> ? ? ? ?(scan_function): Call decl and flags check only for IPA-SRA, check
> ? ? ? ?whether there is a recursive call and whether it has enough arguments.
> ? ? ? ?(all_callers_have_enough_arguments_p): New function.
> ? ? ? ?(convert_callers): Look for recursive calls only when
> ? ? ? ?encountered_recursive_call is set.
> ? ? ? ?(ipa_early_sra): Bail out either if
> ? ? ? ?!all_callers_have_enough_arguments_p or
> ? ? ? ?encountered_unchangable_recursive_call.
>
> ? ? ? ?* testsuite/gcc.dg/ipa/pr42706.c: New testcase.
>
>
> Index: mine/gcc/tree-sra.c
> ===================================================================
> --- mine.orig/gcc/tree-sra.c
> +++ mine/gcc/tree-sra.c
> @@ -258,6 +258,13 @@ static int func_param_count;
> ? ?__builtin_apply_args. ?*/
> ?static bool encountered_apply_args;
>
> +/* Set by scan_function when it finds a recursive call. ?*/
> +static bool encountered_recursive_call;
> +
> +/* Set by scan_function when it finds a recursive call with less actual
> + ? arguments than formal parameters.. ?*/
> +static bool encountered_unchangable_recursive_call;
> +
> ?/* This is a table in which for each basic block and parameter there is a
> ? ?distance (offset + size) in that parameter which is dereferenced and
> ? ?accessed in that BB. ?*/
> @@ -545,6 +552,8 @@ sra_initialize (void)
> ? base_access_vec = pointer_map_create ();
> ? memset (&sra_stats, 0, sizeof (sra_stats));
> ? encountered_apply_args = false;
> + ?encountered_recursive_call = false;
> + ?encountered_unchangable_recursive_call = false;
> ?}
>
> ?/* Hook fed to pointer_map_traverse, deallocate stored vectors. ?*/
> @@ -944,6 +953,14 @@ asm_visit_addr (gimple stmt ATTRIBUTE_UN
> ? return false;
> ?}
>
> +/* Return true iff callsite CALL has at least as many actual arguments as there
> + ? are formal parameters of the function currently processed by IPA-SRA. ?*/
> +
> +static inline bool
> +callsite_has_enough_arguments_p (gimple call)
> +{
> + ?return gimple_call_num_args (call) >= (unsigned) func_param_count;
> +}
>
> ?/* Scan function and look for interesting statements. Return true if any has
> ? ?been found or processed, as indicated by callbacks. ?SCAN_EXPR is a callback
> @@ -1014,15 +1031,24 @@ scan_function (bool (*scan_expr) (tree *
> ? ? ? ? ? ? ? ? ?any |= scan_expr (argp, &gsi, false, data);
> ? ? ? ? ? ? ? ?}
>
> - ? ? ? ? ? ? if (analysis_stage)
> + ? ? ? ? ? ? if (analysis_stage && sra_mode == SRA_MODE_EARLY_IPA)
> ? ? ? ? ? ? ? ?{
> ? ? ? ? ? ? ? ? ?tree dest = gimple_call_fndecl (stmt);
> ? ? ? ? ? ? ? ? ?int flags = gimple_call_flags (stmt);
>
> - ? ? ? ? ? ? ? ? if (dest
> - ? ? ? ? ? ? ? ? ? ? && DECL_BUILT_IN_CLASS (dest) == BUILT_IN_NORMAL
> - ? ? ? ? ? ? ? ? ? ? && DECL_FUNCTION_CODE (dest) == BUILT_IN_APPLY_ARGS)
> - ? ? ? ? ? ? ? ? ? encountered_apply_args = true;
> + ? ? ? ? ? ? ? ? if (dest)
> + ? ? ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? ? ? if (DECL_BUILT_IN_CLASS (dest) == BUILT_IN_NORMAL
> + ? ? ? ? ? ? ? ? ? ? ? ? && DECL_FUNCTION_CODE (dest) == BUILT_IN_APPLY_ARGS)
> + ? ? ? ? ? ? ? ? ? ? ? encountered_apply_args = true;
> + ? ? ? ? ? ? ? ? ? ? if (cgraph_get_node (dest)
> + ? ? ? ? ? ? ? ? ? ? ? ? == cgraph_get_node (current_function_decl))
> + ? ? ? ? ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? ? ? ? ? encountered_recursive_call = true;
> + ? ? ? ? ? ? ? ? ? ? ? ? if (!callsite_has_enough_arguments_p (stmt))
> + ? ? ? ? ? ? ? ? ? ? ? ? ? encountered_unchangable_recursive_call = true;
> + ? ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? ? }
>
> ? ? ? ? ? ? ? ? ?if (final_bbs
> ? ? ? ? ? ? ? ? ? ? ?&& (flags & (ECF_CONST | ECF_PURE)) == 0)
> @@ -3780,6 +3806,21 @@ sra_ipa_reset_debug_stmts (ipa_parm_adju
> ? ? }
> ?}
>
> +/* Return true iff all callers have at least as many actual arguments as there
> + ? are formal parameters in the current function. ?*/
> +
> +static bool
> +all_callers_have_enough_arguments_p (struct cgraph_node *node)
> +{
> + ?struct cgraph_edge *cs;
> + ?for (cs = node->callers; cs; cs = cs->next_caller)
> + ? ?if (!callsite_has_enough_arguments_p (cs->call_stmt))
> + ? ? ?return false;
> +
> + ?return true;
> +}
> +
> +
> ?/* Convert all callers of NODE to pass parameters as given in ADJUSTMENTS. ?*/
>
> ?static void
> @@ -3815,6 +3856,10 @@ convert_callers (struct cgraph_node *nod
> ? BITMAP_FREE (recomputed_callers);
>
> ? current_function_decl = old_cur_fndecl;
> +
> + ?if (!encountered_recursive_call)
> + ? ?return;
> +
> ? FOR_EACH_BB (this_block)
> ? ? {
> ? ? ? gimple_stmt_iterator gsi;
> @@ -3927,6 +3972,14 @@ ipa_early_sra (void)
> ? ? ? goto simple_out;
> ? ? }
>
> + ?if (!all_callers_have_enough_arguments_p (node))
> + ? ?{
> + ? ? ?if (dump_file)
> + ? ? ? fprintf (dump_file, "There are callers with insufficient number of "
> + ? ? ? ? ? ? ? ?"arguments.\n");
> + ? ? ?goto simple_out;
> + ? ?}
> +
> ? bb_dereferences = XCNEWVEC (HOST_WIDE_INT,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? func_param_count
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? * last_basic_block_for_function (cfun));
> @@ -3941,6 +3994,14 @@ ipa_early_sra (void)
> ? ? ? goto out;
> ? ? }
>
> + ?if (encountered_unchangable_recursive_call)
> + ? ?{
> + ? ? ?if (dump_file)
> + ? ? ? fprintf (dump_file, "Function calls itself with insufficient "
> + ? ? ? ? ? ? ? ?"number of arguments.\n");
> + ? ? ?goto out;
> + ? ?}
> +
> ? adjustments = analyze_all_param_acesses ();
> ? if (!adjustments)
> ? ? goto out;
> Index: mine/gcc/testsuite/gcc.dg/ipa/pr42706.c
> ===================================================================
> --- /dev/null
> +++ mine/gcc/testsuite/gcc.dg/ipa/pr42706.c
> @@ -0,0 +1,27 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fno-early-inlining -fipa-sra" ?} */
> +
> +struct S
> +{
> + ?float red;
> + ?int green;
> + ?void *blue;
> +};
> +
> +extern int gi;
> +static int foo ();
> +
> +int
> +bar (void)
> +{
> + ?foo ();
> + ?return 0;
> +}
> +
> +static int
> +foo (struct S s)
> +{
> + ?gi = s.green;
> + ?return 0;
> +}
> +
>