| Message ID | alpine.DEB.2.02.1406300119080.8326@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
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 "" } */ > +} >
On Mon, 7 Jul 2014, Richard Biener wrote: > 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? The main idea of these patches is that when we read from a place that was clobbered, instead of dropping the clobber and reading what was there before, we can use a variable with a default definition to mark that the content is undefined. This enables both warnings and optimizations. The previous patch makes update_ssa handle replacing clobbers with default definitions when a variable doesn't have its address taken anymore. When SRA scalarizes, it creates a new variable and relies on update_ssa to finish the job. So I am inserting a clobber on the new variable so that update_ssa knows to use a default definition. > The new function needs a comment. Indeed, I'll add one once the conversation on the first patch converges. Thanks,
On 07/07/14 02:56, Richard Biener wrote: > 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? Marc is trying to exploit the "undefinedness" of the object that exists after the CLOBBER. When we scalaraize, we are losing the undefinedness quality of the object. It's very similar to what's already happening in tree-into-ssa, he's just handling it in another place. Jeff
On July 7, 2014 11:32:10 AM CEST, Marc Glisse <marc.glisse@inria.fr> wrote: >On Mon, 7 Jul 2014, Richard Biener wrote: > >> 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? > >The main idea of these patches is that when we read from a place that >was >clobbered, instead of dropping the clobber and reading what was there >before, we can use a variable with a default definition to mark that >the >content is undefined. This enables both warnings and optimizations. > >The previous patch makes update_ssa handle replacing clobbers with >default >definitions when a variable doesn't have its address taken anymore. >When >SRA scalarizes, it creates a new variable and relies on update_ssa to >finish the job. So I am inserting a clobber on the new variable so that > >update_ssa knows to use a default definition. OK. But can't SRA simply replace the reads with undef SSA names when the use is dominated by a clobber? (I've long wanted to teach it some flow sensitivity by doing the replacements within a domwalk) Richard. >> The new function needs a comment. > >Indeed, I'll add one once the conversation on the first patch >converges. > >Thanks,
On Mon, 7 Jul 2014, Richard Biener wrote: >> The main idea of these patches is that when we read from a place that >> was clobbered, instead of dropping the clobber and reading what was >> there before, we can use a variable with a default definition to mark >> that the content is undefined. This enables both warnings and >> optimizations. >> >> The previous patch makes update_ssa handle replacing clobbers with >> default definitions when a variable doesn't have its address taken >> anymore. When SRA scalarizes, it creates a new variable and relies on >> update_ssa to finish the job. So I am inserting a clobber on the new >> variable so that update_ssa knows to use a default definition. > > OK. But can't SRA simply replace the reads with undef SSA names when > the use is dominated by a clobber? I guess it could, but it seems safer and much simpler to avoid duplicating the logic. SRA creates a new variable and replaces uses of the old one by uses of the new one. update_address_taken then handles replacing the new variable by a bunch of SSA_NAMEs, tracking which use corresponds to which def, so it seems natural to let it handle the clobber as well. Also, conceptually, saying that when the full variable is clobbered the subvariable is clobbered as well is a rather obvious operation. Otherwise, I could create an extra variable tmp, with the same type as the new variable, and replace the clobber by an assignment of the default definition of tmp to the new variable. update_address_taken will then see a regular assignment and handle it as usual (create a new SSA_NAME for it). So instead of creating a clobber that is immediatly replaced with a NOP, this creates an extra SSA_NAME that only disappears in the next propagation pass. Handling the reads myself directly in SRA and replacing them with undef SSA_NAMEs doesn't seem to fit well with how SRA works. It would certainly be possible, but would require quite a bit of new code. Also, the domination relation between the clobber and the reads is not always obvious and may require some PHIs in between.
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. > > Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-linux-gnu. > > > 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); > } > 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 "" } */ > +} >
On Thu, Jul 10, 2014 at 04:54:53PM +0200, Richard Biener wrote: > > + else if (access->grp_to_be_debug_replaced) > > + { > > Why would we care to create clobbers for debug stmts?! Are those > even valid? It is not valid. Though, the fields supposedly live nowhere after the clobber, so perhaps one could use GIMPLE_DEBUG_BIND_NOVALUE instead of the clobber in the gimple_build_debug_bind call, and drop the previous two lines. Not sure if it is desirable though, it might cause the debug info to say something is unavailable even if it still lives in some register or memory for a few extra instructions. Alex, what do you think? > > + 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); > > +} > > + Jakub
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 "" } */ +}
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.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.