[PATCH] ipa-sra: Do not remove return values needed because of non-call EH (PR 98690)

[Richard Biener]

On Mon, Jan 18, 2021 at 8:27 PM Martin Jambor <mjambor@suse.cz> wrote:
>
> Hi,
>
> IPA-SRA already contains a check to figure out that an otherwise dead
> parameter is actually required because of non-call exceptions, but it
> is not present at the equivalent spot where SRA figures out whether
> the return statement is used for anything useful.  This patch adds
> that condition there.
>
> Unfortunately, even though this patch should be good enough for any
> normal (I'd even say reasonable) use of the compiler, it hints that
> when the user manually switches all sorts of DCE, IPA-SRA would
> probably leave behind problematic statements manipulating what
> originally were return values, just like it does for parameters (PR
> 93385).  Fixing this properly might unfortunately be a separate issue
> from the mentioned bug because the LHS of a call is changed during
> call redirection and the caller often is not a clone.  But I'll see
> what I can do.
>
> Meanwhile, the patch below has been bootstrapped and tested on x86_64.
> OK for trunk and then for the gcc-10 branch?

OK.

Thanks,
Richard.

> Thanks,
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2021-01-18  Martin Jambor  <mjambor@suse.cz>
>
>         PR ipa/98690
>         * ipa-sra.c (ssa_name_only_returned_p): New parameter fun.  Check
>         whether non-call exceptions allow removal of a statement.
>         (isra_analyze_call): Pass the appropriate function to
>         ssa_name_only_returned_p.
>
> gcc/testsuite/ChangeLog:
>
> 2021-01-18  Martin Jambor  <mjambor@suse.cz>
>
>         PR ipa/98690
>         * g++.dg/ipa/pr98690.C: New test.
> ---
>  gcc/ipa-sra.c                      | 20 +++++++++++---------
>  gcc/testsuite/g++.dg/ipa/pr98690.C | 27 +++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 9 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr98690.C
>
> diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c
> index 5d2c0dfce53..1571921cb48 100644
> --- a/gcc/ipa-sra.c
> +++ b/gcc/ipa-sra.c
> @@ -1952,13 +1952,13 @@ scan_function (cgraph_node *node, struct function *fun)
>      }
>  }
>
> -/* Return true if SSA_NAME NAME is only used in return statements, or if
> -   results of any operations it is involved in are only used in return
> -   statements.  ANALYZED is a bitmap that tracks which SSA names we have
> -   already started investigating.  */
> +/* Return true if SSA_NAME NAME of function described by FUN is only used in
> +   return statements, or if results of any operations it is involved in are
> +   only used in return statements.  ANALYZED is a bitmap that tracks which SSA
> +   names we have already started investigating.  */
>
>  static bool
> -ssa_name_only_returned_p (tree name, bitmap analyzed)
> +ssa_name_only_returned_p (function *fun, tree name, bitmap analyzed)
>  {
>    bool res = true;
>    imm_use_iterator imm_iter;
> @@ -1978,8 +1978,9 @@ ssa_name_only_returned_p (tree name, bitmap analyzed)
>               break;
>             }
>         }
> -      else if ((is_gimple_assign (stmt) && !gimple_has_volatile_ops (stmt))
> -              || gimple_code (stmt) == GIMPLE_PHI)
> +      else if (!stmt_unremovable_because_of_non_call_eh_p (fun, stmt)
> +              && ((is_gimple_assign (stmt) && !gimple_has_volatile_ops (stmt))
> +                  || gimple_code (stmt) == GIMPLE_PHI))
>         {
>           /* TODO: And perhaps for const function calls too?  */
>           tree lhs;
> @@ -1995,7 +1996,7 @@ ssa_name_only_returned_p (tree name, bitmap analyzed)
>             }
>           gcc_assert (!gimple_vdef (stmt));
>           if (bitmap_set_bit (analyzed, SSA_NAME_VERSION (lhs))
> -             && !ssa_name_only_returned_p (lhs, analyzed))
> +             && !ssa_name_only_returned_p (fun, lhs, analyzed))
>             {
>               res = false;
>               break;
> @@ -2049,7 +2050,8 @@ isra_analyze_call (cgraph_edge *cs)
>        if (TREE_CODE (lhs) == SSA_NAME)
>         {
>           bitmap analyzed = BITMAP_ALLOC (NULL);
> -         if (ssa_name_only_returned_p (lhs, analyzed))
> +         if (ssa_name_only_returned_p (DECL_STRUCT_FUNCTION (cs->caller->decl),
> +                                       lhs, analyzed))
>             csum->m_return_returned = true;
>           BITMAP_FREE (analyzed);
>         }
> diff --git a/gcc/testsuite/g++.dg/ipa/pr98690.C b/gcc/testsuite/g++.dg/ipa/pr98690.C
> new file mode 100644
> index 00000000000..004418e5b40
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ipa/pr98690.C
> @@ -0,0 +1,27 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fnon-call-exceptions" } */
> +
> +int g;
> +volatile int v;
> +
> +static int * __attribute__((noinline))
> +almost_useless_return (void)
> +{
> +  v = 1;
> +  return &g;
> +}
> +
> +static void __attribute__((noinline))
> +foo (void)
> +{
> +  int *p = almost_useless_return ();
> +  int i = *p;
> +  v = 2;
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  foo ();
> +  return 0;
> +}
> --
> 2.29.2
>