Re: PR 11319 analyzed (Re: Patch fixing 3.3 bug PR 9745 and PR10021)

[Jim Wilson <wilson at tuliptree dot org>]

On Tue, 2003-07-15 at 12:52, Franz Sirl wrote:
> I looked at what goes wrong with PR 11319. In 11319 we have the following 
> short code fragment miscompiled:

I haven't had a chance to look at 11319 yet.  I will have to look into
that and comment later.  I suspect the situation is the same as I have
described in other messages, i.e. loop_regs_update corrupts the alias
info in an inner loop, causing an alias test in an outer loop to fail.

> - why is Dale's patch "hackish", except for not curing all possible cases?

The underlying problem is that the alias info is wrong.  Dale's patch
does not fix that.  It merely papers over it.  If we have MEM_EXPR, and
MEM_EXPR indicates aliasing, then we have an early exit out.  Otherwise,
we fall through and continue to execute the broken code.

Why only the checks in true_dependence and not also output and anti
dependence?  We probably should have a function overlapping_memrefs_p
similar to nonoverlapping_memrefs_p except checking for overlaps instead
of nonoverlaps, and call it everyplace where we call
nonoverlapping_memregs_p.

I do have to admit that Dale's patch is expedient.  We have the patch
now, and the patch should not cause performance regressions except in
cases where the code was probably wrong to begin with.  And the fact
that it is parallel to nonoverlapping_memrefs_p is something I missed
when first looking at the patch.  However, we still have a serious bug
even with Dale's patch, because the underlying alias info is still
wrong.

I do have a patch that tries to fix the underlying problem with the
alias info.  This is the alias.c patch in PR 10021.  Unfortunately, this
patch is blocked at the moment.  David Edelsohn claims that this patch
will cause major performance regressions.  As yet, no one has provided
any verifiable evidence of this claim.  It is hard to refute
unsubstantiated claims, and as yet I haven't tried.  The claim is
credible.  Because of the nature of the problem, it is impossible to
avoid performance regressions.  The problem is that the loop pass is
moving instructions out of loops when it is not safe to do so.  This
results in fast but sometimes buggy code.  If we replace that with the
conservatively correct test, then we get safely correct code, but
obviously less performance.  However, at this point, it isn't clear how
much of a performance loss we get with my patch.  In addition, I have
provided info explaining how to add new optimizations to get back some
of the performance loss, but as yet no one has looked into this.  I just
pointed Roger Sayle's at this info, maybe he will look into it.

> - why does move_movables() fiddle with single insns at all? Shouldn't it leave
>   that to the scheduler? (note that the insn isn't hoisted out of the loop)

We had move_movables about a decade before we had a cross-block
scheduler.  Even with cross-block scheduling, it still makes sense to do
instruction movement in the loop optimization pass, because it has info
about loops that the cross-block scheduler doesn't.

> - why does it move the insn to the beginning of the loop, even across a inner
>   loop?

I don't know.  That does seem like a strange thing to do.
-- 
Jim Wilson, GNU Tools Support, http://www.specifix.com