| Message ID | CABu31nNFebS20WNCc4fzDzT4TQXtGSumQ9qKM2WsGcX7Qni_BQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
> 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.
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. */
> 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
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
> 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.
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
> >> 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...
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
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
> 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?
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)