Re: [patch,avr] PR78883: Implement a dummy scheduler

[Richard Sandiford <richard dot sandiford at arm dot com>]

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