Re: Fix twolf -funroll-loops -O3 miscompilation (a semi-latent web.c bug)

[Steven Bosscher <stevenb dot gcc at gmail dot com>]

On Tue, Nov 27, 2012 at 10:55 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Or rather this one. Same hammer, different color. It turns out that
>> the rtlanal.c change caused problems, so I've got to use a home-brewn
>> equivalent of remove_reg_equal_equiv_notes_for_regno...
>>
>> Test case is unchanged so I'm omitting it here.
>>
>> Ciao!
>> Steven
>>
>>
>> gcc/
>>       * loop-unroll.c (struct iv_to_split): Add new 'orig_var' member.
>>       (analyze_iv_to_split_insn): Record it.
>>       (maybe_strip_eq_note_for_split_iv): New function to remove REG_EQUAL
>>       notes that refer to IVs that are being split.
>>       (apply_opt_in_copies): Use maybe_strip_eq_note_for_split_iv.  Twice.
>>       Use FOR_BB_INSNS_SAFE.
>
> That's fine with me, thanks.  You might want to defer applying it until the
> reason why it isn't apparently sufficient for aermod.f90 is understood though.

Thanks. Yes, I'm first going to try and figure out why this doesn't
fix aermod for Dominique. I suspect the problem is in variable
expansion in the unroller. A previous patch of mine updates REG_EQUAL
notes in variable expansion, but it doesn't clean up notes that refer
to maybe dead registers.

I have to say, I've got a uncomfortable feeling about REG_EQUAL notes
not being allowed to refer to dead registers. In the case at hand, the
code basically does:

S1: r1 = r2 + 0x4  // r2 is the reg from split_iv, r1 was the original IV
S2: r3 = mem[r1]
S3: if ... goto L1;
S4: r4 = r3 // REG_EQUAL mem[r1]
L1:
S5: r1 = r2 + 0x8

At S4, r1 is not live in the usual meaning of register liveness, but
the DEF from S1 still reaches the REG_EQUAL note. This situation is
not only created by loop unrolling. At least gcse.c (PRE),
loop-invariant.c, cse.c, dce.c and combine.c can create situations
like the above. ifcvt.c jumps through hoops to avoid it (see e.g.
PR46315, which you fixed).

Most of the problems are papered over by occasional calls to
df_note_add_problem from some passes, so that df_remove_dead_eq_notes
cleans up any notes referencing dead registers. But if we really want
to declare this kind of REG_EQUAL note reference to a dead register
invalid RTL (which is something at least you, Paolo, and me agree on),
and we expect passes to clean up their own mess by either updating or
removing any notes they invalidate, then df_remove_dead_eq_notes
shouldn't be necessary for correctness because all notes it encounters
should be valid (being updated by passes).

Removing df_remove_dead_eq_notes breaks bootstrap on the four targets
I tried it on (x86_64, powerpc64, ia64, and hppa). That is, breaks
*without* -funroll-loops, and *without* -fweb. With a hack to disable
pass_initialize_regs, bootstrap still fails, but other files are
miscompiled. With another hack on top to disable CSE2, bootstrap still
fails, and yet other files are miscompiled.

What I'm really trying to say, is that even when I fix this particular
issue with aermod (which is apparently 3 issues: the one I fixed last
month for x86_64, the one fixed with the patch in this thread for
SPARC, and the yet-to-be-identified problem Dominique is still seeing
on darwin10) then it's likely other, similar bugs will show up. Bugs
that are hard to reproduce, dependent on the target's RTL, and
difficult to debug as they are usually wrong-code bugs on larger test
cases.

We really need a more robust solution. I've added Jeff and rth to the
CC, looking for opinions/thoughts/suggestions/$0.02s.

Ciao!
Steven