[patch,avr] PR78883: Implement a dummy scheduler
On 03.01.2017 14:43, Richard Sandiford wrote:
Georg-Johann Lay 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.
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...
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
For now, here is the 2nd approach implemented: Add a dummy scheduler.
At least the test case passes with this change.
Johann
gcc/
PR78883
* config/avr/avr-dfa.md: New file.
* config/avr/avr.md
Re: [patch,avr] PR78883: Implement a dummy scheduler
Richard Sandiford schrieb:
Segher Boessenkool 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
Re: [patch,avr] PR78883: Implement a dummy scheduler
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 +, 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
Re: [patch,avr] PR78883: Implement a dummy scheduler
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
Re: [patch,avr] PR78883: Implement a dummy scheduler
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
Re: [patch,avr] PR78883: Implement a dummy scheduler
Georg-Johann Lay 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 +, 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
Re: [patch,avr] PR78883: Implement a dummy scheduler
On 12.01.2017 10:00, Richard Sandiford wrote:
Georg-Johann Lay 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 +, 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,
Richar
Re: [patch,avr] PR78883: Implement a dummy scheduler
Georg-Johann Lay writes:
> On 12.01.2017 10:00, Richard Sandiford wrote:
>> Georg-Johann Lay 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 +, 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 l
Re: [patch,avr] PR78883: Implement a dummy scheduler
Richard Sandiford schrieb:
Georg-Johann Lay writes:
On 12.01.2017 10:00, Richard Sandiford wrote:
Georg-Johann Lay 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 +, 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
Re: [patch,avr] PR78883: Implement a dummy scheduler
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
Re: [patch,avr] PR78883: Implement a dummy scheduler
Segher Boessenkool 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
Re: [patch,avr] PR78883: Implement a dummy scheduler
On Wed, Jan 04, 2017 at 06:42:23PM +, 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
Re: [patch,avr] PR78883: Implement a dummy scheduler
On 01/04/2017 12:18 PM, Segher Boessenkool wrote:
On Wed, Jan 04, 2017 at 06:42:23PM +, 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
