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

login
register
mail settings
Submitter Steven Bosscher
Date Nov. 28, 2012, 12:42 a.m.
Message ID <CABu31nNFebS20WNCc4fzDzT4TQXtGSumQ9qKM2WsGcX7Qni_BQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/202334/
State New
Headers show

Comments

Steven Bosscher - Nov. 28, 2012, 12:42 a.m.
On Wed, Nov 28, 2012 at 12:58 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Wed, Nov 28, 2012 at 12:53 AM, Steven Bosscher wrote:
>> On Wed, Nov 28, 2012 at 12:47 AM, Eric Botcazou wrote:
>>>> Count me in, too.  So let's avoid it:
>>>>
>>>>         * gcse.c (struct reg_use): Remove unused struct.
>>>>         (gcse_emit_move_after): Do not create REG_EQUAL notes that reference
>>>> the SET_DEST of the instruction the note would be attached to.
>>>
>>> OK (with PR rtl-optimization/55006) if it passes bootstrap & regtest, thanks.
>>
>> Thanks for the quick review.
>>
>>
>>>> And perhaps a bit in emit-rtl.c for good measure? I'll see if/where
>>>> this causes breakage...
>>>
>>> I think this would need to be wrapped in a #ifdef ENABLE_CHECKING/#endif pair.
>>
>> Well, let me first try to make it pass some set of tests. This breaks
>> the compiler in too many places to name right now. Here's the first of
>> what's probably going to be a whole series of patches if I keep
>> hitting this internal_error as often as I am now in my set of cc1-i
>> files...
>>
>> I know: ChangeLog, testing and all that. That's not the point yet.
>> This is just a brain dump, to get this saved in some places safer than
>> the compile farm or (worse) my head ;-)
>
> And another one:

And one to end the night here.

Again: Is this really the direction we should be going in? (I haven't
any better ideas...)
Eric Botcazou - Nov. 28, 2012, 7:44 a.m.
> Again: Is this really the direction we should be going in? (I haven't
> any better ideas...)

Do you mean for self-referencing notes?  If so, definitely, these notes are 
either wrong or almost always redundant so we should eliminate them.

As for the more general problem of REG_EQ* notes, perhaps it's time to devise 
a global infrastructure to handle them.  But, as far as I recall, they always 
have been problematic, before or after the DF merge.
Steven Bosscher - Nov. 28, 2012, 3:57 p.m.
On Wed, Nov 28, 2012 at 8:44 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Again: Is this really the direction we should be going in? (I haven't
>> any better ideas...)
>
> Do you mean for self-referencing notes?  If so, definitely, these notes are
> either wrong or almost always redundant so we should eliminate them.

Well, I'm not sure I agree that they are wrong. Consider:

S0: r1 = ...
S1: r2 = r1 + 10
S2: r1 = r2 + 10 { REG_EQUAL r1 + 20 }
S3: r3 = r1 + 10

Clearly it would be wrong to use the REG_EQUAL note on S2 to optimize S3 to:

S0: r1 = ...
S1: r2 = r1 + 10
S2: r1 = r1 + 20
S3: r3 = r1 + 30

However, It seems to me that this would be valid if e.g. the webizer
turns the above into:

S0: r1 = ...
S1: r2 = r1 + 10
S2: r4 = r2 + 10 { REG_EQUAL r1 + 20 }
S3: r3 = r4 + 10

because now the optimization would be valid:

S0: r1 = ...
S1: r2 = r1 + 10
S2: r4 = r1 + 20
S3: r3 = r1 + 30


So self-referencing REG_EQUAL notes should IMHO be considered valid,
and transformations using the notes should just be careful to
invalidate the equivalence before processing the insn that the note
appears on. cse.c doesn't appear to do this, however.


On a completely different note:

df-problems.c has this comment:

            /* Remove the notes that refer to dead registers.  As we
have at most
               one REG_EQUAL/EQUIV note, all of EQ_USES will refer to this note
               so we need to purge the complete EQ_USES vector when removing
               the note using df_notes_rescan.  */

