| Message ID | 54EF7591.3030900@gjlay.de |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 26, 2015 at 8:35 PM, Georg-Johann Lay wrote: > Take #2 introduces a new, avr-specific rtl pass whose sole purpose is to > rectify notes. The pass is scheduled right before cfg does down (right > before .*free_cfg) so that cfg and hence df machinery is available. > > Regression tests look fine and for the test case the patches produce correct > code and correct insn length. Sorry for party-pooping, but it seems to me that the real bug is that the target is lying to reload. IIUC the AVR port selects the insn alternative after register allocation (after reload). It bases its selection on REG_DEAD notes. In PR64331 an alternative is used that clobbers r20 that has a REG_DEAD note, but r20 is not actually dead because hardreg-cprop has propagated it forward without adjusting the note. The "normal" way of things is that the insn alternative is selected in reload (or in LRA) and that the clobbers are added as necessary. In PR64331, an alternative for insn r17 would be selected that has a CLOBBER for r20, prevent hardreg-cprop from propagating r20. Selecting insns based on REG-notes is dangerous business. Lying to reload and to post-RA passes is a mortal sin, the compiler will punish you. There is no guarantee that nothing will change between your new pass to recompute notes, and the final pass that emits the insns. It's not my port, for sure, but I would look for a real fix instead: Don't select insns to output based on unreliable information like REG-notes. Ciao! Steven
2015-02-27 1:45 GMT+03:00 Steven Bosscher <stevenb.gcc@gmail.com>: > On Thu, Feb 26, 2015 at 8:35 PM, Georg-Johann Lay wrote: >> Take #2 introduces a new, avr-specific rtl pass whose sole purpose is to >> rectify notes. The pass is scheduled right before cfg does down (right >> before .*free_cfg) so that cfg and hence df machinery is available. >> >> Regression tests look fine and for the test case the patches produce correct >> code and correct insn length. > > Sorry for party-pooping, but it seems to me that the real bug is that > the target is lying to reload. > > IIUC the AVR port selects the insn alternative after register > allocation (after reload). It bases its selection on REG_DEAD notes. > In PR64331 an alternative is used that clobbers r20 that has a > REG_DEAD note, but r20 is not actually dead because hardreg-cprop has > propagated it forward without adjusting the note. > > The "normal" way of things is that the insn alternative is selected in > reload (or in LRA) and that the clobbers are added as necessary. In > PR64331, an alternative for insn r17 would be selected that has a > CLOBBER for r20, prevent hardreg-cprop from propagating r20. > > Selecting insns based on REG-notes is dangerous business. Lying to > reload and to post-RA passes is a mortal sin, the compiler will punish > you. There is no guarantee that nothing will change between your new > pass to recompute notes, and the final pass that emits the insns. > > It's not my port, for sure, but I would look for a real fix instead: > Don't select insns to output based on unreliable information like > REG-notes. Steven rights. We will have an endless fight with this problem. Better to completely drop `reg_unused_after'. (I know that it used around 40 times in port) What do you think Georg ? Denis.
Am 02/28/2015 um 09:02 AM schrieb Denis Chertykov: > 2015-02-27 1:45 GMT+03:00 Steven Bosscher <stevenb.gcc@gmail.com>: >> On Thu, Feb 26, 2015 at 8:35 PM, Georg-Johann Lay wrote: >>> Take #2 introduces a new, avr-specific rtl pass whose sole purpose is to >>> rectify notes. The pass is scheduled right before cfg does down (right >>> before .*free_cfg) so that cfg and hence df machinery is available. >>> >>> Regression tests look fine and for the test case the patches produce correct >>> code and correct insn length. >> >> Sorry for party-pooping, but it seems to me that the real bug is that >> the target is lying to reload. >> >> IIUC the AVR port selects the insn alternative after register >> allocation (after reload). It bases its selection on REG_DEAD notes. >> In PR64331 an alternative is used that clobbers r20 that has a >> REG_DEAD note, but r20 is not actually dead because hardreg-cprop has >> propagated it forward without adjusting the note. >> >> The "normal" way of things is that the insn alternative is selected in >> reload (or in LRA) and that the clobbers are added as necessary. In >> PR64331, an alternative for insn r17 would be selected that has a >> CLOBBER for r20, prevent hardreg-cprop from propagating r20. >> >> Selecting insns based on REG-notes is dangerous business. Lying to >> reload and to post-RA passes is a mortal sin, the compiler will punish >> you. There is no guarantee that nothing will change between your new >> pass to recompute notes, and the final pass that emits the insns. >> >> It's not my port, for sure, but I would look for a real fix instead: >> Don't select insns to output based on unreliable information like >> REG-notes. > > Steven rights. > We will have an endless fight with this problem. Just consider the patch as prerequisite for further changes atop of it as outlined in https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01745.html The current patches are operating correctly and do nothing wrong. Complete rewriting of reg_unused_after will be much more work and more error prone (e.g. unrecognizable insn, optimization flaws). > Better to completely drop `reg_unused_after'. (I know that it used > around 40 times in port) > > What do you think Georg ? I'd prefer to fix it, c.f. link above. Johann > > Denis. >
2015-03-02 15:32 GMT+03:00 Georg-Johann Lay <avr@gjlay.de>: > Am 02/28/2015 um 09:02 AM schrieb Denis Chertykov: > >> 2015-02-27 1:45 GMT+03:00 Steven Bosscher <stevenb.gcc@gmail.com>: >>> >>> On Thu, Feb 26, 2015 at 8:35 PM, Georg-Johann Lay wrote: >>>> >>>> Take #2 introduces a new, avr-specific rtl pass whose sole purpose is to >>>> rectify notes. The pass is scheduled right before cfg does down (right >>>> before .*free_cfg) so that cfg and hence df machinery is available. >>>> >>>> Regression tests look fine and for the test case the patches produce >>>> correct >>>> code and correct insn length. >>> >>> >>> Sorry for party-pooping, but it seems to me that the real bug is that >>> the target is lying to reload. >>> >>> IIUC the AVR port selects the insn alternative after register >>> allocation (after reload). It bases its selection on REG_DEAD notes. >>> In PR64331 an alternative is used that clobbers r20 that has a >>> REG_DEAD note, but r20 is not actually dead because hardreg-cprop has >>> propagated it forward without adjusting the note. >>> >>> The "normal" way of things is that the insn alternative is selected in >>> reload (or in LRA) and that the clobbers are added as necessary. In >>> PR64331, an alternative for insn r17 would be selected that has a >>> CLOBBER for r20, prevent hardreg-cprop from propagating r20. >>> >>> Selecting insns based on REG-notes is dangerous business. Lying to >>> reload and to post-RA passes is a mortal sin, the compiler will punish >>> you. There is no guarantee that nothing will change between your new >>> pass to recompute notes, and the final pass that emits the insns. >>> >>> It's not my port, for sure, but I would look for a real fix instead: >>> Don't select insns to output based on unreliable information like >>> REG-notes. >> >> >> Steven rights. >> We will have an endless fight with this problem. > > > Just consider the patch as prerequisite for further changes atop of it as > outlined in > > https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01745.html > > The current patches are operating correctly and do nothing wrong. Complete > rewriting of reg_unused_after will be much more work and more error prone > (e.g. unrecognizable insn, optimization flaws). > > >> Better to completely drop `reg_unused_after'. (I know that it used >> around 40 times in port) >> >> What do you think Georg ? > > > I'd prefer to fix it, c.f. link above. Ok. Please fix it. Denis. PS: As I remember: I have copyed `reg_unused_after' from sparc port. It was around 10 years ago. `reg_unused_after' was removed from sparc a few years ago. So, only avr and sh ports use `reg_unused_after'. It's dangerous because GCC core developers don't bothered about two small ports. (Ports with relatively small user base) At least we can remove `reg_unused_after' in any time. (It's relatively easy.)
Index: config/avr/avr.c =================================================================== --- config/avr/avr.c (revision 220963) +++ config/avr/avr.c (working copy) @@ -51,6 +51,8 @@ #include "target-def.h" #include "params.h" #include "df.h" +#include "context.h" +#include "tree-pass.h" /* Maximal allowed offset for an address in the LD command */ #define MAX_LD_OFFSET(MODE) (64 - (signed)GET_MODE_SIZE (MODE)) @@ -285,6 +287,58 @@ avr_to_int_mode (rtx x) } +static const pass_data avr_pass_data_recompute_notes = +{ + RTL_PASS, // type + "", // name (will be patched) + OPTGROUP_NONE, // optinfo_flags + false, // has_gate + true, // has_execute + TV_DF_SCAN, // tv_id + 0, // properties_required + 0, // properties_provided + 0, // properties_destroyed + 0, // todo_flags_start + // todo_flags_finish + TODO_df_finish | TODO_verify_rtl_sharing | TODO_verify_flow +}; + + +class avr_pass_recompute_notes : public rtl_opt_pass +{ +public: + avr_pass_recompute_notes (gcc::context *ctxt, const char *name) + : rtl_opt_pass (avr_pass_data_recompute_notes, ctxt) + { + this->name = name; + } + + unsigned int execute (void) + { + df_note_add_problem (); + df_analyze (); + + return 0; + } +}; // avr_pass_recompute_notes + + +static void +avr_register_passes (void) +{ + /* This avr-specific pass (re)computes insn notes, in particular REG_DEAD + notes which are used by `avr.c::reg_unused_after' and branch offset + computations. These notes must be correct, i.e. there must be no + dangling REG_DEAD notes; otherwise wrong code might result, cf. PR64331. + + DF needs (correct) CFG, hence right before free_cfg is the last + opportunity to rectify notes. */ + + register_pass (new avr_pass_recompute_notes (g, "avr-notes-free-cfg"), + PASS_POS_INSERT_BEFORE, "*free_cfg", 1); +} + + /* Implement `TARGET_OPTION_OVERRIDE'. */ static void @@ -346,6 +400,11 @@ avr_option_override (void) init_machine_status = avr_init_machine_status; avr_log_set_avr_log(); + + /* Register some avr-specific pass(es). There is no canonical place for + pass registration. This function is convenient. */ + + avr_register_passes (); } /* Function to set up the backend function structure. */ 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,37 @@ +/* { 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; +}