Hello,
On Thu, 9 Sep 2021, Richard Biener wrote:
> > I believe something like below would be appropriate, it disables
> > threading if the path contains a latch at the non-last position (due
> > to being backwards on the non-first position in the array). I.e. it
> > disables rotating the loop if there's danger of polluting the back
> > edge. It might be improved if the blocks following (preceding!) the
> > latch are themself empty because then no code is duplicated. It might
> > also be improved if the latch is already non-empty. That code should
> > probably only be active before the loop optimizers, but currently the
> > backward threader isn't differentiating between before/after
> > loop-optims.
> >
> > I haven't tested this patch at all, except that it fixes the testcase
> > :)
>
> Lame comment at the current end of the thread - it's not threading
> through the latch but threading through the loop header that's
> problematic,
I beg to differ, but that's probably because of the ambiguity of the word
"through" (does it or does it not include the ends of the path :) ). If
you thread through the loop header from the entry block (i.e. duplicate
code from header to entry) all would be fine (or not, in case you created
multiple entries from outside). If you thread through the latch, then
through an empty header and then through another non-empty basic block
within the loop, you still wouldn't be fine: you've just created code on
the back edge (and hence into the new latch block). If you thread through
the latch and through an empty header (and stop there), you also are fine.
Also note that in this situation you do _not_ create new entries into the
loop, not even intermediately. The old back edge is the one that goes
away due to the threading, the old entry edge is moved comletely out of
the loop, the edge header->thread-dest becomes the new entry edge, and the
edge new-latch->thread-dest becomes the back edge. No additional entries.
So, no, it's not the threading through the loop header that is problematic
but creating a situation that fills the (new) latch with code, and that
can only happen if the candidate thread contains the latch block.
(Of course it's somewhat related to the header block as well, because that
is simply the only destination the latch has, and hence is usually
included in any thread that also include the latch; but it's not the
header that indicates the situation).
> See tree-ssa-threadupdate.c:thread_block_1
>
> e2 = path->last ()->e;
> if (!e2 || noloop_only)
> {
> /* If NOLOOP_ONLY is true, we only allow threading through the
> header of a loop to exit edges. */
>
> /* One case occurs when there was loop header buried in a jump
> threading path that crosses loop boundaries. We do not try
> and thread this elsewhere, so just cancel the jump threading
> request by clearing the AUX field now. */
> if (bb->loop_father != e2->src->loop_father
> && (!loop_exit_edge_p (e2->src->loop_father, e2)
> || flow_loop_nested_p (bb->loop_father,
> e2->dest->loop_father)))
> {
> /* Since this case is not handled by our special code
> to thread through a loop header, we must explicitly
> cancel the threading request here. */
> delete_jump_thread_path (path);
> e->aux = NULL;
> continue;
> }
Yeah, sure, but I'm not sure if the above check is _really_ testing it
wants to test or if the effects it achieves are side effects. Like in my
proposed patch: I could also test for existence of loop header in the
thread and reject that; it would work as well, except that it works
because any useful thread including a latch (which is the problematic one)
also includes the header. I'm not sure if the above check is in the same
line, or tests for some still another situation.
Ciao,
Michael.
