| Message ID | 20170220205126.GI1849@tucnak |
|---|---|
| State | New |
| Headers | show |
On Mon, 20 Feb 2017, Jakub Jelinek wrote: > As mentioned by Jason in the PR, we've regressed on the following testcase > since we started emitting CLOBBERs at the start of ctors (and we warn as > before with -fno-lifetime-dse -Wuninitialized). > With -fno-lifetime-dse, the vuse on the b.x read stmt is default def > and thus we warn, but if there are clobbers before that, that is not the > case and we don't warn. The patch is quick hack to bypass the initial > clobbers as long as there aren't really many. If we wanted to handle > all initial clobbers, I bet the first time we run into this we could > recursively walk vop uses from the default def and build say a bitmap > of vop SSA_NAMEs which are vdefs of clobbers that only have clobbers > before it as vdef stmts. MEM[(struct &)&b] ={v} {CLOBBER}; _4 = b.x; if (_4 != 0) It would be nice (at some point in a distant future) to turn that into MEM[(struct &)&b] ={v} {CLOBBER}; if (_4(D) != 0) i.e. replace reads from clobbered memory with a default def (with a gimple_nop defining statement). IIRC I tried to do it in FRE once, but failed. > Now, the comment says: > /* For memory the only cheap thing we can do is see if we > have a use of the default def of the virtual operand. > ??? Not so cheap would be to use the alias oracle via > walk_aliased_vdefs, if we don't find any aliasing vdef > warn as is-used-uninitialized, if we don't find an aliasing > vdef that kills our use (stmt_kills_ref_p), warn as > may-be-used-uninitialized. But this walk is quadratic and > so must be limited which means we would miss warning > opportunities. */ > I wonder if it isn't useful to walk even limited number of vdefs this way Seems worth a try for gcc8, there are several PRs it could help with. > anyway (likely GCC8 material though), e.g. if we run into a clobber that > must (rather than just may) portion of the read ref (and of course when > seeing non-clobber stmt that could alias with the ref give up before that), > we could warn even if we are very far from the start of the function.
On Mon, 20 Feb 2017, Jakub Jelinek wrote: > Hi! > > As mentioned by Jason in the PR, we've regressed on the following testcase > since we started emitting CLOBBERs at the start of ctors (and we warn as > before with -fno-lifetime-dse -Wuninitialized). > With -fno-lifetime-dse, the vuse on the b.x read stmt is default def > and thus we warn, but if there are clobbers before that, that is not the > case and we don't warn. The patch is quick hack to bypass the initial > clobbers as long as there aren't really many. If we wanted to handle > all initial clobbers, I bet the first time we run into this we could > recursively walk vop uses from the default def and build say a bitmap > of vop SSA_NAMEs which are vdefs of clobbers that only have clobbers > before it as vdef stmts. > > Now, the comment says: > /* For memory the only cheap thing we can do is see if we > have a use of the default def of the virtual operand. > ??? Not so cheap would be to use the alias oracle via > walk_aliased_vdefs, if we don't find any aliasing vdef > warn as is-used-uninitialized, if we don't find an aliasing > vdef that kills our use (stmt_kills_ref_p), warn as > may-be-used-uninitialized. But this walk is quadratic and > so must be limited which means we would miss warning > opportunities. */ > I wonder if it isn't useful to walk even limited number of vdefs this way > anyway (likely GCC8 material though), e.g. if we run into a clobber that > must (rather than just may) portion of the read ref (and of course when > seeing non-clobber stmt that could alias with the ref give up before that), > we could warn even if we are very far from the start of the function. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk (or do > you want a version with a bitmap and really ignoring all the clobbers, > rather than just 128 of them)? I'd rather not special-case clobbers but take this as an opportunity to implement that ??? comment with walk_aliased_vdefs (you can do some copy&paste programming from the use in tree-ssa-dce.c I think). Just use a very low walking limit. As you say, a kill from a clobber should warn (you skip them). Richard. > 2017-02-20 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/79345 > * tree-ssa-uninit.c (warn_uninitialized_vars): If vuse is not > default def, but there are only CLOBBER stmts as vdefs, handle it like > default def. > > * g++.dg/warn/pr79345.C: New test. > > --- gcc/tree-ssa-uninit.c.jj 2017-01-01 12:45:35.000000000 +0100 > +++ gcc/tree-ssa-uninit.c 2017-02-20 16:43:08.244868075 +0100 > @@ -248,8 +248,7 @@ warn_uninitialized_vars (bool warn_possi > use = gimple_vuse (stmt); > if (use > && gimple_assign_single_p (stmt) > - && !gimple_vdef (stmt) > - && SSA_NAME_IS_DEFAULT_DEF (use)) > + && !gimple_vdef (stmt)) > { > tree rhs = gimple_assign_rhs1 (stmt); > tree base = get_base_address (rhs); > @@ -260,6 +259,23 @@ warn_uninitialized_vars (bool warn_possi > || is_global_var (base)) > continue; > > + /* Look through some CLOBBER stmts. */ > + for (unsigned int cnt = 0; cnt < 128 && use; cnt++) > + { > + if (SSA_NAME_IS_DEFAULT_DEF (use)) > + break; > + > + gimple *def_stmt = SSA_NAME_DEF_STMT (use); > + if (!gimple_clobber_p (def_stmt)) > + { > + use = NULL_TREE; > + break; > + } > + use = gimple_vuse (def_stmt); > + } > + if (use == NULL_TREE) > + continue; > + > if (always_executed) > warn_uninit (OPT_Wuninitialized, use, gimple_assign_rhs1 (stmt), > base, "%qE is used uninitialized in this function", > --- gcc/testsuite/g++.dg/warn/pr79345.C.jj 2017-02-20 17:19:01.138952915 +0100 > +++ gcc/testsuite/g++.dg/warn/pr79345.C 2017-02-20 17:18:20.000000000 +0100 > @@ -0,0 +1,22 @@ > +// PR tree-optimization/79345 > +// { dg-do compile { target c++11 } } > +// { dg-options "-O2 -Wuninitialized" } > + > +struct A { > + A (int); > +}; > + > +struct B : A { > + const bool x = true; > + > + B () : A (x ? 3 : 7) { } // { dg-warning "is used uninitialized in this function" } > +}; > + > +void foo (void*); > + > +void > +bar () > +{ > + B b; > + foo (&b); > +} > > Jakub > >
On Mon, 20 Feb 2017, Marc Glisse wrote: > On Mon, 20 Feb 2017, Jakub Jelinek wrote: > > > As mentioned by Jason in the PR, we've regressed on the following testcase > > since we started emitting CLOBBERs at the start of ctors (and we warn as > > before with -fno-lifetime-dse -Wuninitialized). > > With -fno-lifetime-dse, the vuse on the b.x read stmt is default def > > and thus we warn, but if there are clobbers before that, that is not the > > case and we don't warn. The patch is quick hack to bypass the initial > > clobbers as long as there aren't really many. If we wanted to handle > > all initial clobbers, I bet the first time we run into this we could > > recursively walk vop uses from the default def and build say a bitmap > > of vop SSA_NAMEs which are vdefs of clobbers that only have clobbers > > before it as vdef stmts. > > MEM[(struct &)&b] ={v} {CLOBBER}; > _4 = b.x; > if (_4 != 0) > > It would be nice (at some point in a distant future) to turn that into > > MEM[(struct &)&b] ={v} {CLOBBER}; > if (_4(D) != 0) > > i.e. replace reads from clobbered memory with a default def (with a gimple_nop > defining statement). > > IIRC I tried to do it in FRE once, but failed. Yeah, FRE isn't too happy about VN_TOP in the lattice (which is what you'd use here). I also have/had patches trying to make it (more) happy about VN_TOP but it still broke stuff like for example tail-merging... Need to dissect PRE from tail-merging again, and also possibly PRE from value-numbering (run eliminate() before doing PRE). Richard. > > Now, the comment says: > > /* For memory the only cheap thing we can do is see if we > > have a use of the default def of the virtual operand. > > ??? Not so cheap would be to use the alias oracle via > > walk_aliased_vdefs, if we don't find any aliasing vdef > > warn as is-used-uninitialized, if we don't find an aliasing > > vdef that kills our use (stmt_kills_ref_p), warn as > > may-be-used-uninitialized. But this walk is quadratic and > > so must be limited which means we would miss warning > > opportunities. */ > > I wonder if it isn't useful to walk even limited number of vdefs this way > > Seems worth a try for gcc8, there are several PRs it could help with. > > > anyway (likely GCC8 material though), e.g. if we run into a clobber that > > must (rather than just may) portion of the read ref (and of course when > > seeing non-clobber stmt that could alias with the ref give up before that), > > we could warn even if we are very far from the start of the function. > >
On 02/21/2017 02:01 AM, Richard Biener wrote: > On Mon, 20 Feb 2017, Marc Glisse wrote: > >> On Mon, 20 Feb 2017, Jakub Jelinek wrote: >> >>> As mentioned by Jason in the PR, we've regressed on the following testcase >>> since we started emitting CLOBBERs at the start of ctors (and we warn as >>> before with -fno-lifetime-dse -Wuninitialized). >>> With -fno-lifetime-dse, the vuse on the b.x read stmt is default def >>> and thus we warn, but if there are clobbers before that, that is not the >>> case and we don't warn. The patch is quick hack to bypass the initial >>> clobbers as long as there aren't really many. If we wanted to handle >>> all initial clobbers, I bet the first time we run into this we could >>> recursively walk vop uses from the default def and build say a bitmap >>> of vop SSA_NAMEs which are vdefs of clobbers that only have clobbers >>> before it as vdef stmts. >> >> MEM[(struct &)&b] ={v} {CLOBBER}; >> _4 = b.x; >> if (_4 != 0) >> >> It would be nice (at some point in a distant future) to turn that into >> >> MEM[(struct &)&b] ={v} {CLOBBER}; >> if (_4(D) != 0) >> >> i.e. replace reads from clobbered memory with a default def (with a gimple_nop >> defining statement). >> >> IIRC I tried to do it in FRE once, but failed. > > Yeah, FRE isn't too happy about VN_TOP in the lattice (which is what > you'd use here). I also have/had patches trying to make it (more) happy > about VN_TOP but it still broke stuff like for example tail-merging... > > Need to dissect PRE from tail-merging again, and also possibly PRE > from value-numbering (run eliminate() before doing PRE). Couldn't this be easily modeled as redundant load elimination? Pretend the clobber is a read, which would make the read of b.x redundant. I haven't prototyped it, but that'd be the first approach I'd try. jeff
On Tue, 21 Feb 2017, Jeff Law wrote: > On 02/21/2017 02:01 AM, Richard Biener wrote: > > On Mon, 20 Feb 2017, Marc Glisse wrote: > > > > > On Mon, 20 Feb 2017, Jakub Jelinek wrote: > > > > > > > As mentioned by Jason in the PR, we've regressed on the following > > > > testcase > > > > since we started emitting CLOBBERs at the start of ctors (and we warn as > > > > before with -fno-lifetime-dse -Wuninitialized). > > > > With -fno-lifetime-dse, the vuse on the b.x read stmt is default def > > > > and thus we warn, but if there are clobbers before that, that is not the > > > > case and we don't warn. The patch is quick hack to bypass the initial > > > > clobbers as long as there aren't really many. If we wanted to handle > > > > all initial clobbers, I bet the first time we run into this we could > > > > recursively walk vop uses from the default def and build say a bitmap > > > > of vop SSA_NAMEs which are vdefs of clobbers that only have clobbers > > > > before it as vdef stmts. > > > > > > MEM[(struct &)&b] ={v} {CLOBBER}; > > > _4 = b.x; > > > if (_4 != 0) > > > > > > It would be nice (at some point in a distant future) to turn that into > > > > > > MEM[(struct &)&b] ={v} {CLOBBER}; > > > if (_4(D) != 0) > > > > > > i.e. replace reads from clobbered memory with a default def (with a > > > gimple_nop > > > defining statement). > > > > > > IIRC I tried to do it in FRE once, but failed. > > > > Yeah, FRE isn't too happy about VN_TOP in the lattice (which is what > > you'd use here). I also have/had patches trying to make it (more) happy > > about VN_TOP but it still broke stuff like for example tail-merging... > > > > Need to dissect PRE from tail-merging again, and also possibly PRE > > from value-numbering (run eliminate() before doing PRE). > Couldn't this be easily modeled as redundant load elimination? Pretend the > clobber is a read, which would make the read of b.x redundant. > > I haven't prototyped it, but that'd be the first approach I'd try. Sure, but that only works for must-be-uninit reads. The question is also what to replace the redundant reads with, esp. if you consider the read could be of aggregate type. Obvious choices would be default-defs for regiser type reads and clobbers for aggregate typed ones (but then clobbers are not valid in call argument context for example). Another choice would be {} (or zero for register types), same issue for call arg context. For maybe-uninit reads you could model it as load hoisting -- you'd hoist towards may-defs and must-uninit points and insert appropriate PHIs -- again difficult for the aggregate case. But yes, currently FRE/PRE treat uninitialized as varying, that's something we can improve on. Richard.
--- gcc/tree-ssa-uninit.c.jj 2017-01-01 12:45:35.000000000 +0100 +++ gcc/tree-ssa-uninit.c 2017-02-20 16:43:08.244868075 +0100 @@ -248,8 +248,7 @@ warn_uninitialized_vars (bool warn_possi use = gimple_vuse (stmt); if (use && gimple_assign_single_p (stmt) - && !gimple_vdef (stmt) - && SSA_NAME_IS_DEFAULT_DEF (use)) + && !gimple_vdef (stmt)) { tree rhs = gimple_assign_rhs1 (stmt); tree base = get_base_address (rhs); @@ -260,6 +259,23 @@ warn_uninitialized_vars (bool warn_possi || is_global_var (base)) continue; + /* Look through some CLOBBER stmts. */ + for (unsigned int cnt = 0; cnt < 128 && use; cnt++) + { + if (SSA_NAME_IS_DEFAULT_DEF (use)) + break; + + gimple *def_stmt = SSA_NAME_DEF_STMT (use); + if (!gimple_clobber_p (def_stmt)) + { + use = NULL_TREE; + break; + } + use = gimple_vuse (def_stmt); + } + if (use == NULL_TREE) + continue; + if (always_executed) warn_uninit (OPT_Wuninitialized, use, gimple_assign_rhs1 (stmt), base, "%qE is used uninitialized in this function", --- gcc/testsuite/g++.dg/warn/pr79345.C.jj 2017-02-20 17:19:01.138952915 +0100 +++ gcc/testsuite/g++.dg/warn/pr79345.C 2017-02-20 17:18:20.000000000 +0100 @@ -0,0 +1,22 @@ +// PR tree-optimization/79345 +// { dg-do compile { target c++11 } } +// { dg-options "-O2 -Wuninitialized" } + +struct A { + A (int); +}; + +struct B : A { + const bool x = true; + + B () : A (x ? 3 : 7) { } // { dg-warning "is used uninitialized in this function" } +}; + +void foo (void*); + +void +bar () +{ + B b; + foo (&b); +}
Hi! As mentioned by Jason in the PR, we've regressed on the following testcase since we started emitting CLOBBERs at the start of ctors (and we warn as before with -fno-lifetime-dse -Wuninitialized). With -fno-lifetime-dse, the vuse on the b.x read stmt is default def and thus we warn, but if there are clobbers before that, that is not the case and we don't warn. The patch is quick hack to bypass the initial clobbers as long as there aren't really many. If we wanted to handle all initial clobbers, I bet the first time we run into this we could recursively walk vop uses from the default def and build say a bitmap of vop SSA_NAMEs which are vdefs of clobbers that only have clobbers before it as vdef stmts. Now, the comment says: /* For memory the only cheap thing we can do is see if we have a use of the default def of the virtual operand. ??? Not so cheap would be to use the alias oracle via walk_aliased_vdefs, if we don't find any aliasing vdef warn as is-used-uninitialized, if we don't find an aliasing vdef that kills our use (stmt_kills_ref_p), warn as may-be-used-uninitialized. But this walk is quadratic and so must be limited which means we would miss warning opportunities. */ I wonder if it isn't useful to walk even limited number of vdefs this way anyway (likely GCC8 material though), e.g. if we run into a clobber that must (rather than just may) portion of the read ref (and of course when seeing non-clobber stmt that could alias with the ref give up before that), we could warn even if we are very far from the start of the function. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk (or do you want a version with a bitmap and really ignoring all the clobbers, rather than just 128 of them)? 2017-02-20 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/79345 * tree-ssa-uninit.c (warn_uninitialized_vars): If vuse is not default def, but there are only CLOBBER stmts as vdefs, handle it like default def. * g++.dg/warn/pr79345.C: New test. Jakub