On Mon, 29 Jan 2018, Jeff Law wrote:
> On 01/29/2018 04:37 PM, Jakub Jelinek wrote:
> > Hi!
> >
> > As mentioned in the PR, cunroll sometimes registers some SSA_NAMEs for
> > renaming and then invokes some SCEV using functions before finally updating
> > the SSA form. On the testcase we end up with 3 degenerate PHIs pointing at
> > each other, so follow_copies_to_constant loops forever.
> I'm at a loss to understand how we could get in that situation. Was it
> edge removal (if so for which PHI) or the duplicate process that created
> the loop?
The issue is that
static bool
tree_unroll_loops_completely_1 (bool may_increase_size, bool unroll_outer,
bitmap father_bbs, struct loop *loop)
{
struct loop *loop_father;
bool changed = false;
struct loop *inner;
enum unroll_level ul;
/* Process inner loops first. */
for (inner = loop->inner; inner != NULL; inner = inner->next)
changed |= tree_unroll_loops_completely_1 (may_increase_size,
unroll_outer, father_bbs,
inner);
when recursing tree_unroll_loops_completely_1 appends unrolled
bodies and loops contained therein in the loop->inner chain. We
specifically do _not_ allow processing of outer loops here due
to not up-to-date SSA form and then iterate the unrolling process
instead:
/* If we changed an inner loop we cannot process outer loops in this
iteration because SSA form is not up-to-date. Continue with
siblings of outer loops instead. */
if (changed)
return true;
so I think the proper fix is to avoid visiting loops that were added
in the above iteration over loop->inner. For example by collecting
the loop->inner chain in a vec and then iterating over that (much
like FOR_EACH_LOOP does). Or rely on the fact that loop->num of new
loops will be >= number_of_loops () [number_of_loops () is really
mis-named, would be a good time to add max_loop_num () or so]
Richard.
> We've seen one case where this happened inside DOM, but it was only
> because we ignored an unexecutable edge which then caused a PHI to
> become a degenerate. One or more of the PHI args in the degenerate
> was marked as EDGE_DFS_BACK on its associated edge.
>
> Are any of the PHI args in pr84111 marked in a similar manner?
>
> >
> > The following patch has 2 different fixes for this, one is to avoid looking
> > at SSA_NAMEs registered for update (in theory all we care are the old ssa
> > names, but we don't have an API to query just those), the other is to put
> > some upper bound on how many times we follow SSA_NAMEs (either single defs
> > or degenerate phis). Either of those changes is sufficient to fix this.
> Interestingly enough I was looking at a bug several weeks ago where the
> ability to query one of the sets (I can't remember if it was old or new)
> of names for rewriting would have been useful.
>
>
> >
> > During x86_64-linux and i686-linux bootstraps/regtests
> > follow_copies_to_constant
> > actually never returned anything useful, so the patch doesn't regress at
> > least on these targets anything, there are quite a few cases where SSA_NAME
> > is name_registered_for_update_p but the function would never return anything
> > but var in those cases. And the highest depth ever seen was 3, except for
> > the newly added testcase if the && !name_registered_for_update_p (res) part
> > is commented out.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> > 2018-01-29 Jakub Jelinek <ja...@redhat.com>
> >
> > PR tree-optimization/84111
> > * tree-scalar-evolution.c: Include tree-into-ssa.h.
> > (follow_copies_to_constant): Stop on SSA_NAMEs registered for update,
> > loop at most 128 times.
> >
> > * gcc.c-torture/compile/pr84111.c: New test.
> Probably reasonable. Though I would like to better understand how we
> got the degenerates forming a loop before final ack.
>
> jeff
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