But ira.c:update_equiv_regs creates insns with a REG_EQUAL *and* a
REG_EQUIV note:
(gdb)
update_equiv_regs () at ../../trunk/gcc/ira.c:3177
3177                    = gen_rtx_INSN_LIST (VOIDmode, insn, NULL_RTX);
2: debug_rtx((rtx)0x7ffff4df5e58) = (insn 638 637 639 85 (parallel [
            (set (reg:SI 891)
                (minus:SI (reg:SI 890)
                    (reg:SI 546 [ D.45472 ])))
            (clobber (reg:CC 17 flags))
        ]) ../../trunk/gcc/value-prof.c:928 283 {*subsi_1}
     (expr_list:REG_EQUIV (mem:SI (plus:DI (reg/v/f:DI 204 [ e12 ])
                (const_int 52 [0x34])) [4 e12_223->probability+0 S4 A32])
        (expr_list:REG_UNUSED (reg:CC 17 flags)
            (expr_list:REG_EQUAL (minus:SI (const_int 10000 [0x2710])
                    (reg:SI 546 [ D.45472 ]))
                (nil)))))
void
(gdb)

Is that valid?

Ciao!
Steven




> As for the more general problem of REG_EQ* notes, perhaps it's time to devise
> a global infrastructure to handle them.  But, as far as I recall, they always
> have been problematic, before or after the DF merge.

            /* Remove the notes that refer to dead registers.  As we
have at most
               one REG_EQUAL/EQUIV note, all of EQ_USES will refer to this note
               so we need to purge the complete EQ_USES vector when removing
               the note using df_notes_rescan.  */
Eric Botcazou - Nov. 28, 2012, 10:10 p.m.
> Well, I'm not sure I agree that they are wrong. Consider:
> 
> S0: r1 = ...
> S1: r2 = r1 + 10
> S2: r1 = r2 + 10 { REG_EQUAL r1 + 20 }
> S3: r3 = r1 + 10
> 
> Clearly it would be wrong to use the REG_EQUAL note on S2 to optimize S3 to:
> 
> S0: r1 = ...
> S1: r2 = r1 + 10
> S2: r1 = r1 + 20
> S3: r3 = r1 + 30

But the note is wrong by itself.  The semantics is clear: the note means that 
r1 is equal to r1 + 20 right after S2, which doesn't make any sense.

> However, It seems to me that this would be valid if e.g. the webizer
> turns the above into:
> 
> S0: r1 = ...
> S1: r2 = r1 + 10
> S2: r4 = r2 + 10 { REG_EQUAL r1 + 20 }
> S3: r3 = r4 + 10
> 
> because now the optimization would be valid:
> 
> S0: r1 = ...
> S1: r2 = r1 + 10
> S2: r4 = r1 + 20
> S3: r3 = r1 + 30

Sure, but we have no guarantee that the RTL stream will be massaged that way.

> So self-referencing REG_EQUAL notes should IMHO be considered valid,
> and transformations using the notes should just be careful to
> invalidate the equivalence before processing the insn that the note
> appears on. cse.c doesn't appear to do this, however.

IMO that's a recipe for bugs.
 
> On a completely different note:
> 
> df-problems.c has this comment:
> 
>             /* Remove the notes that refer to dead registers.  As we
> have at most
>                one REG_EQUAL/EQUIV note, all of EQ_USES will refer to this
> note so we need to purge the complete EQ_USES vector when removing the note
> using df_notes_rescan.  */
> 
> But ira.c:update_equiv_regs creates insns with a REG_EQUAL *and* a
> REG_EQUIV note:
> (gdb)
> update_equiv_regs () at ../../trunk/gcc/ira.c:3177
> 3177                    = gen_rtx_INSN_LIST (VOIDmode, insn, NULL_RTX);
> 2: debug_rtx((rtx)0x7ffff4df5e58) = (insn 638 637 639 85 (parallel [
>             (set (reg:SI 891)
>                 (minus:SI (reg:SI 890)
>                     (reg:SI 546 [ D.45472 ])))
>             (clobber (reg:CC 17 flags))
>         ]) ../../trunk/gcc/value-prof.c:928 283 {*subsi_1}
>      (expr_list:REG_EQUIV (mem:SI (plus:DI (reg/v/f:DI 204 [ e12 ])
>                 (const_int 52 [0x34])) [4 e12_223->probability+0 S4 A32])
>         (expr_list:REG_UNUSED (reg:CC 17 flags)
>             (expr_list:REG_EQUAL (minus:SI (const_int 10000 [0x2710])
>                     (reg:SI 546 [ D.45472 ]))
>                 (nil)))))
> void
> (gdb)
> 
> Is that valid?

