https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102542
--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Aldy Hernandez from comment #5)
> (In reply to rguent...@suse.de from comment #4)
> > On Thu, 30 Sep 2021, aldyh at gcc dot gnu.org wrote:
>
> Thanks for the loop explanation. It's quite helpful.
>
> > So the threading at hand rotates the loop (which is only so-so OK),
> > that might be problematic in case it creates a non-do-while loop
> > after loop header copying.
> >
> > In this process it might also lose track of the loop, causing it to
> > be re-discovered as separate entity which has problems of its own.
> >
> > IMHO we should avoid threadings through a loop header if the path
> > ends in the loop itself, even if it doesn't introduce a new entry but
> > just re-directs the existing one.
>
> It looks like Jeff and you have stumbled on various corner cases we must
> address in cancel_invalid_paths().
>
> Could I inconvenience you to tweak this function with your insight? It's a
> tiny function, and it seems you're much more qualified to add the
> restriction code. If not, I'm sure I can stumble around it and send it for
> review.
Something like
diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
index dcabfdb30d2..b1b77e91176 100644
--- a/gcc/tree-ssa-threadupdate.c
+++ b/gcc/tree-ssa-threadupdate.c
@@ -2811,6 +2811,14 @@ jt_path_registry::cancel_invalid_paths
(vec<jump_thread_edge *> &path)
cancel_thread (&path, "Path crosses loops");
return true;
}
+ edge entry = path[0]->e;
+ edge exit = path[path.length () - 1]->e;
+ if (entry->src->loop_father != exit->dest->loop_father
+ && !flow_loop_nested_p (exit->src->loop_father,
entry->dest->loop_father))
+ {
+ cancel_thread (&path, "Path rotates loop");
+ return true;
+ }
return false;
}
that is, the path should either start & end in the same loop or overall
exit the loop it starts in but never enter a loop (the above also catches
creating irreducible loops, not only rotation).
I find the
// The first entry represents the block with an outgoing edge
// that we will redirect to the jump threading path. Thus we
// don't care about that block's loop father.
if ((i > 0 && e->src->loop_father != loop)
|| e->dest->loop_father != loop)
path_crosses_loops = true;
mildly confusing and don't really understand what it is about. What's
not present yet is avoiding to thread through the loop header. For that
I'd add in the loop
if (e->dest->loop_father->header == e->dest
&& !flow_loop_nested_p (exit->dest->loop_father,
e->dest->loop_father)
cancel_thread ("Path crosses loop header but does not exit it");
such threadings tend to create sub-loops which _might_ be OK, not sure.
This is stricter than the above suggestion since it also covers paths
that remain in the same loop but it should cover the above cases as well.
Note it's difficult to tell whether the created sub-loop is irreducible.
This check should also prevent us from creating multiple latches.
> Orthogonal to this, it does look like the extra thread is causing an ICE in
> slp which must be addressed??
Yes, we have duplicates for this crash which is caused by some changes
Richard Sandiford did.
