Re: [PATCH] ipa-sra: Do not bail out when callers cannot be cloned

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Apr 27, 2021 at 5:29 PM Martin Jambor <mjambor@suse.cz> wrote:
>
> Hi,
>
> IPA-SRA fails to produce (very simple) edge summaries when a caller
> cannot be cloned or its signature cannot be changed which makes it
> less powerful for no good reason.  This patch fixes that problem.
>
> Bootstrapped, LTO-bootstrapped and tested on x86_64-linux.  OK for trunk?

OK.

> A common reason why we think that a function cannot change its
> signature is presence of function type attributes.  I dumped those
> that caused this in our testsuite on x86_64 and got:
>
>   - access
>   - alloc_align
>   - alloc_size
>   - externally_visible
>   - fn spec
>   - force_align_arg_pointer
>   - format
>   - interrupt
>   - ms_abi
>   - no_caller_saved_registers
>   - nocf_check
>   - nonnull
>   - regparm
>   - returns_nonnull
>   - sysv_abi
>   - transaction_callable
>   - transaction_may_cancel_outer
>   - transaction_pure
>   - transaction_safe
>   - transaction_unsafe
>   - type generic
>   - warn_unused_result
>
> and on an Aarch64 I have also seen aarch64_vector_pcs.
>
> Allowing to clone and modify functions with some of these parameters
> like returns_nonull should be easy (it still should probably be
> dropped if the clone does not return anything at all) while doing so
> for attributes like fnspec would need massaging their value.  I am not
> sure what to think of the transaction related ones.

I guess one can classify attributes in those that can survive regardless
of signature changes, those that can be safely dropped (safely as in
no wrong-code, only missed optimizations) and those that can be
transformed (fixing missed optimizations).

I suppose some global attribute_handler[] indexed by some new
attribute key enum providing a class interface so an attribute
can implement its own IPA signature transform method (defaulting
to dropping) and predicate on whether it wants to be
exempted from IPA transforms (default to true) would be nice to have.

Unfortunately attribute handling is scattered amongst frontends at
the moment...

It would also finally allow the attributes to be stored with their ID,
not requiring string compares and eventually allowing a better
data structure for searching.

> (I have seen IPA-SRA remove unused return values of functions marked
> as malloc, so the alloc_align could also be dealt with but I am not
> sure how important it is.)
>
> Thanks,
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2021-04-12  Martin Jambor  <mjambor@suse.cz>
>
>         * ipa-sra.c (ipa_sra_dump_all_summaries): Dump edge summaries even
>         when there is no function summary.
>         (ipa_sra_summarize_function): produce edge summaries even when
>         bailing out early.
>
> gcc/testsuite/ChangeLog:
>
> 2021-04-12  Martin Jambor  <mjambor@suse.cz>
>
>         * gcc.dg/ipa/ipa-sra-1.c (main): Revert change done by
>         05193687dde, make the argv again pointer to an array.
> ---
>  gcc/ipa-sra.c                        | 45 +++++++++++++++-------------
>  gcc/testsuite/gcc.dg/ipa/ipa-sra-1.c |  2 +-
>  2 files changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c
> index 7a89906cee6..3f90d4d81b6 100644
> --- a/gcc/ipa-sra.c
> +++ b/gcc/ipa-sra.c
> @@ -2795,27 +2795,27 @@ ipa_sra_dump_all_summaries (FILE *f)
>
>        isra_func_summary *ifs = func_sums->get (node);
>        if (!ifs)
> -       {
> -         fprintf (f, "  Function does not have any associated IPA-SRA "
> -                  "summary\n");
> -         continue;
> -       }
> -      if (!ifs->m_candidate)
> -       {
> -         fprintf (f, "  Not a candidate function\n");
> -         continue;
> -       }
> -      if (ifs->m_returns_value)
> -         fprintf (f, "  Returns value\n");
> -      if (vec_safe_is_empty (ifs->m_parameters))
> -       fprintf (f, "  No parameter information. \n");
> +       fprintf (f, "  Function does not have any associated IPA-SRA "
> +                "summary\n");
>        else
> -       for (unsigned i = 0; i < ifs->m_parameters->length (); ++i)
> -         {
> -           fprintf (f, "  Descriptor for parameter %i:\n", i);
> -           dump_isra_param_descriptor (f, &(*ifs->m_parameters)[i]);
> -         }
> -      fprintf (f, "\n");
> +       {
> +         if (!ifs->m_candidate)
> +           {
> +             fprintf (f, "  Not a candidate function\n");
> +             continue;
> +           }
> +         if (ifs->m_returns_value)
> +           fprintf (f, "  Returns value\n");
> +         if (vec_safe_is_empty (ifs->m_parameters))
> +           fprintf (f, "  No parameter information. \n");
> +         else
> +           for (unsigned i = 0; i < ifs->m_parameters->length (); ++i)
> +             {
> +               fprintf (f, "  Descriptor for parameter %i:\n", i);
> +               dump_isra_param_descriptor (f, &(*ifs->m_parameters)[i]);
> +             }
> +         fprintf (f, "\n");
> +       }
>
>        struct cgraph_edge *cs;
>        for (cs = node->callees; cs; cs = cs->next_callee)
> @@ -4063,7 +4063,10 @@ ipa_sra_summarize_function (cgraph_node *node)
>      fprintf (dump_file, "Creating summary for %s/%i:\n", node->name (),
>              node->order);
>    if (!ipa_sra_preliminary_function_checks (node))
> -    return;
> +    {
> +      isra_analyze_all_outgoing_calls (node);
> +      return;
> +    }
>    gcc_obstack_init (&gensum_obstack);
>    isra_func_summary *ifs = func_sums->get_create (node);
>    ifs->m_candidate = true;
> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-1.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-1.c
> index df7e356daf3..4a22e3978f9 100644
> --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-1.c
> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-1.c
> @@ -24,7 +24,7 @@ ox (struct bovid cow)
>  }
>
>  int
> -main (int argc, char **argv)
> +main (int argc, char *argv[])
>  {
>    struct bovid cow;
>
> --
> 2.31.1
>