On Fri, 5 Aug 2016, Richard Biener wrote:
> On Fri, 5 Aug 2016, Richard Biener wrote:
>
> >
> > This avoids regressing gcc.dg/tree-ssa/pr21417.c with the fix for
> > PR72772 where after it a forwarder block no longer is present.
> > It's easy to simply create it when FSM threading faces the situation
> > that the edge ending the path enters a loop.
> >
> > I also fixed the costs for obviously related anon SSA names (when
> > they are the same).
> >
> > Bootstrap and regtest running on x86_64-unknown-linux-gnu, I'll apply
> > if that finishes successfully.
>
> Ok, so that re-introduces the c-c++-common/ubsan/pr71403-2.c miscompile
> where we mash two loops retaining a bogus loop->nb_iterations_upper_bound.
>
> I think those two testcases show that it should be the dominance
> relation between the entered loop header and the threaded path
> that decides whether we can thread or not -- the path may not
> become another latch of this loop. I can't seem to figure out
> a correct condition for this based on the threading path but the
> following works (maybe by accident as I only have those two
> testcases):
>
> Index: gcc/tree-ssa-threadbackward.c
> ===================================================================
> --- gcc/tree-ssa-threadbackward.c (revision 239164)
> +++ gcc/tree-ssa-threadbackward.c (working copy)
> @@ -35,6 +35,7 @@ along with GCC; see the file COPYING3.
> #include "tree-pass.h"
> #include "gimple-ssa.h"
> #include "tree-phinodes.h"
> +#include "cfghooks.h"
>
> static int max_threaded_paths;
>
> @@ -560,6 +562,14 @@ fsm_find_control_statement_thread_paths
> edge taken_edge = profitable_jump_thread_path (path, bbi, name,
> arg);
> if (taken_edge)
> {
> + /* If the taken edge is a loop entry avoid mashing two
> + loops into one with multiple latches by splitting
> + the edge. This only works if that block won't become
> + a latch of this loop. */
> + if ((bb_loop_depth (taken_edge->src)
> + < bb_loop_depth (taken_edge->dest))
> + && ! single_succ_p (bbi))
> + split_edge (taken_edge);
> if (bb_loop_depth (taken_edge->src)
> >= bb_loop_depth (taken_edge->dest))
> convert_and_register_jump_thread_path (path, taken_edge);
>
> note you need the PR72772 fix to trigger all this.
I think it also shows that a thread path ending not in the inner loop
header but on the preheader block might be miscompiled as well given
said block can become latch as well if other paths leading into it
vanish. Thus there is a latent wrong-code issue with the backward
threading (and maybe it really should invalidate
nb_iterations_upper_bound of all possibly participating loops,
which I guess is the loop the backedge we thread through plus all
inner loops).
> Any idea?
>
> Thanks,
> Richard.
>
> > Richard.
> >
> > 2016-08-05 Richard Biener <rguent...@suse.de>
> >
> > * tree-ssa-threadbackward.c: Include cfghooks.h.
> > (profitable_jump_thread_path): Treat same SSA names related.
> > (fsm_find_control_statement_thread_paths): When the taken edge
> > enters a loop split it instead of giving up.
> >
> > Index: gcc/tree-ssa-threadbackward.c
> > ===================================================================
> > --- gcc/tree-ssa-threadbackward.c (revision 239164)
> > +++ gcc/tree-ssa-threadbackward.c (working copy)
> > @@ -35,6 +35,7 @@ along with GCC; see the file COPYING3.
> > #include "tree-pass.h"
> > #include "gimple-ssa.h"
> > #include "tree-phinodes.h"
> > +#include "cfghooks.h"
> >
> > static int max_threaded_paths;
> >
> > @@ -206,8 +207,9 @@ profitable_jump_thread_path (vec<basic_b
> > /* Note that if both NAME and DST are anonymous
> > SSA_NAMEs, then we do not have enough information
> > to consider them associated. */
> > - if ((SSA_NAME_VAR (dst) != SSA_NAME_VAR (name)
> > - || !SSA_NAME_VAR (dst))
> > + if (dst != name
> > + && (SSA_NAME_VAR (dst) != SSA_NAME_VAR (name)
> > + || !SSA_NAME_VAR (dst))
> > && !virtual_operand_p (dst))
> > ++n_insns;
> > }
> > @@ -560,9 +562,13 @@ fsm_find_control_statement_thread_paths
> > edge taken_edge = profitable_jump_thread_path (path, bbi, name, arg);
> > if (taken_edge)
> > {
> > + /* If the taken edge is a loop entry avoid mashing two
> > + loops into one with multiple latches by splitting
> > + the edge. */
> > if (bb_loop_depth (taken_edge->src)
> > - >= bb_loop_depth (taken_edge->dest))
> > - convert_and_register_jump_thread_path (path, taken_edge);
> > + < bb_loop_depth (taken_edge->dest))
> > + split_edge (taken_edge);
> > + convert_and_register_jump_thread_path (path, taken_edge);
> > path->pop ();
> > }
> > }
> > @@ -586,9 +592,13 @@ fsm_find_control_statement_thread_paths
> > name, arg);
> > if (taken_edge)
> > {
> > + /* If the taken edge is a loop entry avoid mashing two
> > + loops into one with multiple latches by splitting
> > + the edge. */
> > if (bb_loop_depth (taken_edge->src)
> > - >= bb_loop_depth (taken_edge->dest))
> > - convert_and_register_jump_thread_path (path, taken_edge);
> > + < bb_loop_depth (taken_edge->dest))
> > + split_edge (taken_edge);
> > + convert_and_register_jump_thread_path (path, taken_edge);
> > path->pop ();
> > }
> >
> >
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