Yes, the comment is wrong and should have been removed in r183719:
  http://gcc.gnu.org/ml/gcc-patches/2012-01/msg01547.html
Steven Bosscher - Nov. 28, 2012, 11:53 p.m.
On Wed, Nov 28, 2012 at 11:10 PM, Eric Botcazou wrote:
>> Well, I'm not sure I agree that they are wrong. Consider:
>>
>> S0: r1 = ...
>> S1: r2 = r1 + 10
>> S2: r1 = r2 + 10 { REG_EQUAL r1 + 20 }
>> S3: r3 = r1 + 10
>>
>> Clearly it would be wrong to use the REG_EQUAL note on S2 to optimize S3 to:
>>
>> S0: r1 = ...
>> S1: r2 = r1 + 10
>> S2: r1 = r1 + 20
>> S3: r3 = r1 + 30
>
> But the note is wrong by itself.  The semantics is clear: the note means that
> r1 is equal to r1 + 20 right after S2, which doesn't make any sense.

Or maybe the semantics are not what the compiler is actually doing.
Because clearly several places in the compiler can create this kind of
self-referencing note right now, and the main consumer of the notes,
CSE, has apparently not had too much trouble handing them.

But with the documented semantics, you're right. And, to be honest,
I'm of the "the fewer notes, the better" camp :-)


>> 2: debug_rtx((rtx)0x7ffff4df5e58) = (insn 638 637 639 85 (parallel [
>>             (set (reg:SI 891)
>>                 (minus:SI (reg:SI 890)
>>                     (reg:SI 546 [ D.45472 ])))
>>             (clobber (reg:CC 17 flags))
>>         ]) ../../trunk/gcc/value-prof.c:928 283 {*subsi_1}
>>      (expr_list:REG_EQUIV (mem:SI (plus:DI (reg/v/f:DI 204 [ e12 ])
>>                 (const_int 52 [0x34])) [4 e12_223->probability+0 S4 A32])
>>         (expr_list:REG_UNUSED (reg:CC 17 flags)
>>             (expr_list:REG_EQUAL (minus:SI (const_int 10000 [0x2710])
>>                     (reg:SI 546 [ D.45472 ]))
>>                 (nil)))))
>> void
>> (gdb)
>>
>> Is that valid?
>
> Yes, the comment is wrong and should have been removed in r183719:
>   http://gcc.gnu.org/ml/gcc-patches/2012-01/msg01547.html

Ugh, that doesn't look like a very solid fix.

Attached is a different fix. It splits DF_REF_IN_NOTE in two: One flag
for each kind of note. This allows the dead note removal code to
distinguish the source note for the EQ_USES.  I needed to remove one
flag to keep the df_ref_flags 16-bit, but the DF_REF_SUBREG flag looks
completely unnecessary to me, and only ira.c uses it, so it wasn't to
hard to scavenge a single bit.


The patch also includes all places I've found so far where the
compiler could create self-referencing notes:

1. optabs.c: Not sure what it was trying to do, but now it just
refuses to add a note if TARGET is mentioned in one of the source
operands.

2. gcse.c: gcse_emit_move_after added notes, but none of them was very
useful as far as I could tell, and almost all of them turned
self-referencing after CPROP. So I propose we just never add notes in
this case.

3. cprop.c: It seems to me that the purpose here is to propagate
constants. If a reg could not be propagated, then the REG_EQUAL note
will not help much either. Propagating constants via REG_EQUAL notes
still helps folding comparisons sometimes, so I'm proposing we only
propagate those. As a bonus: less garbage RTL because a
cprop_constant_p can be shared.

4. fwprop.c: If the SET_DEST is a REG that is mentioned in the
SET_SRC, don' create a note. This one I'm not very happy with, because
it doesn't handle the case that the REG is somehow simplified out of
the SET_SRC, but being smarter about this would only complicate
things. I for one can't think of anything better for the moment,
anyway.


