Re: [PATCH] tree-sra: Avoid returns of references to SRA candidates

[Andrew Pinski <pinskia@gmail.com>]

On Mon, Nov 27, 2023 at 10:16 AM Martin Jambor <mjambor@suse.cz> wrote:
>
> Hi,
>
> The enhancement to address PR 109849 contained an importsnt thinko,
> and that any reference that is passed to a function and does not
> escape, must also not happen to be aliased by the return value of the
> function.  This has quickly transpired as bugs PR 112711 and PR
> 112721.
>
> Just as IPA-modref does a good enough job to allow us to rely on the
> escaped set of variables, it sems to be doing well also on updating
> EAF_NOT_RETURNED_DIRECTLY call argument flag which happens to address
> exactly the situation we need to avoid.  Of course, if a call
> statement ignores any returned value, we also do not need to check the
> flag.
>
> Hopefully this does not pessimize things too much, I have verified
> that the PR 109849 testcae remains quick and so should also the
> benchmark it is derived from.
>
> The patch has passed bootstrap and testing on x86_64-linux, OK for
> master?
>
> Thanks,
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2023-11-27  Martin Jambor  <mjambor@suse.cz>
>
>         PR tree-optimization/112711
>         PR tree-optimization/112721
>         * tree-sra.cc (build_access_from_call_arg): New parameter
>         CAN_BE_RETURNED, disqualify any candidate passed by reference if it is
>         true.  Adjust leading comment.
>         (scan_function): Pass appropriate value to CAN_BE_RETURNED of
>         build_access_from_call_arg.
>
> gcc/testsuite/ChangeLog:
>
> 2023-11-27  Martin Jambor  <mjambor@suse.cz>
>
>         PR tree-optimization/112711
>         PR tree-optimization/112721
>         * g++.dg/tree-ssa/pr112711.C: New test.
>         * gcc.dg/tree-ssa/pr112721.c: Likewise.
> ---
>  gcc/testsuite/g++.dg/tree-ssa/pr112711.C | 31 ++++++++++++++++++
>  gcc/testsuite/gcc.dg/tree-ssa/pr112721.c | 26 +++++++++++++++
>  gcc/tree-sra.cc                          | 40 ++++++++++++++++++------
>  3 files changed, 88 insertions(+), 9 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/tree-ssa/pr112711.C
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr112721.c
>
> diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr112711.C b/gcc/testsuite/g++.dg/tree-ssa/pr112711.C
> new file mode 100644
> index 00000000000..c04524b04a7
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/tree-ssa/pr112711.C
> @@ -0,0 +1,31 @@
> +/* { dg-do run } */
> +/* { dg-options "-O1" } */
> +
> +typedef          int i32;
> +typedef unsigned int u32;
> +
> +static inline void write_i32(void *memory, i32 value) {
> +  // swap i32 bytes as if it was u32:
> +  u32 u_value = value;
> +  value = __builtin_bswap32(u_value);
> +
> +  // llvm infers '1' alignment from destination type
> +  __builtin_memcpy(__builtin_assume_aligned(memory, 1), &value, sizeof(value));
> +}
> +
> +__attribute__((noipa))
> +static void bug (void) {
> +  #define assert_eq(lhs, rhs) if (lhs != rhs) __builtin_trap()
> +
> +  unsigned char data[5];
> +  write_i32(data, -1362446643);
> +  assert_eq(data[0], 0xAE);
> +  assert_eq(data[1], 0xCA);
> +  write_i32(data + 1, -1362446643);
> +  assert_eq(data[1], 0xAE);
> +}

Only a comment on this testcase, it is only valid for little-endian
and 32bit int targets.
You can modify it to fix it for both though.

Thanks,
Andrew

