| Message ID | cfe8fd2f-a9b8-7a6a-9b6a-4506f52a6a4f@gjlay.de |
|---|---|
| State | New |
| Headers | show |
On Mon, Jan 02, 2017 at 03:47:43PM +0100, Georg-Johann Lay wrote: > This fixes PR78883 which is a problem in reload revealed by a > change to combine.c. The fix is as proposed by Segher: implement > CANNOT_CHANGE_MODE_CLASS. > > Ok for trunk? > > Johann > > > gcc/ > PR target/78883 > * config/avr/avr.h (CANNOT_CHANGE_MODE_CLASS): New define. > * config/avr/avr-protos.h (avr_cannot_change_mode_class): New proto. > * config/avr/avr.c (avr_cannot_change_mode_class): New function. > > gcc/testsuite/ > PR target/78883 > * gcc.c-torture/compile/pr78883.c: New test. > Index: config/avr/avr-protos.h > =================================================================== > --- config/avr/avr-protos.h (revision 244001) > +++ config/avr/avr-protos.h (working copy) > @@ -111,7 +111,7 @@ extern int _reg_unused_after (rtx_insn * > extern int avr_jump_mode (rtx x, rtx_insn *insn); > extern int test_hard_reg_class (enum reg_class rclass, rtx x); > extern int jump_over_one_insn_p (rtx_insn *insn, rtx dest); > - > +extern int avr_cannot_change_mode_class (machine_mode, machine_mode, enum reg_class); > extern int avr_hard_regno_mode_ok (int regno, machine_mode mode); > extern void avr_final_prescan_insn (rtx_insn *insn, rtx *operand, > int num_operands); > Index: config/avr/avr.c > =================================================================== > --- config/avr/avr.c (revision 244001) > +++ config/avr/avr.c (working copy) > @@ -11833,6 +11833,21 @@ jump_over_one_insn_p (rtx_insn *insn, rt > } > > > +/* Worker function for `CANNOT_CHANGE_MODE_CLASS'. */ > + > +int > +avr_cannot_change_mode_class (machine_mode from, machine_mode to, > + enum reg_class /* rclass */) > +{ > + /* We cannot access a hard register in a wider mode, for example we > + must not access (reg:QI 31) as (reg:HI 31). HARD_REGNO_MODE_OK > + would avoid such hard regs, but reload would generate it anyway > + from paradoxical subregs of mem, cf. PR78883. */ > + > + return GET_MODE_SIZE (to) > GET_MODE_SIZE (from); I understand how this fixes the ICE, but is it really necessary to suppress conversions to a wider mode for lower numbered registers? > +} > + > + > /* Worker function for `HARD_REGNO_MODE_OK'. */ > /* Returns 1 if a value of mode MODE can be stored starting with hard > register number REGNO. On the enhanced core, anything larger than > Index: config/avr/avr.h > =================================================================== > --- config/avr/avr.h (revision 244001) > +++ config/avr/avr.h (working copy) > @@ -216,6 +216,9 @@ These two properties are reflected by bu > > #define MODES_TIEABLE_P(MODE1, MODE2) 1 > > +#define CANNOT_CHANGE_MODE_CLASS(MFROM, MTO, RCLASS) \ > + avr_cannot_change_mode_class (MFROM, MTO, RCLASS) > + > enum reg_class { > NO_REGS, > R0_REG, /* r0 */ > Index: testsuite/gcc.c-torture/compile/pr78883.c > =================================================================== > --- testsuite/gcc.c-torture/compile/pr78883.c (nonexistent) > +++ testsuite/gcc.c-torture/compile/pr78883.c (working copy) > @@ -0,0 +1,12 @@ > +/* { dg-do compile } */ > + > +int foo (int *p) > +{ > + int i; > + for (i = 0; i < 5; i++) > + { > + if (p[i] & 1) > + return i; > + } > + return -1; > +} Ciao Dominik ^_^ ^_^
On 02.01.2017 15:54, Dominik Vogt wrote: > On Mon, Jan 02, 2017 at 03:47:43PM +0100, Georg-Johann Lay wrote: >> This fixes PR78883 which is a problem in reload revealed by a >> change to combine.c. The fix is as proposed by Segher: implement >> CANNOT_CHANGE_MODE_CLASS. >> >> Ok for trunk? >> >> Johann >> >> >> gcc/ >> PR target/78883 >> * config/avr/avr.h (CANNOT_CHANGE_MODE_CLASS): New define. >> * config/avr/avr-protos.h (avr_cannot_change_mode_class): New proto. >> * config/avr/avr.c (avr_cannot_change_mode_class): New function. >> >> gcc/testsuite/ >> PR target/78883 >> * gcc.c-torture/compile/pr78883.c: New test. > >> Index: config/avr/avr-protos.h >> =================================================================== >> --- config/avr/avr-protos.h (revision 244001) >> +++ config/avr/avr-protos.h (working copy) >> @@ -111,7 +111,7 @@ extern int _reg_unused_after (rtx_insn * >> extern int avr_jump_mode (rtx x, rtx_insn *insn); >> extern int test_hard_reg_class (enum reg_class rclass, rtx x); >> extern int jump_over_one_insn_p (rtx_insn *insn, rtx dest); >> - >> +extern int avr_cannot_change_mode_class (machine_mode, machine_mode, enum reg_class); >> extern int avr_hard_regno_mode_ok (int regno, machine_mode mode); >> extern void avr_final_prescan_insn (rtx_insn *insn, rtx *operand, >> int num_operands); >> Index: config/avr/avr.c >> =================================================================== >> --- config/avr/avr.c (revision 244001) >> +++ config/avr/avr.c (working copy) >> @@ -11833,6 +11833,21 @@ jump_over_one_insn_p (rtx_insn *insn, rt >> } >> >> >> +/* Worker function for `CANNOT_CHANGE_MODE_CLASS'. */ >> + >> +int >> +avr_cannot_change_mode_class (machine_mode from, machine_mode to, >> + enum reg_class /* rclass */) >> +{ >> + /* We cannot access a hard register in a wider mode, for example we >> + must not access (reg:QI 31) as (reg:HI 31). HARD_REGNO_MODE_OK >> + would avoid such hard regs, but reload would generate it anyway >> + from paradoxical subregs of mem, cf. PR78883. */ >> + >> + return GET_MODE_SIZE (to) > GET_MODE_SIZE (from); > > I understand how this fixes the ICE, but is it really necessary to > suppress conversions to a wider mode for lower numbered registers? If there is a better hook, I'll propose an according patch. My expectation was that HARD_REGNO_MODE_OK would be enough to keep reload from putting HI into odd registers (and in particular into R31). But this is obviously not the case... And internals are not very helpful here. It only mentions modifying ordinary subregs of pseudos, but not paradoxical subreg of memory. What's also astonishing me is that this problem never popped up during the last > 15 years of avr back-end. Johann >> +} >> + >> + >> /* Worker function for `HARD_REGNO_MODE_OK'. */ >> /* Returns 1 if a value of mode MODE can be stored starting with hard >> register number REGNO. On the enhanced core, anything larger than >> Index: config/avr/avr.h >> =================================================================== >> --- config/avr/avr.h (revision 244001) >> +++ config/avr/avr.h (working copy) >> @@ -216,6 +216,9 @@ These two properties are reflected by bu >> >> #define MODES_TIEABLE_P(MODE1, MODE2) 1 >> >> +#define CANNOT_CHANGE_MODE_CLASS(MFROM, MTO, RCLASS) \ >> + avr_cannot_change_mode_class (MFROM, MTO, RCLASS) >> + >> enum reg_class { >> NO_REGS, >> R0_REG, /* r0 */ >> Index: testsuite/gcc.c-torture/compile/pr78883.c >> =================================================================== >> --- testsuite/gcc.c-torture/compile/pr78883.c (nonexistent) >> +++ testsuite/gcc.c-torture/compile/pr78883.c (working copy) >> @@ -0,0 +1,12 @@ >> +/* { dg-do compile } */ >> + >> +int foo (int *p) >> +{ >> + int i; >> + for (i = 0; i < 5; i++) >> + { >> + if (p[i] & 1) >> + return i; >> + } >> + return -1; >> +} > > Ciao > > Dominik ^_^ ^_^ >
Georg-Johann Lay <avr@gjlay.de> writes: > On 02.01.2017 15:54, Dominik Vogt wrote: >> On Mon, Jan 02, 2017 at 03:47:43PM +0100, Georg-Johann Lay wrote: >>> This fixes PR78883 which is a problem in reload revealed by a >>> change to combine.c. The fix is as proposed by Segher: implement >>> CANNOT_CHANGE_MODE_CLASS. >>> >>> Ok for trunk? >>> >>> Johann >>> >>> >>> gcc/ >>> PR target/78883 >>> * config/avr/avr.h (CANNOT_CHANGE_MODE_CLASS): New define. >>> * config/avr/avr-protos.h (avr_cannot_change_mode_class): New proto. >>> * config/avr/avr.c (avr_cannot_change_mode_class): New function. >>> >>> gcc/testsuite/ >>> PR target/78883 >>> * gcc.c-torture/compile/pr78883.c: New test. >> >>> Index: config/avr/avr-protos.h >>> =================================================================== >>> --- config/avr/avr-protos.h (revision 244001) >>> +++ config/avr/avr-protos.h (working copy) >>> @@ -111,7 +111,7 @@ extern int _reg_unused_after (rtx_insn * >>> extern int avr_jump_mode (rtx x, rtx_insn *insn); >>> extern int test_hard_reg_class (enum reg_class rclass, rtx x); >>> extern int jump_over_one_insn_p (rtx_insn *insn, rtx dest); >>> - >>> +extern int avr_cannot_change_mode_class (machine_mode, machine_mode, enum reg_class); >>> extern int avr_hard_regno_mode_ok (int regno, machine_mode mode); >>> extern void avr_final_prescan_insn (rtx_insn *insn, rtx *operand, >>> int num_operands); >>> Index: config/avr/avr.c >>> =================================================================== >>> --- config/avr/avr.c (revision 244001) >>> +++ config/avr/avr.c (working copy) >>> @@ -11833,6 +11833,21 @@ jump_over_one_insn_p (rtx_insn *insn, rt >>> } >>> >>> >>> +/* Worker function for `CANNOT_CHANGE_MODE_CLASS'. */ >>> + >>> +int >>> +avr_cannot_change_mode_class (machine_mode from, machine_mode to, >>> + enum reg_class /* rclass */) >>> +{ >>> + /* We cannot access a hard register in a wider mode, for example we >>> + must not access (reg:QI 31) as (reg:HI 31). HARD_REGNO_MODE_OK >>> + would avoid such hard regs, but reload would generate it anyway >>> + from paradoxical subregs of mem, cf. PR78883. */ >>> + >>> + return GET_MODE_SIZE (to) > GET_MODE_SIZE (from); >> >> I understand how this fixes the ICE, but is it really necessary to >> suppress conversions to a wider mode for lower numbered registers? > > If there is a better hook, I'll propose an according patch. > > My expectation was that HARD_REGNO_MODE_OK would be enough to keep > reload from putting HI into odd registers (and in particular into R31). > But this is obviously not the case... It should be enough in principle. It's just a case of whether you want to fix reload, hack around it, or take the plunge and switch to LRA. Having a (subreg (mem)) is probably key here. If it had been (subreg (reg:HI X)) for some pseudo X then init_subregs_of_mode should have realised that 31 isn't a valid choice for X. I think the reload fix would be to honour simplifiable_subregs when reloading the (subreg (mem)). > And internals are not very helpful here. It only mentions modifying > ordinary subregs of pseudos, but not paradoxical subreg of memory. > > What's also astonishing me is that this problem never popped up > during the last > 15 years of avr back-end. FWIW, the current init_subregs_of_mode/simplifiable_subregs behaviour is fairly recent (2014) and CANNOT_CHANGE_MODE_CLASS had been used in the past to avoid errors like this. Using it that way was always a hack though. An alternative would be to add a new macro to control this block in general_operand: #ifdef INSN_SCHEDULING /* On machines that have insn scheduling, we want all memory reference to be explicit, so outlaw paradoxical SUBREGs. However, we must allow them after reload so that they can get cleaned up by cleanup_subreg_operands. */ if (!reload_completed && MEM_P (sub) && GET_MODE_SIZE (mode) > GET_MODE_SIZE (GET_MODE (sub))) return 0; #endif The default would still be INSN_SCHEDULING, but AVR could override it to 1 and reject the same subregs. That would still be a hack, but at least it would be taking things in a good direction. Having different rules depending on whether targets define a scheduler is just a horrible wart that no-one's ever had chance to fix. If using the code above works well on AVR then that'd be a big step towards making the code unconditional. It'd definitely be worth checking how it affects code quality though. (Although the same goes for the current patch, since C_C_M_C is a pretty big hammer.) Thanks, Richard
On Tue, Jan 03, 2017 at 01:43:01PM +0000, Richard Sandiford wrote: > An alternative would be to add a new macro to control this block in > general_operand: > > #ifdef INSN_SCHEDULING > /* On machines that have insn scheduling, we want all memory > reference to be explicit, so outlaw paradoxical SUBREGs. > However, we must allow them after reload so that they can > get cleaned up by cleanup_subreg_operands. */ > if (!reload_completed && MEM_P (sub) > && GET_MODE_SIZE (mode) > GET_MODE_SIZE (GET_MODE (sub))) > return 0; > #endif > > The default would still be INSN_SCHEDULING, but AVR could override it > to 1 and reject the same subregs. Or you can define INSN_SCHEDULING (by defining a trivial automaton for your port: a define_automaton, a define_cpu_unit for that automaton, and a define_insn_reservation for that unit). It would be nice if we could separate "no subregs of memory" from "has instruction scheduling". Or ideally subregs of memory would just go away completely. > That would still be a hack, but at least it would be taking things in > a good direction. Having different rules depending on whether targets > define a scheduler is just a horrible wart that no-one's ever had chance > to fix. If using the code above works well on AVR then that'd be a big > step towards making the code unconditional. Ugh, it sounds like I am volunteering. Oh well. > It'd definitely be worth checking how it affects code quality though. It shouldn't be too bad, if ports that do have instruction scheduling can live without it. > (Although the same goes for the current patch, since C_C_M_C is a pretty > big hammer.) Yeah, and the current patch disallows much more than is needed here, even. Segher
2017-01-02 19:22 GMT+04:00 Georg-Johann Lay <avr@gjlay.de>: > On 02.01.2017 15:54, Dominik Vogt wrote: >> >> On Mon, Jan 02, 2017 at 03:47:43PM +0100, Georg-Johann Lay wrote: >>> >>> This fixes PR78883 which is a problem in reload revealed by a >>> change to combine.c. The fix is as proposed by Segher: implement >>> CANNOT_CHANGE_MODE_CLASS. >>> >>> Ok for trunk? >>> >>> Johann >>> >>> >>> gcc/ >>> PR target/78883 >>> * config/avr/avr.h (CANNOT_CHANGE_MODE_CLASS): New define. >>> * config/avr/avr-protos.h (avr_cannot_change_mode_class): New >>> proto. >>> * config/avr/avr.c (avr_cannot_change_mode_class): New function. >>> >>> gcc/testsuite/ >>> PR target/78883 >>> * gcc.c-torture/compile/pr78883.c: New test. >> >> >>> Index: config/avr/avr-protos.h >>> =================================================================== >>> --- config/avr/avr-protos.h (revision 244001) >>> +++ config/avr/avr-protos.h (working copy) >>> @@ -111,7 +111,7 @@ extern int _reg_unused_after (rtx_insn * >>> extern int avr_jump_mode (rtx x, rtx_insn *insn); >>> extern int test_hard_reg_class (enum reg_class rclass, rtx x); >>> extern int jump_over_one_insn_p (rtx_insn *insn, rtx dest); >>> - >>> +extern int avr_cannot_change_mode_class (machine_mode, machine_mode, >>> enum reg_class); >>> extern int avr_hard_regno_mode_ok (int regno, machine_mode mode); >>> extern void avr_final_prescan_insn (rtx_insn *insn, rtx *operand, >>> int num_operands); >>> Index: config/avr/avr.c >>> =================================================================== >>> --- config/avr/avr.c (revision 244001) >>> +++ config/avr/avr.c (working copy) >>> @@ -11833,6 +11833,21 @@ jump_over_one_insn_p (rtx_insn *insn, rt >>> } >>> >>> >>> +/* Worker function for `CANNOT_CHANGE_MODE_CLASS'. */ >>> + >>> +int >>> +avr_cannot_change_mode_class (machine_mode from, machine_mode to, >>> + enum reg_class /* rclass */) >>> +{ >>> + /* We cannot access a hard register in a wider mode, for example we >>> + must not access (reg:QI 31) as (reg:HI 31). HARD_REGNO_MODE_OK >>> + would avoid such hard regs, but reload would generate it anyway >>> + from paradoxical subregs of mem, cf. PR78883. */ >>> + >>> + return GET_MODE_SIZE (to) > GET_MODE_SIZE (from); >> >> >> I understand how this fixes the ICE, but is it really necessary to >> suppress conversions to a wider mode for lower numbered registers? > > > If there is a better hook, I'll propose an according patch. > > My expectation was that HARD_REGNO_MODE_OK would be enough to keep > reload from putting HI into odd registers (and in particular into R31). > But this is obviously not the case... > > And internals are not very helpful here. It only mentions modifying > ordinary subregs of pseudos, but not paradoxical subreg of memory. > > What's also astonishing me is that this problem never popped up > during the last > 15 years of avr back-end. May be it's a related problem: https://gcc.gnu.org/ml/gcc/2011-02/msg00444.html > > Johann > > > >>> +} >>> + >>> + >>> /* Worker function for `HARD_REGNO_MODE_OK'. */ >>> /* Returns 1 if a value of mode MODE can be stored starting with hard >>> register number REGNO. On the enhanced core, anything larger than >>> Index: config/avr/avr.h >>> =================================================================== >>> --- config/avr/avr.h (revision 244001) >>> +++ config/avr/avr.h (working copy) >>> @@ -216,6 +216,9 @@ These two properties are reflected by bu >>> >>> #define MODES_TIEABLE_P(MODE1, MODE2) 1 >>> >>> +#define CANNOT_CHANGE_MODE_CLASS(MFROM, MTO, RCLASS) \ >>> + avr_cannot_change_mode_class (MFROM, MTO, RCLASS) >>> + >>> enum reg_class { >>> NO_REGS, >>> R0_REG, /* r0 */ >>> Index: testsuite/gcc.c-torture/compile/pr78883.c >>> =================================================================== >>> --- testsuite/gcc.c-torture/compile/pr78883.c (nonexistent) >>> +++ testsuite/gcc.c-torture/compile/pr78883.c (working copy) >>> @@ -0,0 +1,12 @@ >>> +/* { dg-do compile } */ >>> + >>> +int foo (int *p) >>> +{ >>> + int i; >>> + for (i = 0; i < 5; i++) >>> + { >>> + if (p[i] & 1) >>> + return i; >>> + } >>> + return -1; >>> +} >> >> >> Ciao >> >> Dominik ^_^ ^_^ >> >
Index: config/avr/avr-protos.h =================================================================== --- config/avr/avr-protos.h (revision 244001) +++ config/avr/avr-protos.h (working copy) @@ -111,7 +111,7 @@ extern int _reg_unused_after (rtx_insn * extern int avr_jump_mode (rtx x, rtx_insn *insn); extern int test_hard_reg_class (enum reg_class rclass, rtx x); extern int jump_over_one_insn_p (rtx_insn *insn, rtx dest); - +extern int avr_cannot_change_mode_class (machine_mode, machine_mode, enum reg_class); extern int avr_hard_regno_mode_ok (int regno, machine_mode mode); extern void avr_final_prescan_insn (rtx_insn *insn, rtx *operand, int num_operands); Index: config/avr/avr.c =================================================================== --- config/avr/avr.c (revision 244001) +++ config/avr/avr.c (working copy) @@ -11833,6 +11833,21 @@ jump_over_one_insn_p (rtx_insn *insn, rt } +/* Worker function for `CANNOT_CHANGE_MODE_CLASS'. */ + +int +avr_cannot_change_mode_class (machine_mode from, machine_mode to, + enum reg_class /* rclass */) +{ + /* We cannot access a hard register in a wider mode, for example we + must not access (reg:QI 31) as (reg:HI 31). HARD_REGNO_MODE_OK + would avoid such hard regs, but reload would generate it anyway + from paradoxical subregs of mem, cf. PR78883. */ + + return GET_MODE_SIZE (to) > GET_MODE_SIZE (from); +} + + /* Worker function for `HARD_REGNO_MODE_OK'. */ /* Returns 1 if a value of mode MODE can be stored starting with hard register number REGNO. On the enhanced core, anything larger than Index: config/avr/avr.h =================================================================== --- config/avr/avr.h (revision 244001) +++ config/avr/avr.h (working copy) @@ -216,6 +216,9 @@ These two properties are reflected by bu #define MODES_TIEABLE_P(MODE1, MODE2) 1 +#define CANNOT_CHANGE_MODE_CLASS(MFROM, MTO, RCLASS) \ + avr_cannot_change_mode_class (MFROM, MTO, RCLASS) + enum reg_class { NO_REGS, R0_REG, /* r0 */ Index: testsuite/gcc.c-torture/compile/pr78883.c =================================================================== --- testsuite/gcc.c-torture/compile/pr78883.c (nonexistent) +++ testsuite/gcc.c-torture/compile/pr78883.c (working copy) @@ -0,0 +1,12 @@ +/* { dg-do compile } */ + +int foo (int *p) +{ + int i; + for (i = 0; i < 5; i++) + { + if (p[i] & 1) + return i; + } + return -1; +}