| Message ID | bb622d64-3af5-f965-8d0c-b3724506484d@gjlay.de |
|---|---|
| State | New |
| Headers | show |
On Wed, Jan 04, 2017 at 04:39:36PM +0100, Georg-Johann Lay wrote: > Well, if it can be done in the back-end, then that's generally my strong > preference. And the blocker for LRA is that it doesn't support cc0, > hence LRA will require an almost complete rewrite of the avr back-end... Heh, getting rid of cc0 isn't *that* dramatic a change. It does require knowing the target really well (or some serious time with arch manuals). One day all cc0 support will be removed, and it's advantageous to use the newer code instead anyway... > + // Currently, the only purpose of the insn scheduler is to work > + // around PR78883, i.e. we only need the side effect of defining > + // INSN_SCHEDULING. Even with a dummy scheduler we will see reodering > + // of instructions, which is not wanted if not actually needed. > + { OPT_LEVELS_ALL, OPT_fschedule_insns, NULL, 0 }, > + { OPT_LEVELS_ALL, OPT_fschedule_insns2, NULL, 0 }, > +;; Notice that we disable -fschedule-insns and -fschedule-insns2 in > +;; avr-common.c::TARGET_OPTION_OPTIMIZATION_TABLE because some "random" > +;; reordering of instructions is not wanted. > + > +(define_automaton "avr_dfa") > + > +(define_cpu_unit "avr_cpu" "avr_dfa") > + > +(define_insn_reservation "avr_cpu_reserve" 1 > + (const_int 1) > + "avr_cpu") I think you won't see this "random" reordering if you use (const_int 0) for the condition instead, i.e. the reservation will never match (instead of always as you have now). Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Wed, Jan 04, 2017 at 04:39:36PM +0100, Georg-Johann Lay wrote: >> Well, if it can be done in the back-end, then that's generally my strong >> preference. And the blocker for LRA is that it doesn't support cc0, >> hence LRA will require an almost complete rewrite of the avr back-end... > > Heh, getting rid of cc0 isn't *that* dramatic a change. It does require > knowing the target really well (or some serious time with arch manuals). > > One day all cc0 support will be removed, and it's advantageous to use the > newer code instead anyway... > >> + // Currently, the only purpose of the insn scheduler is to work >> + // around PR78883, i.e. we only need the side effect of defining >> + // INSN_SCHEDULING. Even with a dummy scheduler we will see reodering >> + // of instructions, which is not wanted if not actually needed. >> + { OPT_LEVELS_ALL, OPT_fschedule_insns, NULL, 0 }, >> + { OPT_LEVELS_ALL, OPT_fschedule_insns2, NULL, 0 }, > >> +;; Notice that we disable -fschedule-insns and -fschedule-insns2 in >> +;; avr-common.c::TARGET_OPTION_OPTIMIZATION_TABLE because some "random" >> +;; reordering of instructions is not wanted. >> + >> +(define_automaton "avr_dfa") >> + >> +(define_cpu_unit "avr_cpu" "avr_dfa") >> + >> +(define_insn_reservation "avr_cpu_reserve" 1 >> + (const_int 1) >> + "avr_cpu") > > I think you won't see this "random" reordering if you use (const_int 0) > for the condition instead, i.e. the reservation will never match (instead > of always as you have now). Both ways feel wrong to me TBH. The sequence that led to this patch is: 1. reload has a bug that no-one really wants to fix (understandable) 2. the bug is triggered by paradoxical subregs of mems 3. those subregs are normally disabled on targets that support insn scheduling 4. therefore, define an insn scheduler 5. we don't actually want insn scheduling, so either (a) make sure the insn scheduler is never actually used for insn scheduling, or (b) allow the insn scheduler to run anyway but encourage it to do nothing (other than take compile time) (4) and (5) feel like too much of a hack to me. They're going to have other consequences, e.g. we'll no longer give the warning: instruction scheduling not supported on this target machine if users try to use -fschedule-insns. And since we don't support meaningful insn scheduling even after this patch, giving the warning seems more user-friendly than dropping it. I think the consensus is that we don't want these subregs for AVR regardless of whether scheduling is used, and probably wouldn't want them even without this bug. So why not instead change the condition used by general_operand, like we were talking about yesterday? It seems simpler and more direct. Thanks, Richard
On Wed, Jan 04, 2017 at 06:42:23PM +0000, Richard Sandiford wrote: > 1. reload has a bug that no-one really wants to fix (understandable) > 2. the bug is triggered by paradoxical subregs of mems > 3. those subregs are normally disabled on targets that support insn > scheduling > 4. therefore, define an insn scheduler > 5. we don't actually want insn scheduling, so either > (a) make sure the insn scheduler is never actually used for insn > scheduling, or > (b) allow the insn scheduler to run anyway but encourage it to do nothing > (other than take compile time) > > (4) and (5) feel like too much of a hack to me. They're going to have > other consequences, e.g. we'll no longer give the warning: > > instruction scheduling not supported on this target machine > > if users try to use -fschedule-insns. And since we don't support > meaningful insn scheduling even after this patch, giving the warning > seems more user-friendly than dropping it. > > I think the consensus is that we don't want these subregs for AVR > regardless of whether scheduling is used, and probably wouldn't want > them even without this bug. Right, and the same is true for most targets. Subregs of memory are not something you want. As rtl.texi says: @item mem @code{subreg}s of @code{mem} were common in earlier versions of GCC and are still supported. During the reload pass these are replaced by plain @code{mem}s. On machines that do not do instruction scheduling, use of @code{subreg}s of @code{mem} are still used, but this is no longer recommended. Such @code{subreg}s are considered to be @code{register_operand}s rather than @code{memory_operand}s before and during reload. Because of this, the scheduling passes cannot properly schedule instructions with @code{subreg}s of @code{mem}, so for machines that do scheduling, @code{subreg}s of @code{mem} should never be used. To support this, the combine and recog passes have explicit code to inhibit the creation of @code{subreg}s of @code{mem} when @code{INSN_SCHEDULING} is defined. > So why not instead change the condition > used by general_operand, like we were talking about yesterday? > It seems simpler and more direct. We should split off a new "SUBREGS_OF_MEM_ALLOWED" from !INSN_SCHEDULING, and then probably even default it to false. Segher
On 01/04/2017 12:18 PM, Segher Boessenkool wrote: > On Wed, Jan 04, 2017 at 06:42:23PM +0000, Richard Sandiford wrote: >> 1. reload has a bug that no-one really wants to fix (understandable) >> 2. the bug is triggered by paradoxical subregs of mems >> 3. those subregs are normally disabled on targets that support insn >> scheduling >> 4. therefore, define an insn scheduler >> 5. we don't actually want insn scheduling, so either >> (a) make sure the insn scheduler is never actually used for insn >> scheduling, or >> (b) allow the insn scheduler to run anyway but encourage it to do nothing >> (other than take compile time) >> >> (4) and (5) feel like too much of a hack to me. They're going to have >> other consequences, e.g. we'll no longer give the warning: >> >> instruction scheduling not supported on this target machine >> >> if users try to use -fschedule-insns. And since we don't support >> meaningful insn scheduling even after this patch, giving the warning >> seems more user-friendly than dropping it. >> >> I think the consensus is that we don't want these subregs for AVR >> regardless of whether scheduling is used, and probably wouldn't want >> them even without this bug. > > Right, and the same is true for most targets. Subregs of memory are not > something you want. As rtl.texi says: > > > @item mem > @code{subreg}s of @code{mem} were common in earlier versions of GCC and > are still supported. During the reload pass these are replaced by plain > @code{mem}s. On machines that do not do instruction scheduling, use of > @code{subreg}s of @code{mem} are still used, but this is no longer > recommended. Such @code{subreg}s are considered to be > @code{register_operand}s rather than @code{memory_operand}s before and > during reload. Because of this, the scheduling passes cannot properly > schedule instructions with @code{subreg}s of @code{mem}, so for machines > that do scheduling, @code{subreg}s of @code{mem} should never be used. > To support this, the combine and recog passes have explicit code to > inhibit the creation of @code{subreg}s of @code{mem} when > @code{INSN_SCHEDULING} is defined. > > >> So why not instead change the condition >> used by general_operand, like we were talking about yesterday? >> It seems simpler and more direct. > > We should split off a new "SUBREGS_OF_MEM_ALLOWED" from !INSN_SCHEDULING, > and then probably even default it to false. That would work for me :-) The question in my mind would be unexpected fallout at this point in the release process. Maybe default it to !INSN_SCHEDULING to minimize such fallout now, then to false for gcc-8? jeff
Richard Sandiford schrieb: > Segher Boessenkool <segher@kernel.crashing.org> writes: >> On Wed, Jan 04, 2017 at 04:39:36PM +0100, Georg-Johann Lay wrote: >>> Well, if it can be done in the back-end, then that's generally my strong >>> preference. And the blocker for LRA is that it doesn't support cc0, >>> hence LRA will require an almost complete rewrite of the avr back-end... >> Heh, getting rid of cc0 isn't *that* dramatic a change. It does require >> knowing the target really well (or some serious time with arch manuals). >> >> One day all cc0 support will be removed, and it's advantageous to use the >> newer code instead anyway... >> >>> + // Currently, the only purpose of the insn scheduler is to work >>> + // around PR78883, i.e. we only need the side effect of defining >>> + // INSN_SCHEDULING. Even with a dummy scheduler we will see reodering >>> + // of instructions, which is not wanted if not actually needed. >>> + { OPT_LEVELS_ALL, OPT_fschedule_insns, NULL, 0 }, >>> + { OPT_LEVELS_ALL, OPT_fschedule_insns2, NULL, 0 }, >>> +;; Notice that we disable -fschedule-insns and -fschedule-insns2 in >>> +;; avr-common.c::TARGET_OPTION_OPTIMIZATION_TABLE because some "random" >>> +;; reordering of instructions is not wanted. >>> + >>> +(define_automaton "avr_dfa") >>> + >>> +(define_cpu_unit "avr_cpu" "avr_dfa") >>> + >>> +(define_insn_reservation "avr_cpu_reserve" 1 >>> + (const_int 1) >>> + "avr_cpu") >> I think you won't see this "random" reordering if you use (const_int 0) Unfortunately, not. I tried that before switching off the schedulers. >> for the condition instead, i.e. the reservation will never match (instead >> of always as you have now). > > Both ways feel wrong to me TBH. The sequence that led to this patch is: > > 1. reload has a bug that no-one really wants to fix (understandable) > 2. the bug is triggered by paradoxical subregs of mems > 3. those subregs are normally disabled on targets that support insn > scheduling > 4. therefore, define an insn scheduler > 5. we don't actually want insn scheduling, so either > (a) make sure the insn scheduler is never actually used for insn > scheduling, or > (b) allow the insn scheduler to run anyway but encourage it to do nothing > (other than take compile time) > > (4) and (5) feel like too much of a hack to me. They're going to have > other consequences, e.g. we'll no longer give the warning: > > instruction scheduling not supported on this target machine > > if users try to use -fschedule-insns. And since we don't support Good catch. I never saw a user setting -fschedule-*. The only "user" that actually would set it would be the testsuite. > meaningful insn scheduling even after this patch, giving the warning > seems more user-friendly than dropping it. I actually considered implementing a scheduler, not because AVR had a pipeline (it don't) but to reorder code by some of the sched hooks and increase peep2 odds to combine 2 MOVs to one MOVW. > I think the consensus is that we don't want these subregs for AVR > regardless of whether scheduling is used, and probably wouldn't want > them even without this bug. So why not instead change the condition > used by general_operand, like we were talking about yesterday? > It seems simpler and more direct. As #if or #ifdefi guess? because hookizing such hot code is not wanted. Johann
On 04.01.2017 20:29, Jeff Law wrote: > On 01/04/2017 12:18 PM, Segher Boessenkool wrote: >> On Wed, Jan 04, 2017 at 06:42:23PM +0000, Richard Sandiford wrote: >>> 1. reload has a bug that no-one really wants to fix (understandable) >>> 2. the bug is triggered by paradoxical subregs of mems >>> 3. those subregs are normally disabled on targets that support insn >>> scheduling >>> 4. therefore, define an insn scheduler >>> 5. we don't actually want insn scheduling, so either >>> (a) make sure the insn scheduler is never actually used for insn >>> scheduling, or >>> (b) allow the insn scheduler to run anyway but encourage it to do >>> nothing >>> (other than take compile time) >>> >>> (4) and (5) feel like too much of a hack to me. They're going to have >>> other consequences, e.g. we'll no longer give the warning: >>> >>> instruction scheduling not supported on this target machine >>> >>> if users try to use -fschedule-insns. And since we don't support >>> meaningful insn scheduling even after this patch, giving the warning >>> seems more user-friendly than dropping it. >>> >>> I think the consensus is that we don't want these subregs for AVR >>> regardless of whether scheduling is used, and probably wouldn't want >>> them even without this bug. >> >> Right, and the same is true for most targets. Subregs of memory are not >> something you want. As rtl.texi says: >> >> >> @item mem >> @code{subreg}s of @code{mem} were common in earlier versions of GCC and >> are still supported. During the reload pass these are replaced by plain >> @code{mem}s. On machines that do not do instruction scheduling, use of >> @code{subreg}s of @code{mem} are still used, but this is no longer >> recommended. Such @code{subreg}s are considered to be >> @code{register_operand}s rather than @code{memory_operand}s before and >> during reload. Because of this, the scheduling passes cannot properly >> schedule instructions with @code{subreg}s of @code{mem}, so for machines >> that do scheduling, @code{subreg}s of @code{mem} should never be used. >> To support this, the combine and recog passes have explicit code to >> inhibit the creation of @code{subreg}s of @code{mem} when >> @code{INSN_SCHEDULING} is defined. >> >> >>> So why not instead change the condition >>> used by general_operand, like we were talking about yesterday? >>> It seems simpler and more direct. >> >> We should split off a new "SUBREGS_OF_MEM_ALLOWED" from !INSN_SCHEDULING, >> and then probably even default it to false. > That would work for me :-) The question in my mind would be unexpected > fallout at this point in the release process. Maybe default it to > !INSN_SCHEDULING to minimize such fallout now, then to false for gcc-8? > > > jeff Bit if we disable it, what's the point of introducing changes to combine which come up with even more of such subregs? For targets with scheduling, which applies to most of the targets, the "optimization" in combine will be void as rejected by general_operand, hence a target would have explicit paradoxical subregs in the back end or use some home brew predicated that allow that stuff and use internal knowledge of what combine does. Moreover I have some problems in explaining what the new hook macro is supposed to do: "Disable/enable paradoxical SUBREGs of MEM in general_operands before register allocation. Use this hook if your back end has trouble with paradoxical subregs of mem. Enabled per default iff the target provides an insn scheduler." Who would understand this and infer from the docs whether this macro should be used? And if such subregs are forbidden, why are they generated in the first place? Shouldn't combine also respect that hook? Johann
On Wed, Jan 04, 2017 at 12:29:49PM -0700, Jeff Law wrote: > >We should split off a new "SUBREGS_OF_MEM_ALLOWED" from !INSN_SCHEDULING, > >and then probably even default it to false. > That would work for me :-) The question in my mind would be unexpected > fallout at this point in the release process. Maybe default it to > !INSN_SCHEDULING to minimize such fallout now, then to false for gcc-8? Do we want to change anything right now? AVR has a workaround for now. Segher
Segher Boessenkool schrieb: > On Wed, Jan 04, 2017 at 12:29:49PM -0700, Jeff Law wrote: >>> We should split off a new "SUBREGS_OF_MEM_ALLOWED" from !INSN_SCHEDULING, >>> and then probably even default it to false. >> That would work for me :-) The question in my mind would be unexpected >> fallout at this point in the release process. Maybe default it to >> !INSN_SCHEDULING to minimize such fallout now, then to false for gcc-8? > > Do we want to change anything right now? AVR has a workaround for now. None of which got approval... > > Segher >
Georg-Johann Lay <avr@gjlay.de> writes: > On 04.01.2017 20:29, Jeff Law wrote: >> On 01/04/2017 12:18 PM, Segher Boessenkool wrote: >>> On Wed, Jan 04, 2017 at 06:42:23PM +0000, Richard Sandiford wrote: >>>> 1. reload has a bug that no-one really wants to fix (understandable) >>>> 2. the bug is triggered by paradoxical subregs of mems >>>> 3. those subregs are normally disabled on targets that support insn >>>> scheduling >>>> 4. therefore, define an insn scheduler >>>> 5. we don't actually want insn scheduling, so either >>>> (a) make sure the insn scheduler is never actually used for insn >>>> scheduling, or >>>> (b) allow the insn scheduler to run anyway but encourage it to do >>>> nothing >>>> (other than take compile time) >>>> >>>> (4) and (5) feel like too much of a hack to me. They're going to have >>>> other consequences, e.g. we'll no longer give the warning: >>>> >>>> instruction scheduling not supported on this target machine >>>> >>>> if users try to use -fschedule-insns. And since we don't support >>>> meaningful insn scheduling even after this patch, giving the warning >>>> seems more user-friendly than dropping it. >>>> >>>> I think the consensus is that we don't want these subregs for AVR >>>> regardless of whether scheduling is used, and probably wouldn't want >>>> them even without this bug. >>> >>> Right, and the same is true for most targets. Subregs of memory are not >>> something you want. As rtl.texi says: >>> >>> >>> @item mem >>> @code{subreg}s of @code{mem} were common in earlier versions of GCC and >>> are still supported. During the reload pass these are replaced by plain >>> @code{mem}s. On machines that do not do instruction scheduling, use of >>> @code{subreg}s of @code{mem} are still used, but this is no longer >>> recommended. Such @code{subreg}s are considered to be >>> @code{register_operand}s rather than @code{memory_operand}s before and >>> during reload. Because of this, the scheduling passes cannot properly >>> schedule instructions with @code{subreg}s of @code{mem}, so for machines >>> that do scheduling, @code{subreg}s of @code{mem} should never be used. >>> To support this, the combine and recog passes have explicit code to >>> inhibit the creation of @code{subreg}s of @code{mem} when >>> @code{INSN_SCHEDULING} is defined. >>> >>> >>>> So why not instead change the condition >>>> used by general_operand, like we were talking about yesterday? >>>> It seems simpler and more direct. >>> >>> We should split off a new "SUBREGS_OF_MEM_ALLOWED" from !INSN_SCHEDULING, >>> and then probably even default it to false. >> That would work for me :-) The question in my mind would be unexpected >> fallout at this point in the release process. Maybe default it to >> !INSN_SCHEDULING to minimize such fallout now, then to false for gcc-8? >> >> >> jeff > > Bit if we disable it, what's the point of introducing changes to combine > which come up with even more of such subregs? > > For targets with scheduling, which applies to most of the targets, the > "optimization" in combine will be void as rejected by general_operand, > hence a target would have explicit paradoxical subregs in the back end > or use some home brew predicated that allow that stuff and use internal > knowledge of what combine does. > > Moreover I have some problems in explaining what the new hook macro is > supposed to do: > > "Disable/enable paradoxical SUBREGs of MEM in general_operands before > register allocation. Use this hook if your back end has trouble with > paradoxical subregs of mem. Enabled per default iff the target > provides an insn scheduler." Sounds OK to me, but... > Who would understand this and infer from the docs whether this macro > should be used? > Who would understand this and infer from the docs whether this macro > should be used? ...how about: ----------------------------------------------------------------------- Define this macro if you do not want predicates such as @code{general_operand} and @code{register_operand} to accept paradoxical @code{subreg}s of @code{mem}s before register allocation. Early versions of GCC treated such @code{subreg}s as register operands and required the register allocator to load the inner @code{mem} into a temporary register. However, this approach effectively hid the load from pre-allocation optimizations like CSE and scheduling and is therefore deprecated. The macro exists so that targets can individually move to the new, preferred, behavior before the deprecated behavior is removed. The macro is defined by default for targets that support instruction scheduling. ----------------------------------------------------------------------- > And if such subregs are forbidden, why are they generated in the first > place? Shouldn't combine also respect that hook? Combine often tries things that the target doesn't accept, so I think what it's doing is valid. Are you worried about a missed optimisation? Thanks, Richard
On 12.01.2017 10:00, Richard Sandiford wrote: > Georg-Johann Lay <avr@gjlay.de> writes: >> On 04.01.2017 20:29, Jeff Law wrote: >>> On 01/04/2017 12:18 PM, Segher Boessenkool wrote: >>>> On Wed, Jan 04, 2017 at 06:42:23PM +0000, Richard Sandiford wrote: >>>>> 1. reload has a bug that no-one really wants to fix (understandable) >>>>> 2. the bug is triggered by paradoxical subregs of mems >>>>> 3. those subregs are normally disabled on targets that support insn >>>>> scheduling >>>>> 4. therefore, define an insn scheduler >>>>> 5. we don't actually want insn scheduling, so either >>>>> (a) make sure the insn scheduler is never actually used for insn >>>>> scheduling, or >>>>> (b) allow the insn scheduler to run anyway but encourage it to do >>>>> nothing >>>>> (other than take compile time) >>>>> >>>>> (4) and (5) feel like too much of a hack to me. They're going to have >>>>> other consequences, e.g. we'll no longer give the warning: >>>>> >>>>> instruction scheduling not supported on this target machine >>>>> >>>>> if users try to use -fschedule-insns. And since we don't support >>>>> meaningful insn scheduling even after this patch, giving the warning >>>>> seems more user-friendly than dropping it. >>>>> >>>>> I think the consensus is that we don't want these subregs for AVR >>>>> regardless of whether scheduling is used, and probably wouldn't want >>>>> them even without this bug. >>>> >>>> Right, and the same is true for most targets. Subregs of memory are not >>>> something you want. As rtl.texi says: >>>> >>>> >>>> @item mem >>>> @code{subreg}s of @code{mem} were common in earlier versions of GCC and >>>> are still supported. During the reload pass these are replaced by plain >>>> @code{mem}s. On machines that do not do instruction scheduling, use of >>>> @code{subreg}s of @code{mem} are still used, but this is no longer >>>> recommended. Such @code{subreg}s are considered to be >>>> @code{register_operand}s rather than @code{memory_operand}s before and >>>> during reload. Because of this, the scheduling passes cannot properly >>>> schedule instructions with @code{subreg}s of @code{mem}, so for machines >>>> that do scheduling, @code{subreg}s of @code{mem} should never be used. >>>> To support this, the combine and recog passes have explicit code to >>>> inhibit the creation of @code{subreg}s of @code{mem} when >>>> @code{INSN_SCHEDULING} is defined. >>>> >>>> >>>>> So why not instead change the condition >>>>> used by general_operand, like we were talking about yesterday? >>>>> It seems simpler and more direct. >>>> >>>> We should split off a new "SUBREGS_OF_MEM_ALLOWED" from !INSN_SCHEDULING, >>>> and then probably even default it to false. >>> That would work for me :-) The question in my mind would be unexpected >>> fallout at this point in the release process. Maybe default it to >>> !INSN_SCHEDULING to minimize such fallout now, then to false for gcc-8? >>> >>> >>> jeff >> >> Bit if we disable it, what's the point of introducing changes to combine >> which come up with even more of such subregs? >> >> For targets with scheduling, which applies to most of the targets, the >> "optimization" in combine will be void as rejected by general_operand, >> hence a target would have explicit paradoxical subregs in the back end >> or use some home brew predicated that allow that stuff and use internal >> knowledge of what combine does. >> >> Moreover I have some problems in explaining what the new hook macro is >> supposed to do: >> >> "Disable/enable paradoxical SUBREGs of MEM in general_operands before >> register allocation. Use this hook if your back end has trouble with >> paradoxical subregs of mem. Enabled per default iff the target >> provides an insn scheduler." > > Sounds OK to me, but... > >> Who would understand this and infer from the docs whether this macro >> should be used? > >> Who would understand this and infer from the docs whether this macro >> should be used? > > ...how about: > > ----------------------------------------------------------------------- > Define this macro if you do not want predicates such as > @code{general_operand} and @code{register_operand} to accept paradoxical > @code{subreg}s of @code{mem}s before register allocation. Early versions > of GCC treated such @code{subreg}s as register operands and required > the register allocator to load the inner @code{mem} into a temporary > register. However, this approach effectively hid the load from > pre-allocation optimizations like CSE and scheduling and is therefore > deprecated. As avr cannot operate on memory, it will have to load such values, hence the conclusion would be to enable that stuff? > The macro exists so that targets can individually move to the new, > preferred, behavior before the deprecated behavior is removed. > The macro is defined by default for targets that support instruction > scheduling. Also it appears as purely an optimization tweak, not about kicking out expressions which ICE your backend. > ----------------------------------------------------------------------- > >> And if such subregs are forbidden, why are they generated in the first >> place? Shouldn't combine also respect that hook? > > Combine often tries things that the target doesn't accept, so I think > what it's doing is valid. Are you worried about a missed optimisation? > > Thanks, > Richard If the chunk of code which allows paradoxical subregs of mem will be removed in the near future (that's what Jeff proposed if I understand correctly), then it is pointless for combine to produce such terms (except for backend which allows such terms by special predicates), hence the current "tweak" to combine is actually void. Johann
Georg-Johann Lay <avr@gjlay.de> writes: > On 12.01.2017 10:00, Richard Sandiford wrote: >> Georg-Johann Lay <avr@gjlay.de> writes: >>> On 04.01.2017 20:29, Jeff Law wrote: >>>> On 01/04/2017 12:18 PM, Segher Boessenkool wrote: >>>>> On Wed, Jan 04, 2017 at 06:42:23PM +0000, Richard Sandiford wrote: >>>>>> 1. reload has a bug that no-one really wants to fix (understandable) >>>>>> 2. the bug is triggered by paradoxical subregs of mems >>>>>> 3. those subregs are normally disabled on targets that support insn >>>>>> scheduling >>>>>> 4. therefore, define an insn scheduler >>>>>> 5. we don't actually want insn scheduling, so either >>>>>> (a) make sure the insn scheduler is never actually used for insn >>>>>> scheduling, or >>>>>> (b) allow the insn scheduler to run anyway but encourage it to do >>>>>> nothing >>>>>> (other than take compile time) >>>>>> >>>>>> (4) and (5) feel like too much of a hack to me. They're going to have >>>>>> other consequences, e.g. we'll no longer give the warning: >>>>>> >>>>>> instruction scheduling not supported on this target machine >>>>>> >>>>>> if users try to use -fschedule-insns. And since we don't support >>>>>> meaningful insn scheduling even after this patch, giving the warning >>>>>> seems more user-friendly than dropping it. >>>>>> >>>>>> I think the consensus is that we don't want these subregs for AVR >>>>>> regardless of whether scheduling is used, and probably wouldn't want >>>>>> them even without this bug. >>>>> >>>>> Right, and the same is true for most targets. Subregs of memory are not >>>>> something you want. As rtl.texi says: >>>>> >>>>> >>>>> @item mem >>>>> @code{subreg}s of @code{mem} were common in earlier versions of GCC and >>>>> are still supported. During the reload pass these are replaced by plain >>>>> @code{mem}s. On machines that do not do instruction scheduling, use of >>>>> @code{subreg}s of @code{mem} are still used, but this is no longer >>>>> recommended. Such @code{subreg}s are considered to be >>>>> @code{register_operand}s rather than @code{memory_operand}s before and >>>>> during reload. Because of this, the scheduling passes cannot properly >>>>> schedule instructions with @code{subreg}s of @code{mem}, so for machines >>>>> that do scheduling, @code{subreg}s of @code{mem} should never be used. >>>>> To support this, the combine and recog passes have explicit code to >>>>> inhibit the creation of @code{subreg}s of @code{mem} when >>>>> @code{INSN_SCHEDULING} is defined. >>>>> >>>>> >>>>>> So why not instead change the condition >>>>>> used by general_operand, like we were talking about yesterday? >>>>>> It seems simpler and more direct. >>>>> >>>>> We should split off a new "SUBREGS_OF_MEM_ALLOWED" from !INSN_SCHEDULING, >>>>> and then probably even default it to false. >>>> That would work for me :-) The question in my mind would be unexpected >>>> fallout at this point in the release process. Maybe default it to >>>> !INSN_SCHEDULING to minimize such fallout now, then to false for gcc-8? >>>> >>>> >>>> jeff >>> >>> Bit if we disable it, what's the point of introducing changes to combine >>> which come up with even more of such subregs? >>> >>> For targets with scheduling, which applies to most of the targets, the >>> "optimization" in combine will be void as rejected by general_operand, >>> hence a target would have explicit paradoxical subregs in the back end >>> or use some home brew predicated that allow that stuff and use internal >>> knowledge of what combine does. >>> >>> Moreover I have some problems in explaining what the new hook macro is >>> supposed to do: >>> >>> "Disable/enable paradoxical SUBREGs of MEM in general_operands before >>> register allocation. Use this hook if your back end has trouble with >>> paradoxical subregs of mem. Enabled per default iff the target >>> provides an insn scheduler." >> >> Sounds OK to me, but... >> >>> Who would understand this and infer from the docs whether this macro >>> should be used? >> >>> Who would understand this and infer from the docs whether this macro >>> should be used? >> >> ...how about: >> >> ----------------------------------------------------------------------- >> Define this macro if you do not want predicates such as >> @code{general_operand} and @code{register_operand} to accept paradoxical >> @code{subreg}s of @code{mem}s before register allocation. Early versions >> of GCC treated such @code{subreg}s as register operands and required >> the register allocator to load the inner @code{mem} into a temporary >> register. However, this approach effectively hid the load from >> pre-allocation optimizations like CSE and scheduling and is therefore >> deprecated. > > As avr cannot operate on memory, it will have to load such values, > hence the conclusion would be to enable that stuff? The point is that the load happens either way. It's just a question of whether it happens before register allocation (new behaviour, selected by the macro) or whether it's left to the register allocator itself (old behaviour, to be removed). Doing it before RA should help optimisation. >> The macro exists so that targets can individually move to the new, >> preferred, behavior before the deprecated behavior is removed. >> The macro is defined by default for targets that support instruction >> scheduling. > > Also it appears as purely an optimization tweak, not about kicking > out expressions which ICE your backend. Well, the current code (selected by instruction scheduling) *is* an optimisation tweak. The other point is to switch proactively to behaviour that will be mandatory in future. We shouldn't document it as "define this macro if you hit a reload bug related to paradoxical subregs" :-) Thanks, Richard
Richard Sandiford schrieb: > Georg-Johann Lay <avr@gjlay.de> writes: >> On 12.01.2017 10:00, Richard Sandiford wrote: >>> Georg-Johann Lay <avr@gjlay.de> writes: >>>> On 04.01.2017 20:29, Jeff Law wrote: >>>>> On 01/04/2017 12:18 PM, Segher Boessenkool wrote: >>>>>> On Wed, Jan 04, 2017 at 06:42:23PM +0000, Richard Sandiford wrote: >>>>>>> 1. reload has a bug that no-one really wants to fix (understandable) >>>>>>> 2. the bug is triggered by paradoxical subregs of mems >>>>>>> 3. those subregs are normally disabled on targets that support insn >>>>>>> scheduling >>>>>>> 4. therefore, define an insn scheduler >>>>>>> 5. we don't actually want insn scheduling, so either >>>>>>> (a) make sure the insn scheduler is never actually used for insn >>>>>>> scheduling, or >>>>>>> (b) allow the insn scheduler to run anyway but encourage it to do >>>>>>> nothing >>>>>>> (other than take compile time) >>>>>>> >>>>>>> (4) and (5) feel like too much of a hack to me. They're going to have >>>>>>> other consequences, e.g. we'll no longer give the warning: >>>>>>> >>>>>>> instruction scheduling not supported on this target machine >>>>>>> >>>>>>> if users try to use -fschedule-insns. And since we don't support >>>>>>> meaningful insn scheduling even after this patch, giving the warning >>>>>>> seems more user-friendly than dropping it. >>>>>>> >>>>>>> I think the consensus is that we don't want these subregs for AVR >>>>>>> regardless of whether scheduling is used, and probably wouldn't want >>>>>>> them even without this bug. >>>>>> Right, and the same is true for most targets. Subregs of memory are not >>>>>> something you want. As rtl.texi says: >>>>>> >>>>>> >>>>>> @item mem >>>>>> @code{subreg}s of @code{mem} were common in earlier versions of GCC and >>>>>> are still supported. During the reload pass these are replaced by plain >>>>>> @code{mem}s. On machines that do not do instruction scheduling, use of >>>>>> @code{subreg}s of @code{mem} are still used, but this is no longer >>>>>> recommended. Such @code{subreg}s are considered to be >>>>>> @code{register_operand}s rather than @code{memory_operand}s before and >>>>>> during reload. Because of this, the scheduling passes cannot properly >>>>>> schedule instructions with @code{subreg}s of @code{mem}, so for machines >>>>>> that do scheduling, @code{subreg}s of @code{mem} should never be used. >>>>>> To support this, the combine and recog passes have explicit code to >>>>>> inhibit the creation of @code{subreg}s of @code{mem} when >>>>>> @code{INSN_SCHEDULING} is defined. >>>>>> >>>>>> >>>>>>> So why not instead change the condition >>>>>>> used by general_operand, like we were talking about yesterday? >>>>>>> It seems simpler and more direct. >>>>>> We should split off a new "SUBREGS_OF_MEM_ALLOWED" from !INSN_SCHEDULING, >>>>>> and then probably even default it to false. >>>>> That would work for me :-) The question in my mind would be unexpected >>>>> fallout at this point in the release process. Maybe default it to >>>>> !INSN_SCHEDULING to minimize such fallout now, then to false for gcc-8? >>>>> >>>>> >>>>> jeff >>>> Bit if we disable it, what's the point of introducing changes to combine >>>> which come up with even more of such subregs? >>>> >>>> For targets with scheduling, which applies to most of the targets, the >>>> "optimization" in combine will be void as rejected by general_operand, >>>> hence a target would have explicit paradoxical subregs in the back end >>>> or use some home brew predicated that allow that stuff and use internal >>>> knowledge of what combine does. >>>> >>>> Moreover I have some problems in explaining what the new hook macro is >>>> supposed to do: >>>> >>>> "Disable/enable paradoxical SUBREGs of MEM in general_operands before >>>> register allocation. Use this hook if your back end has trouble with >>>> paradoxical subregs of mem. Enabled per default iff the target >>>> provides an insn scheduler." >>> Sounds OK to me, but... >>> >>>> Who would understand this and infer from the docs whether this macro >>>> should be used? >>>> Who would understand this and infer from the docs whether this macro >>>> should be used? >>> ...how about: >>> >>> ----------------------------------------------------------------------- >>> Define this macro if you do not want predicates such as >>> @code{general_operand} and @code{register_operand} to accept paradoxical >>> @code{subreg}s of @code{mem}s before register allocation. Early versions >>> of GCC treated such @code{subreg}s as register operands and required >>> the register allocator to load the inner @code{mem} into a temporary >>> register. However, this approach effectively hid the load from >>> pre-allocation optimizations like CSE and scheduling and is therefore >>> deprecated. >> As avr cannot operate on memory, it will have to load such values, >> hence the conclusion would be to enable that stuff? > > The point is that the load happens either way. It's just a question > of whether it happens before register allocation (new behaviour, > selected by the macro) or whether it's left to the register allocator > itself (old behaviour, to be removed). Doing it before RA should > help optimisation. > >>> The macro exists so that targets can individually move to the new, >>> preferred, behavior before the deprecated behavior is removed. >>> The macro is defined by default for targets that support instruction >>> scheduling. >> Also it appears as purely an optimization tweak, not about kicking >> out expressions which ICE your backend. > > Well, the current code (selected by instruction scheduling) *is* an > optimisation tweak. The other point is to switch proactively to > behaviour that will be mandatory in future. You mean removing the "#ifdef INSN_SCHEDULING" from general_operand? Johann > We shouldn't document it as "define this macro if you hit a reload > bug related to paradoxical subregs" :-) > > Thanks, > Richard
Index: common/config/avr/avr-common.c =================================================================== --- common/config/avr/avr-common.c (revision 244001) +++ common/config/avr/avr-common.c (working copy) @@ -28,9 +28,18 @@ static const struct default_options avr_option_optimization_table[] = { { OPT_LEVELS_1_PLUS, OPT_fomit_frame_pointer, NULL, 1 }, + // The only effect of -fcaller-saves might be that it triggers // a frame without need when it tries to be smart around calls. { OPT_LEVELS_ALL, OPT_fcaller_saves, NULL, 0 }, + + // Currently, the only purpose of the insn scheduler is to work + // around PR78883, i.e. we only need the side effect of defining + // INSN_SCHEDULING. Even with a dummy scheduler we will see reodering + // of instructions, which is not wanted if not actually needed. + { OPT_LEVELS_ALL, OPT_fschedule_insns, NULL, 0 }, + { OPT_LEVELS_ALL, OPT_fschedule_insns2, NULL, 0 }, + { OPT_LEVELS_NONE, 0, NULL, 0 } }; Index: config/avr/avr-dfa.md =================================================================== --- config/avr/avr-dfa.md (nonexistent) +++ config/avr/avr-dfa.md (working copy) @@ -0,0 +1,48 @@ +;; Copyright (C) 1998-2017 Free Software Foundation, Inc. +;; +;; This file is part of GCC. +;; +;; GCC is free software; you can redistribute it and/or modify +;; it under the terms of the GNU General Public License as published by +;; the Free Software Foundation; either version 3, or (at your option) +;; any later version. +;; +;; GCC is distributed in the hope that it will be useful, +;; but WITHOUT ANY WARRANTY; without even the implied warranty of +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;; GNU General Public License for more details. +;; +;; You should have received a copy of the GNU General Public License +;; along with GCC; see the file COPYING3. If not see +;; <http://www.gnu.org/licenses/>. */ + + +;; FIXME: The only purpose of this dummy scheduler is to work around PR78883 +;; as a less intrusive alternative to implementing CANNOT_CHANGE_MODE_CLASS, +;; cf. https://gcc.gnu.org/ml/gcc-patches/2017-01/msg00078.html +;; +;; Desired side effect is defining INSN_SCHEDULING, which in turn enables the +;; following chunk of code in recog.c::general_operand, which in turn +;; defeats the paradoxical subregs of mem which caused PR78883. +;; +;; #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 +;; +;; Notice that we disable -fschedule-insns and -fschedule-insns2 in +;; avr-common.c::TARGET_OPTION_OPTIMIZATION_TABLE because some "random" +;; reordering of instructions is not wanted. + +(define_automaton "avr_dfa") + +(define_cpu_unit "avr_cpu" "avr_dfa") + +(define_insn_reservation "avr_cpu_reserve" 1 + (const_int 1) + "avr_cpu") Index: config/avr/avr-protos.h =================================================================== --- config/avr/avr-protos.h (revision 244001) +++ config/avr/avr-protos.h (working copy) @@ -31,6 +31,7 @@ extern int avr_hard_regno_rename_ok (uns extern rtx avr_return_addr_rtx (int count, rtx tem); extern void avr_register_target_pragmas (void); extern void avr_init_expanders (void); +extern int avr_hard_regno_call_part_clobbered (unsigned, int); #ifdef TREE_CODE extern void avr_asm_output_aligned_decl_common (FILE*, tree, const char*, unsigned HOST_WIDE_INT, unsigned int, bool); @@ -46,7 +47,6 @@ extern void avr_init_cumulative_args (CU #endif /* TREE_CODE */ #ifdef RTX_CODE -extern int avr_hard_regno_call_part_clobbered (unsigned, machine_mode); extern const char *output_movqi (rtx_insn *insn, rtx operands[], int *l); extern const char *output_movhi (rtx_insn *insn, rtx operands[], int *l); extern const char *output_movsisf (rtx_insn *insn, rtx operands[], int *l); Index: config/avr/avr.c =================================================================== --- config/avr/avr.c (revision 244001) +++ config/avr/avr.c (working copy) @@ -11873,8 +11873,10 @@ avr_hard_regno_mode_ok (int regno, machi /* Implement `HARD_REGNO_CALL_PART_CLOBBERED'. */ int -avr_hard_regno_call_part_clobbered (unsigned regno, machine_mode mode) +avr_hard_regno_call_part_clobbered (unsigned regno, int imode) { + machine_mode mode = (machine_mode) imode; + /* FIXME: This hook gets called with MODE:REGNO combinations that don't represent valid hard registers like, e.g. HI:29. Returning TRUE for such registers can lead to performance degradation as mentioned Index: config/avr/avr.h =================================================================== --- config/avr/avr.h (revision 244001) +++ config/avr/avr.h (working copy) @@ -285,8 +285,11 @@ enum reg_class { #define REGNO_OK_FOR_INDEX_P(NUM) 0 +/* Prototype needed because sched-deps.c does not include tm_p.h. */ +extern int avr_hard_regno_call_part_clobbered (unsigned, int); + #define HARD_REGNO_CALL_PART_CLOBBERED(REGNO, MODE) \ - avr_hard_regno_call_part_clobbered (REGNO, MODE) + avr_hard_regno_call_part_clobbered (REGNO, (int) (MODE)) #define TARGET_SMALL_REGISTER_CLASSES_FOR_MODE_P hook_bool_mode_true Index: config/avr/avr.md =================================================================== --- config/avr/avr.md (revision 244001) +++ config/avr/avr.md (working copy) @@ -6927,3 +6927,6 @@ (define_insn_and_split "*extract.subreg. ;; Operations on 64-bit registers (include "avr-dimode.md") + +;; A dummy DFA to cure PR78883 +(include "avr-dfa.md") 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; +}