This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: SRA: don't drop clobbers


On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> with this patch on top of
> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html
> we finally warn for the testcase of PR 60517.
>
> The new function is copied from init_subtree_with_zero right above. I guess
> it might be possible to merge them into a single function, if desired. I
> don't understand the debug stuff, but hopefully by keeping the functions
> similar enough it shouldn't be too broken.
>
> When we see a clobber during scalarization, instead of dropping it, we add a
> clobber to the new variable, which the previous patch turns into an SSA_NAME
> with a default def. Then either we reach uninit and warn, or the variable
> appears in a PHI and CCP optimizes.

What's the point of a clobber of sth that was scalarized away?  So ...
can you please explain in more detail?

> Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-linux-gnu.

The new function needs a comment.

Richard.

>
> 2014-07-01  Marc Glisse  <marc.glisse@inria.fr>
>
>         PR c++/60517
>         PR tree-optimization/60770
> gcc/
>         * tree-sra.c (clobber_subtree): New function.
>         (sra_modify_constructor_assign): Call it.
> gcc/testsuite/
>         * g++.dg/tree-ssa/pr60517.C: New file.
>
> --
> Marc Glisse
> Index: gcc/tree-sra.c
> ===================================================================
> --- gcc/tree-sra.c      (revision 212126)
> +++ gcc/tree-sra.c      (working copy)
> @@ -2727,20 +2727,58 @@ init_subtree_with_zero (struct access *a
>        if (insert_after)
>         gsi_insert_after (gsi, ds, GSI_NEW_STMT);
>        else
>         gsi_insert_before (gsi, ds, GSI_SAME_STMT);
>      }
>
>    for (child = access->first_child; child; child = child->next_sibling)
>      init_subtree_with_zero (child, gsi, insert_after, loc);
>  }
>
> +static void
> +clobber_subtree (struct access *access, gimple_stmt_iterator *gsi,
> +               bool insert_after, location_t loc)
> +
> +{
> +  struct access *child;
> +
> +  if (access->grp_to_be_replaced)
> +    {
> +      tree rep = get_access_replacement (access);
> +      tree clobber = build_constructor (access->type, NULL);
> +      TREE_THIS_VOLATILE (clobber) = 1;
> +      gimple stmt = gimple_build_assign (rep, clobber);
> +
> +      if (insert_after)
> +       gsi_insert_after (gsi, stmt, GSI_NEW_STMT);
> +      else
> +       gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
> +      update_stmt (stmt);
> +      gimple_set_location (stmt, loc);
> +    }
> +  else if (access->grp_to_be_debug_replaced)
> +    {
> +      tree rep = get_access_replacement (access);
> +      tree clobber = build_constructor (access->type, NULL);
> +      TREE_THIS_VOLATILE (clobber) = 1;
> +      gimple ds = gimple_build_debug_bind (rep, clobber, gsi_stmt (*gsi));
> +
> +      if (insert_after)
> +       gsi_insert_after (gsi, ds, GSI_NEW_STMT);
> +      else
> +       gsi_insert_before (gsi, ds, GSI_SAME_STMT);
> +    }
> +
> +  for (child = access->first_child; child; child = child->next_sibling)
> +    clobber_subtree (child, gsi, insert_after, loc);
> +}
> +
>  /* Search for an access representative for the given expression EXPR and
>     return it or NULL if it cannot be found.  */
>
>  static struct access *
>  get_access_for_expr (tree expr)
>  {
>    HOST_WIDE_INT offset, size, max_size;
>    tree base;
>
>    /* FIXME: This should not be necessary but Ada produces V_C_Es with a
> type of
> @@ -3039,43 +3077,41 @@ enum assignment_mod_result { SRA_AM_NONE
>                              SRA_AM_REMOVED };  /* stmt eliminated */
>
>  /* Modify assignments with a CONSTRUCTOR on their RHS.  STMT contains a
> pointer
>     to the assignment and GSI is the statement iterator pointing at it.
> Returns
>     the same values as sra_modify_assign.  */
>
>  static enum assignment_mod_result
>  sra_modify_constructor_assign (gimple *stmt, gimple_stmt_iterator *gsi)
>  {
>    tree lhs = gimple_assign_lhs (*stmt);
> -  struct access *acc;
> -  location_t loc;
> -
> -  acc = get_access_for_expr (lhs);
> +  struct access *acc = get_access_for_expr (lhs);
>    if (!acc)
>      return SRA_AM_NONE;
> +  location_t loc = gimple_location (*stmt);
>
>    if (gimple_clobber_p (*stmt))
>      {
> -      /* Remove clobbers of fully scalarized variables, otherwise
> -        do nothing.  */
> +      /* Clobber the replacement variable.  */
> +      clobber_subtree (acc, gsi, !acc->grp_covered, loc);
> +      /* Remove clobbers of fully scalarized variables, they are dead.  */
>        if (acc->grp_covered)
>         {
>           unlink_stmt_vdef (*stmt);
>           gsi_remove (gsi, true);
>           release_defs (*stmt);
>           return SRA_AM_REMOVED;
>         }
>        else
> -       return SRA_AM_NONE;
> +       return SRA_AM_MODIFIED;
>      }
>
> -  loc = gimple_location (*stmt);
>    if (vec_safe_length (CONSTRUCTOR_ELTS (gimple_assign_rhs1 (*stmt))) > 0)
>      {
>        /* I have never seen this code path trigger but if it can happen the
>          following should handle it gracefully.  */
>        if (access_has_children_p (acc))
>         generate_subtree_copies (acc->first_child, lhs, acc->offset, 0, 0,
> gsi,
>                                  true, true, loc);
>        return SRA_AM_MODIFIED;
>      }
>
> Index: gcc/testsuite/g++.dg/tree-ssa/pr60517.C
> ===================================================================
> --- gcc/testsuite/g++.dg/tree-ssa/pr60517.C     (revision 0)
> +++ gcc/testsuite/g++.dg/tree-ssa/pr60517.C     (working copy)
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -Wall" } */
> +
> +struct B {
> +    double x[2];
> +};
> +struct A {
> +    B b;
> +    B getB() { return b; }
> +};
> +
> +double foo(A a) {
> +    double * x = &(a.getB().x[0]);
> +    return x[0]; /* { dg-warning "" } */
> +}
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]