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

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

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