Message ID | alpine.LSU.2.20.1905031324400.10704@zhemvz.fhfr.qr |
---|---|
State | New |
Headers | show |
Series | Fix PR90316 | expand |
On Fri, 3 May 2019, Richard Biener wrote: > > I am testing the following patch to remove the code determining > the target virtual operand to walk to and instead compute it > based on the immediate dominator which we will reach anyways > (or a dominating block) during maybe_skip_until. > > More simplifying might be possible but I'm trying to keep the > patch small and suitable for backporting up to the GCC 8 branch > where this regressed. > > Note this will handle even more CFG shapes now and seems to > expose some uninit warnings in dwarf2out.c (at least). I can't seem to find an initializer that would "trap" on use so I'm going to do Index: gcc/dwarf2out.c =================================================================== --- gcc/dwarf2out.c (revision 270849) +++ gcc/dwarf2out.c (working copy) @@ -15461,7 +15461,7 @@ mem_loc_descriptor (rtx rtl, machine_mod if (mode != GET_MODE (rtl) && GET_MODE (rtl) != VOIDmode) return NULL; - scalar_int_mode int_mode, inner_mode, op1_mode; + scalar_int_mode int_mode = SImode, inner_mode, op1_mode; switch (GET_CODE (rtl)) { case POST_INC: unless somebody comes up with something clever over the weekend... Richard. > Bootstrap & regtest running on x86_64-unknown-linux-gnu. > > Richard. > > 2019-05-03 Richard Biener <rguenther@suse.de> > > PR tree-optimization/90316 > * tree-ssa-alias.c (maybe_skip_until): Pass in target BB, > compute target on demand. > (get_continuation_for_phi): Remove code walking stmts to > get to a target virtual operand which could end up being > quadratic. > > Index: gcc/tree-ssa-alias.c > =================================================================== > --- gcc/tree-ssa-alias.c (revision 270847) > +++ gcc/tree-ssa-alias.c (working copy) > @@ -2598,8 +2598,8 @@ stmt_kills_ref_p (gimple *stmt, tree ref > case false is returned. The walk starts with VUSE, one argument of PHI. */ > > static bool > -maybe_skip_until (gimple *phi, tree target, ao_ref *ref, > - tree vuse, unsigned int *cnt, bitmap *visited, > +maybe_skip_until (gimple *phi, tree &target, basic_block target_bb, > + ao_ref *ref, tree vuse, unsigned int *cnt, bitmap *visited, > bool abort_on_visited, > void *(*translate)(ao_ref *, tree, void *, bool *), > void *data) > @@ -2615,6 +2615,19 @@ maybe_skip_until (gimple *phi, tree targ > while (vuse != target) > { > gimple *def_stmt = SSA_NAME_DEF_STMT (vuse); > + /* If we are searching for the target VUSE by walking up to > + TARGET_BB dominating the original PHI we are finished once > + we reach a default def or a definition in a block dominating > + that block. Update TARGET and return. */ > + if (!target > + && (gimple_nop_p (def_stmt) > + || dominated_by_p (CDI_DOMINATORS, > + target_bb, gimple_bb (def_stmt)))) > + { > + target = vuse; > + return true; > + } > + > /* Recurse for PHI nodes. */ > if (gimple_code (def_stmt) == GIMPLE_PHI) > { > @@ -2698,49 +2711,17 @@ get_continuation_for_phi (gimple *phi, a > arg0 = NULL_TREE; > } > /* If not, look if we can reach such candidate by walking defs > - of a PHI arg without crossing other PHIs. */ > - if (! arg0) > - for (i = 0; i < nargs; ++i) > - { > - arg0 = PHI_ARG_DEF (phi, i); > - gimple *def = SSA_NAME_DEF_STMT (arg0); > - /* Backedges can't work. */ > - if (dominated_by_p (CDI_DOMINATORS, > - gimple_bb (def), phi_bb)) > - continue; > - /* See below. */ > - if (gimple_code (def) == GIMPLE_PHI) > - continue; > - while (! dominated_by_p (CDI_DOMINATORS, > - phi_bb, gimple_bb (def))) > - { > - arg0 = gimple_vuse (def); > - if (SSA_NAME_IS_DEFAULT_DEF (arg0)) > - break; > - def = SSA_NAME_DEF_STMT (arg0); > - if (gimple_code (def) == GIMPLE_PHI) > - { > - /* Do not try to look through arbitrarily complicated > - CFGs. For those looking for the first VUSE starting > - from the end of the immediate dominator of phi_bb > - is likely faster. */ > - arg0 = NULL_TREE; > - goto next; > - } > - } > - break; > -next:; > - } > - if (! arg0) > - return NULL_TREE; > + until we hit the immediate dominator. maybe_skip_until will > + do that for us. */ > + basic_block dom = get_immediate_dominator (CDI_DOMINATORS, phi_bb); > > - /* Then check against the found candidate. */ > + /* Then check against the (to be) found candidate. */ > for (i = 0; i < nargs; ++i) > { > arg1 = PHI_ARG_DEF (phi, i); > if (arg1 == arg0) > ; > - else if (! maybe_skip_until (phi, arg0, ref, arg1, cnt, visited, > + else if (! maybe_skip_until (phi, arg0, dom, ref, arg1, cnt, visited, > abort_on_visited, > /* Do not translate when walking over > backedges. */ >
Richard Biener <rguenther@suse.de> writes: > On Fri, 3 May 2019, Richard Biener wrote: > >> >> I am testing the following patch to remove the code determining >> the target virtual operand to walk to and instead compute it >> based on the immediate dominator which we will reach anyways >> (or a dominating block) during maybe_skip_until. >> >> More simplifying might be possible but I'm trying to keep the >> patch small and suitable for backporting up to the GCC 8 branch >> where this regressed. >> >> Note this will handle even more CFG shapes now and seems to >> expose some uninit warnings in dwarf2out.c (at least). > > I can't seem to find an initializer that would "trap" on use > so I'm going to do > > Index: gcc/dwarf2out.c > =================================================================== > --- gcc/dwarf2out.c (revision 270849) > +++ gcc/dwarf2out.c (working copy) > @@ -15461,7 +15461,7 @@ mem_loc_descriptor (rtx rtl, machine_mod > if (mode != GET_MODE (rtl) && GET_MODE (rtl) != VOIDmode) > return NULL; > > - scalar_int_mode int_mode, inner_mode, op1_mode; > + scalar_int_mode int_mode = SImode, inner_mode, op1_mode; > switch (GET_CODE (rtl)) > { > case POST_INC: > > unless somebody comes up with something clever over the weekend... Nothing clever, but something rare like BImode is probably safer than SImode, in case doing this masks real "uninitialised" uses in future. Thanks, Richard
On Sat, 4 May 2019, Richard Sandiford wrote: > Richard Biener <rguenther@suse.de> writes: > > On Fri, 3 May 2019, Richard Biener wrote: > > > >> > >> I am testing the following patch to remove the code determining > >> the target virtual operand to walk to and instead compute it > >> based on the immediate dominator which we will reach anyways > >> (or a dominating block) during maybe_skip_until. > >> > >> More simplifying might be possible but I'm trying to keep the > >> patch small and suitable for backporting up to the GCC 8 branch > >> where this regressed. > >> > >> Note this will handle even more CFG shapes now and seems to > >> expose some uninit warnings in dwarf2out.c (at least). > > > > I can't seem to find an initializer that would "trap" on use > > so I'm going to do > > > > Index: gcc/dwarf2out.c > > =================================================================== > > --- gcc/dwarf2out.c (revision 270849) > > +++ gcc/dwarf2out.c (working copy) > > @@ -15461,7 +15461,7 @@ mem_loc_descriptor (rtx rtl, machine_mod > > if (mode != GET_MODE (rtl) && GET_MODE (rtl) != VOIDmode) > > return NULL; > > > > - scalar_int_mode int_mode, inner_mode, op1_mode; > > + scalar_int_mode int_mode = SImode, inner_mode, op1_mode; > > switch (GET_CODE (rtl)) > > { > > case POST_INC: > > > > unless somebody comes up with something clever over the weekend... > > Nothing clever, but something rare like BImode is probably safer than > SImode, in case doing this masks real "uninitialised" uses in future. Ick, and I forgot to install this hunk when I committed it this morning. Thus fixed as obvious now, with BImode. Richard.
Hi! On 2019-05-06T11:36:22+0200, Richard Biener <rguenther@suse.de> wrote: > On Sat, 4 May 2019, Richard Sandiford wrote: >> Richard Biener <rguenther@suse.de> writes: >> > On Fri, 3 May 2019, Richard Biener wrote: >> >> I am testing the following patch [...] ... which apparently also got backported to gcc-9-branch eventually... >> >> Note this will handle even more CFG shapes now and seems to >> >> expose some uninit warnings in dwarf2out.c (at least). ..., and when building gcc-9-branch with '--enable-checking=yes,extra,rtl' (apparently I'm the only one doing that, huh?), runs into the following (at least I suppose that's what's meant with "expose some uninit warnings in dwarf2out.c"?): In file included from [...]/source-gcc/gcc/coretypes.h:433, from [...]/source-gcc/gcc/dwarf2out.c:60: [...]/source-gcc/gcc/machmode.h: In function 'dw_loc_descr_node* mem_loc_descriptor(rtx, machine_mode, machine_mode, var_init_status)': [...]/source-gcc/gcc/machmode.h:520:42: error: 'int_mode' may be used uninitialized in this function [-Werror=maybe-uninitialized] 520 | ? mode_size_inline (mode) : mode_size[mode]); | ^~~~ [...]/source-gcc/gcc/dwarf2out.c:15464:19: note: 'int_mode' was declared here 15464 | scalar_int_mode int_mode, inner_mode, op1_mode; | ^~~~~~~~ cc1plus: all warnings being treated as errors make[3]: *** [dwarf2out.o] Error 1 >> > I can't seem to find an initializer that would "trap" on use >> > so I'm going to do >> > >> > Index: gcc/dwarf2out.c >> > =================================================================== >> > --- gcc/dwarf2out.c (revision 270849) >> > +++ gcc/dwarf2out.c (working copy) >> > @@ -15461,7 +15461,7 @@ mem_loc_descriptor (rtx rtl, machine_mod >> > if (mode != GET_MODE (rtl) && GET_MODE (rtl) != VOIDmode) >> > return NULL; >> > >> > - scalar_int_mode int_mode, inner_mode, op1_mode; >> > + scalar_int_mode int_mode = SImode, inner_mode, op1_mode; >> > switch (GET_CODE (rtl)) >> > { >> > case POST_INC: >> > >> > unless somebody comes up with something clever over the weekend... >> >> Nothing clever, but something rare like BImode is probably safer than >> SImode, in case doing this masks real "uninitialised" uses in future. > > Ick, and I forgot to install this hunk when I committed it this morning. > > Thus fixed as obvious now, with BImode. ..., so I backported that fix (or is it rather "fix"?) to gcc-9-branch in r277608 "Avoid '-Wmaybe-uninitialized' diagnostic in 'gcc/dwarf2out.c'", see attached. Grüße Thomas
On Wed, Oct 30, 2019 at 11:57:12AM +0100, Thomas Schwinge wrote: > ..., and when building gcc-9-branch with > '--enable-checking=yes,extra,rtl' (apparently I'm the only one doing > that, huh?), runs into the following (at least I suppose that's what's I'm testing release branches with ../configure --enable-languages=default,ada,obj-c++,lto,go,brig,d --enable-checking=yes,rtl,extra too, and saw (last time I've bootstrapped/regtested gcc-9-branch this way was Oct 21st though): ../../gcc/machmode.h: In function ‘dw_loc_descr_node* mem_loc_descriptor(rtx, machine_mode, machine_mode, var_init_status)’: ../../gcc/machmode.h:520:42: warning: ‘int_mode’ may be used uninitialized in this function [-Wmaybe-uninitialized] 520 | ? mode_size_inline (mode) : mode_size[mode]); | ^~~~ ../../gcc/dwarf2out.c:15464:19: note: ‘int_mode’ was declared here 15464 | scalar_int_mode int_mode, inner_mode, op1_mode; | ^~~~~~~~ and no fatal error. Are you using --enable-werror or something similar in addition? Jakub
Hi! On 2019-10-30T12:19:28+0100, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Oct 30, 2019 at 11:57:12AM +0100, Thomas Schwinge wrote: >> ..., and when building gcc-9-branch with >> '--enable-checking=yes,extra,rtl' (apparently I'm the only one doing >> that, huh?), runs into the following (at least I suppose that's what's > > I'm testing release branches with > ../configure --enable-languages=default,ada,obj-c++,lto,go,brig,d --enable-checking=yes,rtl,extra > too, and saw (last time I've bootstrapped/regtested gcc-9-branch > this way was Oct 21st though): > ../../gcc/machmode.h: In function ‘dw_loc_descr_node* mem_loc_descriptor(rtx, machine_mode, machine_mode, var_init_status)’: > ../../gcc/machmode.h:520:42: warning: ‘int_mode’ may be used uninitialized in this function [-Wmaybe-uninitialized] > 520 | ? mode_size_inline (mode) : mode_size[mode]); > | ^~~~ > ../../gcc/dwarf2out.c:15464:19: note: ‘int_mode’ was declared here > 15464 | scalar_int_mode int_mode, inner_mode, op1_mode; > | ^~~~~~~~ > and no fatal error. Are you using --enable-werror or something similar > in addition? Eh, you're right, of course: '--enable-werror', not '--enable-checking' is relevant here. (Recovering from a cold -- brain still a bit "slow".) Grüße Thomas
Index: gcc/tree-ssa-alias.c =================================================================== --- gcc/tree-ssa-alias.c (revision 270847) +++ gcc/tree-ssa-alias.c (working copy) @@ -2598,8 +2598,8 @@ stmt_kills_ref_p (gimple *stmt, tree ref case false is returned. The walk starts with VUSE, one argument of PHI. */ static bool -maybe_skip_until (gimple *phi, tree target, ao_ref *ref, - tree vuse, unsigned int *cnt, bitmap *visited, +maybe_skip_until (gimple *phi, tree &target, basic_block target_bb, + ao_ref *ref, tree vuse, unsigned int *cnt, bitmap *visited, bool abort_on_visited, void *(*translate)(ao_ref *, tree, void *, bool *), void *data) @@ -2615,6 +2615,19 @@ maybe_skip_until (gimple *phi, tree targ while (vuse != target) { gimple *def_stmt = SSA_NAME_DEF_STMT (vuse); + /* If we are searching for the target VUSE by walking up to + TARGET_BB dominating the original PHI we are finished once + we reach a default def or a definition in a block dominating + that block. Update TARGET and return. */ + if (!target + && (gimple_nop_p (def_stmt) + || dominated_by_p (CDI_DOMINATORS, + target_bb, gimple_bb (def_stmt)))) + { + target = vuse; + return true; + } + /* Recurse for PHI nodes. */ if (gimple_code (def_stmt) == GIMPLE_PHI) { @@ -2698,49 +2711,17 @@ get_continuation_for_phi (gimple *phi, a arg0 = NULL_TREE; } /* If not, look if we can reach such candidate by walking defs - of a PHI arg without crossing other PHIs. */ - if (! arg0) - for (i = 0; i < nargs; ++i) - { - arg0 = PHI_ARG_DEF (phi, i); - gimple *def = SSA_NAME_DEF_STMT (arg0); - /* Backedges can't work. */ - if (dominated_by_p (CDI_DOMINATORS, - gimple_bb (def), phi_bb)) - continue; - /* See below. */ - if (gimple_code (def) == GIMPLE_PHI) - continue; - while (! dominated_by_p (CDI_DOMINATORS, - phi_bb, gimple_bb (def))) - { - arg0 = gimple_vuse (def); - if (SSA_NAME_IS_DEFAULT_DEF (arg0)) - break; - def = SSA_NAME_DEF_STMT (arg0); - if (gimple_code (def) == GIMPLE_PHI) - { - /* Do not try to look through arbitrarily complicated - CFGs. For those looking for the first VUSE starting - from the end of the immediate dominator of phi_bb - is likely faster. */ - arg0 = NULL_TREE; - goto next; - } - } - break; -next:; - } - if (! arg0) - return NULL_TREE; + until we hit the immediate dominator. maybe_skip_until will + do that for us. */ + basic_block dom = get_immediate_dominator (CDI_DOMINATORS, phi_bb); - /* Then check against the found candidate. */ + /* Then check against the (to be) found candidate. */ for (i = 0; i < nargs; ++i) { arg1 = PHI_ARG_DEF (phi, i); if (arg1 == arg0) ; - else if (! maybe_skip_until (phi, arg0, ref, arg1, cnt, visited, + else if (! maybe_skip_until (phi, arg0, dom, ref, arg1, cnt, visited, abort_on_visited, /* Do not translate when walking over backedges. */