| Message ID | alpine.DEB.2.02.1411031329120.20095@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
Hi, On Mon, Nov 03, 2014 at 01:59:24PM +0100, Marc Glisse wrote: > Hello, > > now that the update_address_taken patch is in, let me re-post the > SRA follow-up. With this patch, testcase pr60517.C (attached) has a > use of an undefined variable at the time of the uninit pass. Sadly, > while this warned with earlier versions of the other patch (when I > was inserting default definitions of new variables), it does not > anymore because we have TREE_NO_WARNING all over the place. Still, I > believe this is a step in the right direction, and it passed > bootstrap+testsuite (minus the new testcase). I am not sure what you mean by "minus the new testcase" but, with the gimple checking compiler at least, the testcase produces an ICE: pr60517.C: In function ‘double foo(A)’: pr60517.C:15:1: error: non-decl/MEM_REF LHS in clobber statement } ^ SR.1_8 SR.1_8 ={v} {CLOBBER}; pr60517.C:15:1: internal compiler error: verify_gimple failed 0xc7810d verify_gimple_in_cfg(function*, bool) /home/mjambor/gcc/mine/src/gcc/tree-cfg.c:5039 0xb8256f execute_function_todo /home/mjambor/gcc/mine/src/gcc/passes.c:1758 0xb83183 execute_todo /home/mjambor/gcc/mine/src/gcc/passes.c:1815 The problem is exactly what the verifier complains about, a clobber of an SSA_NAME which I agree with the verifier is a really bad idea. If you want SRA to produce information that at a given point a scalar replacement (which will become an SSA_NAME) does not have any valid data, the way to represent that is to load it with a value from a default-definition SSA name. Something along the lines of get_repl_default_def_ssa_name which unfortunately you cannot directly use without confusing the SSA renamer. The easiest way, at least to test a proof of concept, is probably to create another declaration of the same type (reusing bits from create_access_replacement, especially setting the same DECL_DEBUG_EXPR), get its default-definition using get_or_create_ssa_default_def and then assign that to the replacement instead of trying to clobber it. (A more correct approach would be to design a way of telling the renamer that at this point the scalar becomes undefined and that it should start using default-defs until the next definition appears but that is likely to be difficult, although that is just my guess, it might be easy.) > Would it be ok to commit the tree-sra.c change? I don't think so, sorry about the bad news. Martin
On Mon, 3 Nov 2014, Martin Jambor wrote: > On Mon, Nov 03, 2014 at 01:59:24PM +0100, Marc Glisse wrote: >> Hello, >> >> now that the update_address_taken patch is in, let me re-post the >> SRA follow-up. With this patch, testcase pr60517.C (attached) has a >> use of an undefined variable at the time of the uninit pass. Sadly, >> while this warned with earlier versions of the other patch (when I >> was inserting default definitions of new variables), it does not >> anymore because we have TREE_NO_WARNING all over the place. Still, I >> believe this is a step in the right direction, and it passed >> bootstrap+testsuite (minus the new testcase). > > I am not sure what you mean by "minus the new testcase" but, with the > gimple checking compiler at least, the testcase produces an ICE: > > pr60517.C: In function ‘double foo(A)’: > pr60517.C:15:1: error: non-decl/MEM_REF LHS in clobber statement > } > ^ > SR.1_8 > SR.1_8 ={v} {CLOBBER}; > pr60517.C:15:1: internal compiler error: verify_gimple failed > 0xc7810d verify_gimple_in_cfg(function*, bool) > /home/mjambor/gcc/mine/src/gcc/tree-cfg.c:5039 > 0xb8256f execute_function_todo > /home/mjambor/gcc/mine/src/gcc/passes.c:1758 > 0xb83183 execute_todo > /home/mjambor/gcc/mine/src/gcc/passes.c:1815 > > The problem is exactly what the verifier complains about, a clobber of > an SSA_NAME which I agree with the verifier is a really bad idea. > > If you want SRA to produce information that at a given point a scalar > replacement (which will become an SSA_NAME) does not have any valid > data, the way to represent that is to load it with a value from a > default-definition SSA name. Something along the lines of > get_repl_default_def_ssa_name which unfortunately you cannot directly > use without confusing the SSA renamer. > > The easiest way, at least to test a proof of concept, is probably to > create another declaration of the same type (reusing bits from > create_access_replacement, especially setting the same > DECL_DEBUG_EXPR), get its default-definition using > get_or_create_ssa_default_def and then assign that to the replacement > instead of trying to clobber it. > > (A more correct approach would be to design a way of telling the > renamer that at this point the scalar becomes undefined and that it > should start using default-defs until the next definition appears but > that is likely to be difficult, although that is just my guess, it > might be easy.) > >> Would it be ok to commit the tree-sra.c change? > > I don't think so, sorry about the bad news. Thanks for your help. I am quite confused. I am not seeing the failure you mention, but since I am building from trunk with no specific option I should be getting gimple checking. The way I expect things to work: 1) we have a clobber for a variable V 2) SRA creates a variable SR and replaces uses of V 3) (new) next to V's clobber, we insert a clobber for SR 4) update_address_taken notices that SR does not have its address taken, and thus replaces it by SSA_NAMEs, and after the clobber by an undefined variable (yesterday it would just have removed the clobber). What you describe in a parenthesis about the renamer is pretty much what I think rewrite_update_stmt does, with the clobber as the hint. Do you have further comments, based on these explanations, that could help me pinpoint what is going wrong?
Hi, On Mon, Nov 03, 2014 at 05:17:22PM +0100, Marc Glisse wrote: > On Mon, 3 Nov 2014, Martin Jambor wrote: > >On Mon, Nov 03, 2014 at 01:59:24PM +0100, Marc Glisse wrote: > >> > >>now that the update_address_taken patch is in, let me re-post the > >>SRA follow-up. With this patch, testcase pr60517.C (attached) has a > >>use of an undefined variable at the time of the uninit pass. Sadly, > >>while this warned with earlier versions of the other patch (when I > >>was inserting default definitions of new variables), it does not > >>anymore because we have TREE_NO_WARNING all over the place. Still, I > >>believe this is a step in the right direction, and it passed > >>bootstrap+testsuite (minus the new testcase). > > > >I am not sure what you mean by "minus the new testcase" but, with the > >gimple checking compiler at least, the testcase produces an ICE: > > > > pr60517.C: In function ‘double foo(A)’: > > pr60517.C:15:1: error: non-decl/MEM_REF LHS in clobber statement > > } > > ^ > > SR.1_8 > > SR.1_8 ={v} {CLOBBER}; > > pr60517.C:15:1: internal compiler error: verify_gimple failed > > 0xc7810d verify_gimple_in_cfg(function*, bool) > > /home/mjambor/gcc/mine/src/gcc/tree-cfg.c:5039 > > 0xb8256f execute_function_todo > > /home/mjambor/gcc/mine/src/gcc/passes.c:1758 > > 0xb83183 execute_todo > > /home/mjambor/gcc/mine/src/gcc/passes.c:1815 > > > >The problem is exactly what the verifier complains about, a clobber of > >an SSA_NAME which I agree with the verifier is a really bad idea. > > > >If you want SRA to produce information that at a given point a scalar > >replacement (which will become an SSA_NAME) does not have any valid > >data, the way to represent that is to load it with a value from a > >default-definition SSA name. Something along the lines of > >get_repl_default_def_ssa_name which unfortunately you cannot directly > >use without confusing the SSA renamer. > > > >The easiest way, at least to test a proof of concept, is probably to > >create another declaration of the same type (reusing bits from > >create_access_replacement, especially setting the same > >DECL_DEBUG_EXPR), get its default-definition using > >get_or_create_ssa_default_def and then assign that to the replacement > >instead of trying to clobber it. > > > >(A more correct approach would be to design a way of telling the > >renamer that at this point the scalar becomes undefined and that it > >should start using default-defs until the next definition appears but > >that is likely to be difficult, although that is just my guess, it > >might be easy.) > > > >>Would it be ok to commit the tree-sra.c change? > > > >I don't think so, sorry about the bad news. > > Thanks for your help. I am quite confused. I am not seeing the > failure you mention, but since I am building from trunk with no > specific option I should be getting gimple checking. > > The way I expect things to work: > 1) we have a clobber for a variable V > 2) SRA creates a variable SR and replaces uses of V > 3) (new) next to V's clobber, we insert a clobber for SR > 4) update_address_taken notices that SR does not have its address > taken, and thus replaces it by SSA_NAMEs, and after the clobber by > an undefined variable (yesterday it would just have removed the > clobber). update_address_taken? The conversion to SSA form is done by means of returning TODO_update_ssa from the (early) SRA pass (which is what I mean by the renamer). > > What you describe in a parenthesis about the renamer is pretty much > what I think rewrite_update_stmt does, with the clobber as the hint. > > Do you have further comments, based on these explanations, that > could help me pinpoint what is going wrong? I just applied your patch on top of trunk revision 217032 on my x86_64-linux (openSUSE 13.1) desktop, configured with /home/mjambor/gcc/mine/src/configure --prefix=/home/mjambor/gcc/mine/inst --enable-languages=c,c++ --enable-checking=yes --disable-bootstrap --with-plugin-ld=/home/mjambor/binutils/obj/gold/ld-new --disable-libsanitizer --disable-multilib built with simple make -j8, and ran the testcase with make -k check RUNTESTFLAGS="dg.exp=pr60517.C" And the error popped out in gcc/testsuite/g++/g++.log. I really can't think of reason why you don't see it. But I do and little wonder, really, given the second condition in verify_gimple_assign_single, which is there for more than a year. Looking at the dumped statement it seems that the LHS is already an SSA_NAME. Moreover, when I dumped the erroneous body in gdb, I have not seen any default definitions anywhere. So if your intention was to teach the renamer to turn this into default definitions, it does not work and update_ssa did not get the hint. Martin
On Mon, 3 Nov 2014, Martin Jambor wrote:
> I just applied your patch on top of trunk revision 217032 on my
Ah, that explains it, thanks. This patch is a follow-up to r217034. Still,
I didn't expect the ICE you are seeing by applying this patch to older
trunk, I'll try to reproduce that.
On Mon, 3 Nov 2014, Marc Glisse wrote: > On Mon, 3 Nov 2014, Martin Jambor wrote: > >> I just applied your patch on top of trunk revision 217032 on my > > Ah, that explains it, thanks. This patch is a follow-up to r217034. Still, I > didn't expect the ICE you are seeing by applying this patch to older trunk, > I'll try to reproduce that. It is TODO_update_address_taken that used to remove clobbers, and as you said ESRA goes straight to TODO_update_ssa, which explains why the clobbers caused trouble. In any case, after r217034, update_ssa should handle clobbers much better. Could you take an other look based on a more recent trunk, please?
Hi, On Mon, Nov 03, 2014 at 10:46:49PM +0100, Marc Glisse wrote: > On Mon, 3 Nov 2014, Marc Glisse wrote: > > >On Mon, 3 Nov 2014, Martin Jambor wrote: > > > >>I just applied your patch on top of trunk revision 217032 on my > > > >Ah, that explains it, thanks. This patch is a follow-up to > >r217034. Still, I didn't expect the ICE you are seeing by applying > >this patch to older trunk, I'll try to reproduce that. > > It is TODO_update_address_taken that used to remove clobbers, and as > you said ESRA goes straight to TODO_update_ssa, which explains why > the clobbers caused trouble. In any case, after r217034, update_ssa > should handle clobbers much better. Could you take an other look > based on a more recent trunk, please? > Sorry for the delay. Anyway, on the current trunk (i.e. Tuesday checkout) the patch works as expected, there are assignments from default definitions now and even though we do not warn as we should, the patch improves the generated code. The function foo from the testcase is optimized to "return SR.1_2(D);" as soon as release_ssa now, whereas unpatched trunk leaves an undefined load even in the optimized dump. Thus, I like the patch and given that you posted it well before stage1 end, I'd like to see it committed. Richi, can you have a look and perhaps approve it? Thanks, Martin
On Thu, Nov 20, 2014 at 7:11 PM, Martin Jambor <mjambor@suse.cz> wrote: > Hi, > > On Mon, Nov 03, 2014 at 10:46:49PM +0100, Marc Glisse wrote: >> On Mon, 3 Nov 2014, Marc Glisse wrote: >> >> >On Mon, 3 Nov 2014, Martin Jambor wrote: >> > >> >>I just applied your patch on top of trunk revision 217032 on my >> > >> >Ah, that explains it, thanks. This patch is a follow-up to >> >r217034. Still, I didn't expect the ICE you are seeing by applying >> >this patch to older trunk, I'll try to reproduce that. >> >> It is TODO_update_address_taken that used to remove clobbers, and as >> you said ESRA goes straight to TODO_update_ssa, which explains why >> the clobbers caused trouble. In any case, after r217034, update_ssa >> should handle clobbers much better. Could you take an other look >> based on a more recent trunk, please? >> > > Sorry for the delay. Anyway, on the current trunk (i.e. Tuesday > checkout) the patch works as expected, there are assignments from > default definitions now and even though we do not warn as we should, > the patch improves the generated code. The function foo from the > testcase is optimized to "return SR.1_2(D);" as soon as release_ssa > now, whereas unpatched trunk leaves an undefined load even in the > optimized dump. > > Thus, I like the patch and given that you posted it well before stage1 > end, I'd like to see it committed. Richi, can you have a look and > perhaps approve it? Yes, the patch is ok. Thanks, Richard. > Thanks, > > Martin >
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: gcc/tree-sra.c =================================================================== --- gcc/tree-sra.c (revision 217035) +++ gcc/tree-sra.c (working copy) @@ -2716,20 +2716,51 @@ 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); } +/* Clobber all scalar replacements in an access subtree. ACCESS is the the + root of the subtree to be processed. GSI is the statement iterator used + for inserting statements which are added after the current statement if + INSERT_AFTER is true or before it otherwise. */ + +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); + } + + 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 @@ -3028,43 +3059,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; }