> +
> +int main() {
> +    bug();
> +    return 0;
> +}
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr112721.c b/gcc/testsuite/gcc.dg/tree-ssa/pr112721.c
> new file mode 100644
> index 00000000000..adf62613266
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr112721.c
> @@ -0,0 +1,26 @@
> +/* { dg-do run } */
> +/* { dg-options "-O1" } */
> +
> +unsigned * volatile gv;
> +
> +struct a {
> +  int b;
> +};
> +int c, e;
> +long d;
> +unsigned * __attribute__((noinline))
> +f(unsigned *g) {
> +  for (; c;)
> +    e = d;
> +  return gv ? gv : g;
> +}
> +int main() {
> +  int *h;
> +  struct a i = {8};
> +  int *j = &i.b;
> +  h = (unsigned *) f(j);
> +  *h = 0;
> +  if (i.b != 0)
> +    __builtin_abort ();
> +  return 0;
> +}
> diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
> index 3a0d52675fe..6a759783990 100644
> --- a/gcc/tree-sra.cc
> +++ b/gcc/tree-sra.cc
> @@ -1268,18 +1268,27 @@ abnormal_edge_after_stmt_p (gimple *stmt, enum out_edge_check *oe_check)
>  }
>
>  /* Scan expression EXPR which is an argument of a call and create access
> -   structures for all accesses to candidates for scalarization.  Return true if
> -   any access has been inserted.  STMT must be the statement from which the
> -   expression is taken.  */
> +   structures for all accesses to candidates for scalarization.  Return true
> +   if any access has been inserted.  STMT must be the statement from which the
> +   expression is taken.  CAN_BE_RETURNED must be true if call argument flags
> +   do not rule out that the argument is directly returned.  OE_CHECK is used
> +   to remember result of a test for abnormal outgoing edges after this
> +   statement.  */
>
>  static bool
> -build_access_from_call_arg (tree expr, gimple *stmt,
> +build_access_from_call_arg (tree expr, gimple *stmt, bool can_be_returned,
>                             enum out_edge_check *oe_check)
>  {
>    if (TREE_CODE (expr) == ADDR_EXPR)
>      {
>        tree base = get_base_address (TREE_OPERAND (expr, 0));
>
> +      if (can_be_returned)
> +       {
> +         disqualify_base_of_expr (base, "Address possibly returned, "
> +                                  "leading to an alis SRA may not know.");
> +         return false;
> +       }
>        if (abnormal_edge_after_stmt_p (stmt, oe_check))
>         {
>           disqualify_base_of_expr (base, "May lead to need to add statements "
> @@ -1508,12 +1517,25 @@ scan_function (void)
>             case GIMPLE_CALL:
>               {
>                 enum out_edge_check oe_check = SRA_OUTGOING_EDGES_UNCHECKED;
> -               for (i = 0; i < gimple_call_num_args (stmt); i++)
> -                 ret |= build_access_from_call_arg (gimple_call_arg (stmt, i),
> -                                                    stmt, &oe_check);
> +               gcall *call = as_a <gcall *> (stmt);
> +               for (i = 0; i < gimple_call_num_args (call); i++)
> +                 {
> +                   bool can_be_returned;
> +                   if (gimple_call_lhs (call))
> +                     {
> +                       int af = gimple_call_arg_flags (call, i);
> +                       can_be_returned = !(af & EAF_NOT_RETURNED_DIRECTLY);
> +                     }
> +                   else
> +                     can_be_returned = false;
> +                   ret |= build_access_from_call_arg (gimple_call_arg (call,
> +                                                                       i),
> +                                                      stmt, can_be_returned,
> +                                                      &oe_check);
> +                 }
>                 if (gimple_call_chain(stmt))
> -                 ret |= build_access_from_call_arg (gimple_call_chain(stmt),
> -                                                    stmt, &oe_check);
> +                 ret |= build_access_from_call_arg (gimple_call_chain(call),
> +                                                    stmt, false,  &oe_check);
>               }
>
>               t = gimple_call_lhs (stmt);
> --
> 2.43.0
>