| Message ID | 20141211170655.GA28396@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
On Thu, Dec 11, 2014 at 06:06:55PM +0100, Jan Hubicka wrote: > Hi, > in firefox .optimized dumps one can see few places where __builtin_unreachable > is called (as a result of devirtualization code proving the code path to be > undefined). There is usually some argument setup for the parameters of > __builtin_unreachable that are dead. This patch makes it somewhat better > so now we get: > <bb 30>: > # prephitmp_222 = PHI <_52(27), pretmp_245(29)> > _57 = prephitmp_222 + 2; > pool_40(D)->ptr = _57; > __builtin_unreachable (); > > Why DSE does not eliminate the stores prior noreturn const function? > > Bootstrapped/regtested x86_64-linux, OK? > > Honza > * tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Remove dead parameters > of BUILT_IN_UNREACHABLE Shouldn't this be done when you actually change the call to __builtin_unreachable ()? I mean, __builtin_unreachable () has no arguments, so leaving any arguments there is broken IL, even if you clean it up during the next DCE. > --- tree-ssa-dce.c (revision 218610) > +++ tree-ssa-dce.c (working copy) > @@ -250,6 +250,15 @@ mark_stmt_if_obviously_necessary (gimple > case BUILT_IN_ALLOCA: > case BUILT_IN_ALLOCA_WITH_ALIGN: > return; > + case BUILT_IN_UNREACHABLE: > + /* All parameters of BUILT_IN_UNREACHABLE are dead. Remove them > + from the stmt, so we can remove their definitions. */ > + if (gimple_call_num_args (stmt)) > + { > + gimple_set_num_ops (stmt, 3); > + update_stmt (stmt); > + } > + break; > > default:; > } Jakub
> On Thu, Dec 11, 2014 at 06:06:55PM +0100, Jan Hubicka wrote: > > Hi, > > in firefox .optimized dumps one can see few places where __builtin_unreachable > > is called (as a result of devirtualization code proving the code path to be > > undefined). There is usually some argument setup for the parameters of > > __builtin_unreachable that are dead. This patch makes it somewhat better > > so now we get: > > <bb 30>: > > # prephitmp_222 = PHI <_52(27), pretmp_245(29)> > > _57 = prephitmp_222 + 2; > > pool_40(D)->ptr = _57; > > __builtin_unreachable (); > > > > Why DSE does not eliminate the stores prior noreturn const function? > > > > Bootstrapped/regtested x86_64-linux, OK? > > > > Honza > > * tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Remove dead parameters > > of BUILT_IN_UNREACHABLE > > Shouldn't this be done when you actually change the call to > __builtin_unreachable ()? I mean, __builtin_unreachable () has no > arguments, so leaving any arguments there is broken IL, even if you clean it > up during the next DCE. Hmm, I tought there was some reason to not do so becuase of inplace folding and memory-SSA. I can give a try to update all the places we can put builtin_unreachable into IL. (I wonder if that also include standard constant propagation) Honza > > > --- tree-ssa-dce.c (revision 218610) > > +++ tree-ssa-dce.c (working copy) > > @@ -250,6 +250,15 @@ mark_stmt_if_obviously_necessary (gimple > > case BUILT_IN_ALLOCA: > > case BUILT_IN_ALLOCA_WITH_ALIGN: > > return; > > + case BUILT_IN_UNREACHABLE: > > + /* All parameters of BUILT_IN_UNREACHABLE are dead. Remove them > > + from the stmt, so we can remove their definitions. */ > > + if (gimple_call_num_args (stmt)) > > + { > > + gimple_set_num_ops (stmt, 3); > > + update_stmt (stmt); > > + } > > + break; > > > > default:; > > } > > Jakub
Hi, On Thu, Dec 11, 2014 at 07:16:43PM +0100, Jan Hubicka wrote: > > On Thu, Dec 11, 2014 at 06:06:55PM +0100, Jan Hubicka wrote: > > > Hi, > > > in firefox .optimized dumps one can see few places where __builtin_unreachable > > > is called (as a result of devirtualization code proving the code path to be > > > undefined). There is usually some argument setup for the parameters of > > > __builtin_unreachable that are dead. This patch makes it somewhat better > > > so now we get: > > > <bb 30>: > > > # prephitmp_222 = PHI <_52(27), pretmp_245(29)> > > > _57 = prephitmp_222 + 2; > > > pool_40(D)->ptr = _57; > > > __builtin_unreachable (); > > > > > > Why DSE does not eliminate the stores prior noreturn const function? > > > > > > Bootstrapped/regtested x86_64-linux, OK? > > > > > > Honza > > > * tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Remove dead parameters > > > of BUILT_IN_UNREACHABLE > > > > Shouldn't this be done when you actually change the call to > > __builtin_unreachable ()? I mean, __builtin_unreachable () has no > > arguments, so leaving any arguments there is broken IL, even if you clean it > > up during the next DCE. > > Hmm, I tought there was some reason to not do so becuase of inplace folding and memory-SSA. > I can give a try to update all the places we can put builtin_unreachable into IL. > (I wonder if that also include standard constant propagation) I think that's what we ought to do, see also PR 61591. Martin
On Thu, Dec 11, 2014 at 7:16 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> On Thu, Dec 11, 2014 at 06:06:55PM +0100, Jan Hubicka wrote: >> > Hi, >> > in firefox .optimized dumps one can see few places where __builtin_unreachable >> > is called (as a result of devirtualization code proving the code path to be >> > undefined). There is usually some argument setup for the parameters of >> > __builtin_unreachable that are dead. This patch makes it somewhat better >> > so now we get: >> > <bb 30>: >> > # prephitmp_222 = PHI <_52(27), pretmp_245(29)> >> > _57 = prephitmp_222 + 2; >> > pool_40(D)->ptr = _57; >> > __builtin_unreachable (); >> > >> > Why DSE does not eliminate the stores prior noreturn const function? Probably because it has a very special special-casing of "function exit" and because pool_40(D)->ptr is a global store which cannot be eliminated (it doesn't special case noreturn const function "exits"). >> > Bootstrapped/regtested x86_64-linux, OK? >> > >> > Honza >> > * tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Remove dead parameters >> > of BUILT_IN_UNREACHABLE >> >> Shouldn't this be done when you actually change the call to >> __builtin_unreachable ()? I mean, __builtin_unreachable () has no >> arguments, so leaving any arguments there is broken IL, even if you clean it >> up during the next DCE. > > Hmm, I tought there was some reason to not do so becuase of inplace folding and memory-SSA. Well, reducing the number of used ops is fine for in-place folding. memory-SSA shouldn't be an issue. Richard. > I can give a try to update all the places we can put builtin_unreachable into IL. > (I wonder if that also include standard constant propagation) > > Honza >> >> > --- tree-ssa-dce.c (revision 218610) >> > +++ tree-ssa-dce.c (working copy) >> > @@ -250,6 +250,15 @@ mark_stmt_if_obviously_necessary (gimple >> > case BUILT_IN_ALLOCA: >> > case BUILT_IN_ALLOCA_WITH_ALIGN: >> > return; >> > + case BUILT_IN_UNREACHABLE: >> > + /* All parameters of BUILT_IN_UNREACHABLE are dead. Remove them >> > + from the stmt, so we can remove their definitions. */ >> > + if (gimple_call_num_args (stmt)) >> > + { >> > + gimple_set_num_ops (stmt, 3); >> > + update_stmt (stmt); >> > + } >> > + break; >> > >> > default:; >> > } >> >> Jakub
On 12/11/14 10:06, Jan Hubicka wrote: > Hi, > in firefox .optimized dumps one can see few places where __builtin_unreachable > is called (as a result of devirtualization code proving the code path to be > undefined). There is usually some argument setup for the parameters of > __builtin_unreachable that are dead. This patch makes it somewhat better > so now we get: > <bb 30>: > # prephitmp_222 = PHI <_52(27), pretmp_245(29)> > _57 = prephitmp_222 + 2; > pool_40(D)->ptr = _57; > __builtin_unreachable (); > > Why DSE does not eliminate the stores prior noreturn const function? > > Bootstrapped/regtested x86_64-linux, OK? > > Honza > * tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Remove dead parameters > of BUILT_IN_UNREACHABLE ISTM that we shouldn't need any special handling in DCE for this. When something is transformed into a builtin_unreachable(), arguments which are SSA_NAMEs which are no longer referenced ought to just go away via the normal DCE mechanisms. A memory store has side effects that might be observable prior to reaching the builtin_unreachable, thus it ought not be deleted. At least that's my recollection of how we wanted this to work for other transformations which turned turned statements with undefined behaviour into a __builtin_unreachable. jeff
Index: tree-ssa-dce.c =================================================================== --- tree-ssa-dce.c (revision 218610) +++ tree-ssa-dce.c (working copy) @@ -250,6 +250,15 @@ mark_stmt_if_obviously_necessary (gimple case BUILT_IN_ALLOCA: case BUILT_IN_ALLOCA_WITH_ALIGN: return; + case BUILT_IN_UNREACHABLE: + /* All parameters of BUILT_IN_UNREACHABLE are dead. Remove them + from the stmt, so we can remove their definitions. */ + if (gimple_call_num_args (stmt)) + { + gimple_set_num_ops (stmt, 3); + update_stmt (stmt); + } + break; default:; }