This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


> >> 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...

-- 
Eric Botcazou


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]