| Message ID | alpine.LSU.2.20.1903081519480.4934@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
| Series | Cleanup TODO handling for CFG clenaup & SSA update | expand |
On 3/8/19 7:23 AM, Richard Biener wrote: > > There's an old comment > > /* When cleanup_tree_cfg merges consecutive blocks, it may > perform some simplistic propagation when removing single > valued PHI nodes. This propagation may, in turn, cause the > SSA form to become out-of-date (see PR 22037). So, even > if the parent pass had not scheduled an SSA update, we may > still need to do one. */ > if (!(flags & TODO_update_ssa_any) && need_ssa_update_p (cfun)) > flags |= TODO_update_ssa; > > which is from times we've had multiple virtual operands. After > those went away we could still run into this for example when > propagating a non-const function address into an indirect call > through a const function type. This has been fixed as well > (we retain the const-ness of the call). Thus the above is > no longer necessary and we can simplify the code. > > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress. > > I'm not really nervous about this change but if you think it > should wait for GCC 10 speak up. > > Richard. > > 2019-03-08 Richard Biener <rguenther@suse.de> > > * passes.c (execute_function_todo): Remove dead code. What's driving the desire to change this for gcc-9? I think it's a fine cleanup for gcc-10, but it's not clear to me we want to push it into gcc-9. jeff
On March 8, 2019 3:52:36 PM GMT+01:00, Jeff Law <law@redhat.com> wrote: >On 3/8/19 7:23 AM, Richard Biener wrote: >> >> There's an old comment >> >> /* When cleanup_tree_cfg merges consecutive blocks, it may >> perform some simplistic propagation when removing single >> valued PHI nodes. This propagation may, in turn, cause the >> SSA form to become out-of-date (see PR 22037). So, even >> if the parent pass had not scheduled an SSA update, we may >> still need to do one. */ >> if (!(flags & TODO_update_ssa_any) && need_ssa_update_p (cfun)) >> flags |= TODO_update_ssa; >> >> which is from times we've had multiple virtual operands. After >> those went away we could still run into this for example when >> propagating a non-const function address into an indirect call >> through a const function type. This has been fixed as well >> (we retain the const-ness of the call). Thus the above is >> no longer necessary and we can simplify the code. >> >> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress. >> >> I'm not really nervous about this change but if you think it >> should wait for GCC 10 speak up. >> >> Richard. >> >> 2019-03-08 Richard Biener <rguenther@suse.de> >> >> * passes.c (execute_function_todo): Remove dead code. >What's driving the desire to change this for gcc-9? I think it's a >fine >cleanup for gcc-10, but it's not clear to me we want to push it into >gcc-9. Just that I came along this with the previous related CFG cleanup fix and got the time to test it. Queued for GCC 10 instead. Richard. >jeff
On Fri, Mar 8, 2019 at 4:59 PM Richard Biener <rguenther@suse.de> wrote: > > On March 8, 2019 3:52:36 PM GMT+01:00, Jeff Law <law@redhat.com> wrote: > >On 3/8/19 7:23 AM, Richard Biener wrote: > >> > >> There's an old comment > >> > >> /* When cleanup_tree_cfg merges consecutive blocks, it may > >> perform some simplistic propagation when removing single > >> valued PHI nodes. This propagation may, in turn, cause the > >> SSA form to become out-of-date (see PR 22037). So, even > >> if the parent pass had not scheduled an SSA update, we may > >> still need to do one. */ > >> if (!(flags & TODO_update_ssa_any) && need_ssa_update_p (cfun)) > >> flags |= TODO_update_ssa; > >> > >> which is from times we've had multiple virtual operands. After > >> those went away we could still run into this for example when > >> propagating a non-const function address into an indirect call > >> through a const function type. This has been fixed as well > >> (we retain the const-ness of the call). Thus the above is > >> no longer necessary and we can simplify the code. > >> > >> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress. > >> > >> I'm not really nervous about this change but if you think it > >> should wait for GCC 10 speak up. > >> > >> Richard. > >> > >> 2019-03-08 Richard Biener <rguenther@suse.de> > >> > >> * passes.c (execute_function_todo): Remove dead code. > >What's driving the desire to change this for gcc-9? I think it's a > >fine > >cleanup for gcc-10, but it's not clear to me we want to push it into > >gcc-9. > > Just that I came along this with the previous related CFG cleanup fix and got the time to test it. > > Queued for GCC 10 instead. Applied as r270832. Richard. > Richard. > > >jeff >
Index: gcc/passes.c =================================================================== --- gcc/passes.c (revision 269302) +++ gcc/passes.c (working copy) @@ -1924,26 +1924,12 @@ execute_function_todo (function *fn, voi push_cfun (fn); - /* Always cleanup the CFG before trying to update SSA. */ + /* If we need to cleanup the CFG let it perform a needed SSA update. */ if (flags & TODO_cleanup_cfg) - { - cleanup_tree_cfg (flags & TODO_update_ssa_any); - - /* When cleanup_tree_cfg merges consecutive blocks, it may - perform some simplistic propagation when removing single - valued PHI nodes. This propagation may, in turn, cause the - SSA form to become out-of-date (see PR 22037). So, even - if the parent pass had not scheduled an SSA update, we may - still need to do one. */ - if (!(flags & TODO_update_ssa_any) && need_ssa_update_p (cfun)) - flags |= TODO_update_ssa; - } - - if (flags & TODO_update_ssa_any) - { - unsigned update_flags = flags & TODO_update_ssa_any; - update_ssa (update_flags); - } + cleanup_tree_cfg (flags & TODO_update_ssa_any); + else if (flags & TODO_update_ssa_any) + update_ssa (flags & TODO_update_ssa_any); + gcc_assert (!need_ssa_update_p (fn)); if (flag_tree_pta && (flags & TODO_rebuild_alias)) compute_may_aliases ();
There's an old comment /* When cleanup_tree_cfg merges consecutive blocks, it may perform some simplistic propagation when removing single valued PHI nodes. This propagation may, in turn, cause the SSA form to become out-of-date (see PR 22037). So, even if the parent pass had not scheduled an SSA update, we may still need to do one. */ if (!(flags & TODO_update_ssa_any) && need_ssa_update_p (cfun)) flags |= TODO_update_ssa; which is from times we've had multiple virtual operands. After those went away we could still run into this for example when propagating a non-const function address into an indirect call through a const function type. This has been fixed as well (we retain the const-ness of the call). Thus the above is no longer necessary and we can simplify the code. Bootstrapped on x86_64-unknown-linux-gnu, testing in progress. I'm not really nervous about this change but if you think it should wait for GCC 10 speak up. Richard. 2019-03-08 Richard Biener <rguenther@suse.de> * passes.c (execute_function_todo): Remove dead code.