Message ID | 54EB06C0.2080503@gjlay.de |
---|---|
State | New |
Headers | show |
On Mon, Feb 23, 2015 at 11:53:52AM +0100, Georg-Johann Lay wrote: > This patch fixes PR64331 which produced wrong code because of outdated (too > many) REG_DEAD notes. These notes are not (re)computed per default, hence > do the computation by hand each time avr.c:reg_unused_after is called in a > different pass. > > Ok to apply? > I was testing a similar patch a while back. Mine had a call to df_finish_pass after calling df_analyze, and I didn't "cache" the pass number. I remember running into ICEs building libgcc after that - from df_verify, IIRC. Do you know why df_finish_pass isn't needed in this case? AFAICT, all it appears to do is to clean up some stuff and enable a verification flag. Regards Senthil > Johann > > > gcc/ > PR target/64331 > * config/avr/avr.c (reg_unused_after): Recompute insn notes if the > pass changed since the last (re)computation. > * config/avr/avr.h (machine_function.dead_notes_pass_number): New > component. > > gcc/testsuite/ > PR target/64331 > * gcc.target/avr/torture/pr64331.c: New run test. > > Index: config/avr/avr.c > =================================================================== > --- config/avr/avr.c (revision 220738) > +++ config/avr/avr.c (working copy) > @@ -51,6 +51,7 @@ > #include "target-def.h" > #include "params.h" > #include "df.h" > +#include "tree-pass.h" /* for current_pass */ > > /* Maximal allowed offset for an address in the LD command */ > #define MAX_LD_OFFSET(MODE) (64 - (signed)GET_MODE_SIZE (MODE)) > @@ -7862,8 +7863,22 @@ avr_adjust_insn_length (rtx insn, int le > int > reg_unused_after (rtx insn, rtx reg) > { > + if (cfun->machine->dead_notes_pass_number > + != current_pass->static_pass_number) > + { > + /* `dead_or_set_p' needs correct REG_DEAD notes, cf. PR64331. Hence > + recompute them each time we find ourselves in a different pass. > + `reg_unused_after' is used during final for insn output, and in > + some earlier passes it is involved in insn length computations. */ > + > + df_note_add_problem (); > + df_analyze (); > + > + cfun->machine->dead_notes_pass_number = current_pass->static_pass_number; > + } > + > return (dead_or_set_p (insn, reg) > - || (REG_P(reg) && _reg_unused_after (insn, reg))); > + || (REG_P (reg) && _reg_unused_after (insn, reg))); > } > > /* Return nonzero if REG is not used after INSN. > Index: config/avr/avr.h > =================================================================== > --- config/avr/avr.h (revision 220738) > +++ config/avr/avr.h (working copy) > @@ -592,6 +592,10 @@ struct GTY(()) machine_function > /* 'true' if the above is_foo predicates are sanity-checked to avoid > multiple diagnose for the same function. */ > int attributes_checked_p; > + > + /* The latest static pass number for which REG_DEAD notes have been > + computed, cf. PR64331. */ > + int dead_notes_pass_number; > }; > > /* AVR does not round pushes, but the existence of this macro is > Index: testsuite/gcc.target/avr/torture/pr64331.c > =================================================================== > --- testsuite/gcc.target/avr/torture/pr64331.c (revision 0) > +++ testsuite/gcc.target/avr/torture/pr64331.c (revision 0) > @@ -0,0 +1,38 @@ > +/* { dg-do run } */ > + > +typedef struct > +{ > + unsigned a, b; > +} T2; > + > + > +__attribute__((__noinline__, __noclone__)) > +void foo2 (T2 *t, int x) > +{ > + if (x != t->a) > + { > + t->a = x; > + > + if (x && x == t->b) > + t->a = 20; > + } > +} > + > + > +T2 t; > + > +int main (void) > +{ > + t.a = 1; > + t.b = 1234; > + > + foo2 (&t, 1234); > + > + if (t.a != 20) > + __builtin_abort(); > + > + __builtin_exit (0); > + > + return 0; > +} > +
2015-02-23 13:53 GMT+03:00 Georg-Johann Lay <avr@gjlay.de>: > This patch fixes PR64331 which produced wrong code because of outdated (too > many) REG_DEAD notes. These notes are not (re)computed per default, hence > do the computation by hand each time avr.c:reg_unused_after is called in a > different pass. > > Ok to apply? > > Johann > > > gcc/ > PR target/64331 > * config/avr/avr.c (reg_unused_after): Recompute insn notes if the > pass changed since the last (re)computation. > * config/avr/avr.h (machine_function.dead_notes_pass_number): New > component. > > gcc/testsuite/ > PR target/64331 > * gcc.target/avr/torture/pr64331.c: New run test. > Approved. Please apply. Denis.
Index: config/avr/avr.c =================================================================== --- config/avr/avr.c (revision 220738) +++ config/avr/avr.c (working copy) @@ -51,6 +51,7 @@ #include "target-def.h" #include "params.h" #include "df.h" +#include "tree-pass.h" /* for current_pass */ /* Maximal allowed offset for an address in the LD command */ #define MAX_LD_OFFSET(MODE) (64 - (signed)GET_MODE_SIZE (MODE)) @@ -7862,8 +7863,22 @@ avr_adjust_insn_length (rtx insn, int le int reg_unused_after (rtx insn, rtx reg) { + if (cfun->machine->dead_notes_pass_number + != current_pass->static_pass_number) + { + /* `dead_or_set_p' needs correct REG_DEAD notes, cf. PR64331. Hence + recompute them each time we find ourselves in a different pass. + `reg_unused_after' is used during final for insn output, and in + some earlier passes it is involved in insn length computations. */ + + df_note_add_problem (); + df_analyze (); + + cfun->machine->dead_notes_pass_number = current_pass->static_pass_number; + } + return (dead_or_set_p (insn, reg) - || (REG_P(reg) && _reg_unused_after (insn, reg))); + || (REG_P (reg) && _reg_unused_after (insn, reg))); } /* Return nonzero if REG is not used after INSN. Index: config/avr/avr.h =================================================================== --- config/avr/avr.h (revision 220738) +++ config/avr/avr.h (working copy) @@ -592,6 +592,10 @@ struct GTY(()) machine_function /* 'true' if the above is_foo predicates are sanity-checked to avoid multiple diagnose for the same function. */ int attributes_checked_p; + + /* The latest static pass number for which REG_DEAD notes have been + computed, cf. PR64331. */ + int dead_notes_pass_number; }; /* AVR does not round pushes, but the existence of this macro is Index: testsuite/gcc.target/avr/torture/pr64331.c =================================================================== --- testsuite/gcc.target/avr/torture/pr64331.c (revision 0) +++ testsuite/gcc.target/avr/torture/pr64331.c (revision 0) @@ -0,0 +1,38 @@ +/* { dg-do run } */ + +typedef struct +{ + unsigned a, b; +} T2; + + +__attribute__((__noinline__, __noclone__)) +void foo2 (T2 *t, int x) +{ + if (x != t->a) + { + t->a = x; + + if (x && x == t->b) + t->a = 20; + } +} + + +T2 t; + +int main (void) +{ + t.a = 1; + t.b = 1234; + + foo2 (&t, 1234); + + if (t.a != 20) + __builtin_abort(); + + __builtin_exit (0); + + return 0; +} +