Finally, it makes sense to compute the NOTE problem for CSE.

Bootstrap&testing in progress at the older revision I've been using to
debug the darwin10 and sparc bugs. I'll test trunk head on x86_64 and
powerpc later. In the mean time, let me hear what you think of this
one :-)

Ciao!
Steven
Eric Botcazou - Dec. 1, 2012, 2:54 p.m.
> Attached is a different fix. It splits DF_REF_IN_NOTE in two: One flag
> for each kind of note. This allows the dead note removal code to
> distinguish the source note for the EQ_USES.  I needed to remove one
> flag to keep the df_ref_flags 16-bit, but the DF_REF_SUBREG flag looks
> completely unnecessary to me, and only ira.c uses it, so it wasn't to
> hard to scavenge a single bit.

I'll defer this to Paolo.

> The patch also includes all places I've found so far where the
> compiler could create self-referencing notes:
> 
> 1. optabs.c: Not sure what it was trying to do, but now it just
> refuses to add a note if TARGET is mentioned in one of the source
> operands.

OK.

> 2. gcse.c: gcse_emit_move_after added notes, but none of them was very
> useful as far as I could tell, and almost all of them turned
> self-referencing after CPROP. So I propose we just never add notes in
> this case.

gcse_emit_move_after also preserves existing notes.  Are they problematic?

> 3. cprop.c: It seems to me that the purpose here is to propagate
> constants. If a reg could not be propagated, then the REG_EQUAL note
> will not help much either. Propagating constants via REG_EQUAL notes
> still helps folding comparisons sometimes, so I'm proposing we only
> propagate those. As a bonus: less garbage RTL because a
> cprop_constant_p can be shared.

That seems a bit radical to me, especially in try_replace_reg which is used 
for copy propagation as well.  In cprop_jump, why is attaching a note to the 
jump problematic?

> 4. fwprop.c: If the SET_DEST is a REG that is mentioned in the
> SET_SRC, don' create a note. This one I'm not very happy with, because
> it doesn't handle the case that the REG is somehow simplified out of
> the SET_SRC, but being smarter about this would only complicate
> things. I for one can't think of anything better for the moment,
> anyway.

OK.

> Finally, it makes sense to compute the NOTE problem for CSE.

Why?  It only uses REG_EQ* notes, so it seems like we're back to the earlier 
trick of using df_note_add_problem to clean up pre-existing REG_EQ* notes.
Steven Bosscher - Dec. 1, 2012, 4:44 p.m.
On Sat, Dec 1, 2012 at 3:54 PM, Eric Botcazou wrote:
>> The patch also includes all places I've found so far where the
>> compiler could create self-referencing notes:
>>
>> 1. optabs.c: Not sure what it was trying to do, but now it just
>> refuses to add a note if TARGET is mentioned in one of the source
>> operands.
>
> OK.

Thanks. I'll commit this separately.


>> 2. gcse.c: gcse_emit_move_after added notes, but none of them was very
>> useful as far as I could tell, and almost all of them turned
>> self-referencing after CPROP. So I propose we just never add notes in
>> this case.
>
> gcse_emit_move_after also preserves existing notes.  Are they problematic?

Yes, they tend to be invalid after PRE because the registers used in
the PRE'd expression usually are not live anymore (making the note
invalid). Sometimes CPROP "re-validates" the notes, but it doesn't
seem wise to me to rely on that.


>> 3. cprop.c: It seems to me that the purpose here is to propagate
>> constants. If a reg could not be propagated, then the REG_EQUAL note
>> will not help much either. Propagating constants via REG_EQUAL notes
>> still helps folding comparisons sometimes, so I'm proposing we only
>> propagate those. As a bonus: less garbage RTL because a
>> cprop_constant_p can be shared.
>
> That seems a bit radical to me, especially in try_replace_reg which is used
> for copy propagation as well.  In cprop_jump, why is attaching a note to the
> jump problematic?

Most of the time a note from copy-propagation was not valid because
the copy-prop'd reg was not live at the point of the note.


