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
