[patch,avr] PR78883: Implement a dummy scheduler

[Richard Sandiford]

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