>> 4. fwprop.c: If the SET_DEST is a REG that is mentioned in the
>> SET_SRC, don' create a note. This one I'm not very happy with, because
>> it doesn't handle the case that the REG is somehow simplified out of
>> the SET_SRC, but being smarter about this would only complicate
>> things. I for one can't think of anything better for the moment,
>> anyway.
>
> OK.

I'll commit this along with the optabs.c part.


>> Finally, it makes sense to compute the NOTE problem for CSE.
>
> Why?  It only uses REG_EQ* notes,

Not really. It uses single_set in a few places, including
delete_trivially_dead_insns and cse_extended_basic_block.


> so it seems like we're back to the earlier
> trick of using df_note_add_problem to clean up pre-existing REG_EQ* notes.

Again: Not really. I also bootstrapped&tested without the cse.c change.

I plan (and promise ;-) to add a REG_EQ* note verifier for GCC 4.9.

Ciao!
Steven
Eric Botcazou - Dec. 3, 2012, 6:23 p.m.
> >> 2. gcse.c: gcse_emit_move_after added notes, but none of them was very
> >> useful as far as I could tell, and almost all of them turned
> >> self-referencing after CPROP. So I propose we just never add notes in
> >> this case.
> > 
> > gcse_emit_move_after also preserves existing notes.  Are they
> > problematic?
> Yes, they tend to be invalid after PRE because the registers used in
> the PRE'd expression usually are not live anymore (making the note
> invalid). Sometimes CPROP "re-validates" the notes, but it doesn't
> seem wise to me to rely on that.

So the compiler doesn't bootstrap with the gcse.c patch you posted earlier in 
the thread?  Still this seems too bold to me, the note datum could be a 
constant and should be preserved in this case.

> >> 3. cprop.c: It seems to me that the purpose here is to propagate
> >> constants. If a reg could not be propagated, then the REG_EQUAL note
> >> will not help much either. Propagating constants via REG_EQUAL notes
> >> still helps folding comparisons sometimes, so I'm proposing we only
> >> propagate those. As a bonus: less garbage RTL because a
> >> cprop_constant_p can be shared.
> > 
> > That seems a bit radical to me, especially in try_replace_reg which is
> > used for copy propagation as well.  In cprop_jump, why is attaching a
> > note to the jump problematic?
> 
> Most of the time a note from copy-propagation was not valid because
> the copy-prop'd reg was not live at the point of the note.

This one I think we should drop for now, or just avoid the self-referential 
case.  There is a comment explicitly saying that the REG_EQUAL note added by 
try_replace_reg are part of the algorithm.

> Not really. It uses single_set in a few places, including
> delete_trivially_dead_insns and cse_extended_basic_block.
> 
> > so it seems like we're back to the earlier
> > trick of using df_note_add_problem to clean up pre-existing REG_EQ*
> > notes.
> Again: Not really. I also bootstrapped&tested without the cse.c change.

The cse.c hunk is OK then.

> I plan (and promise ;-) to add a REG_EQ* note verifier for GCC 4.9.

Thanks (no need to promise though :-), that will be helpful.  In the meantime, 
I don't think that we should aim for perfection in 4.8, these REG_EQ* notes 
and their quirks have been with us for a long time...
Paolo Bonzini - Dec. 3, 2012, 8:15 p.m.
Il 01/12/2012 15:54, Eric Botcazou ha scritto:
>> Attached is a different fix. It splits DF_REF_IN_NOTE in two: One flag
>> > for each kind of note. This allows the dead note removal code to
>> > distinguish the source note for the EQ_USES.  I needed to remove one
>> > flag to keep the df_ref_flags 16-bit, but the DF_REF_SUBREG flag looks
>> > completely unnecessary to me, and only ira.c uses it, so it wasn't to
>> > hard to scavenge a single bit.
> I'll defer this to Paolo.

Ok, I'll review this tomorrow.

