On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse <marc.gli...@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.
>
> Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-linux-gnu.
>
>
> 2014-07-01 Marc Glisse <marc.gli...@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);
> }
>
Missing comment.
> +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)
> + {
Why would we care to create clobbers for debug stmts?! Are those
even valid?
Otherwise this looks good I think.
Richard.
> + 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 "" } */
> +}
>