Paolo
Steven Bosscher - Dec. 3, 2012, 8:19 p.m.
On Mon, Dec 3, 2012 at 7:23 PM, Eric Botcazou wrote:
>> >> 2. gcse.c: gcse_emit_move_after added notes, but none of them was very
>> >> useful as far as I could tell, and almost all of them turned
>> >> self-referencing after CPROP. So I propose we just never add notes in
>> >> this case.
>> >
>> > gcse_emit_move_after also preserves existing notes.  Are they
>> > problematic?
>> Yes, they tend to be invalid after PRE because the registers used in
>> the PRE'd expression usually are not live anymore (making the note
>> invalid). Sometimes CPROP "re-validates" the notes, but it doesn't
>> seem wise to me to rely on that.
>
> So the compiler doesn't bootstrap with the gcse.c patch you posted earlier in
> the thread?  Still this seems too bold to me, the note datum could be a
> constant and should be preserved in this case.

You mean the patch at
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg02275.html right?

I haven't tried that other patch. I'll test that one.


>> >> 3. cprop.c: It seems to me that the purpose here is to propagate
>> >> constants. If a reg could not be propagated, then the REG_EQUAL note
>> >> will not help much either. Propagating constants via REG_EQUAL notes
>> >> still helps folding comparisons sometimes, so I'm proposing we only
>> >> propagate those. As a bonus: less garbage RTL because a
>> >> cprop_constant_p can be shared.
>> >
>> > That seems a bit radical to me, especially in try_replace_reg which is
>> > used for copy propagation as well.  In cprop_jump, why is attaching a
>> > note to the jump problematic?
>>
>> Most of the time a note from copy-propagation was not valid because
>> the copy-prop'd reg was not live at the point of the note.
>
> This one I think we should drop for now, or just avoid the self-referential
> case.  There is a comment explicitly saying that the REG_EQUAL note added by
> try_replace_reg are part of the algorithm.

I suppose so. But this was all added before RTL fwprop and way before
GIMPLE optimizations. Avoiding the self-referential case is just more
difficult to do, quite expensive (have to scan the SET_SRC pattern),
and AFAICT doesn't bring much pay-off.

I'll prepare something to avoid the self-referential case, but I think
we're making our lives complicated for no good reason.


>> Not really. It uses single_set in a few places, including
>> delete_trivially_dead_insns and cse_extended_basic_block.
>>
>> > so it seems like we're back to the earlier
>> > trick of using df_note_add_problem to clean up pre-existing REG_EQ*
>> > notes.
>> Again: Not really. I also bootstrapped&tested without the cse.c change.
>
> The cse.c hunk is OK then.

Thanks, I'll commit it separately.


>> I plan (and promise ;-) to add a REG_EQ* note verifier for GCC 4.9.
>
> Thanks (no need to promise though :-), that will be helpful.  In the meantime,
> I don't think that we should aim for perfection in 4.8, these REG_EQ* notes
> and their quirks have been with us for a long time...

Well, yes they've been with us for a long time, but my LR_RD change
exposed all these problems that were simply hidden before. I think
we're safe for GCC 4.8 but I don't feel comfortable about this
situation...

Ciao!
Steven
Eric Botcazou - Dec. 3, 2012, 9:10 p.m.
> You mean the patch at
> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg02275.html right?
> 
> I haven't tried that other patch. I'll test that one.

Yes, thanks.

> I suppose so. But this was all added before RTL fwprop and way before
> GIMPLE optimizations. Avoiding the self-referential case is just more
> difficult to do, quite expensive (have to scan the SET_SRC pattern),
> and AFAICT doesn't bring much pay-off.
> 
> I'll prepare something to avoid the self-referential case, but I think
> we're making our lives complicated for no good reason.

Can't you just use the same best-effort approach used for fwprop.c here?

Patch

Index: fwprop.c
===================================================================
--- fwprop.c    (revision 193394)
+++ fwprop.c    (working copy)
@@ -1321,7 +1321,9 @@  forward_propagate_and_simplify (df_ref u
         separately try plugging the definition in the note and simplifying.
         And only install a REQ_EQUAL note when the destination is a REG,
         as the note would be invalid otherwise.  */
-      set_reg_equal = (note == NULL_RTX && REG_P (SET_DEST (use_set)));
+      set_reg_equal = (note == NULL_RTX && REG_P (SET_DEST (use_set))
+                      && ! reg_mentioned_p (SET_DEST (use_set),
+                                            SET_SRC (use_set)));
     }

   if (GET_MODE (*loc) == VOIDmode)