Re: PATCH: MIPS 74K load/store scheduling tweak (take 2)

[Richard Sandiford <richard@codesourcery.com>]

David Ung <davidu@mips.com> writes:
> Richard Sandiford wrote:
>> Sandra Loosemore <sandra@codesourcery.com> writes:
>>> + /* Conditionally swap the instructions at POS1 and POS2 in ready queue
>>> +    READY, also adjusting the priority of the instruction formerly at
>>> +    POS1 when we do so.  */
>>> + 
>>> + static void
>>> + mips_maybe_swap_ready (rtx *ready, int pos1, int pos2)
>>> + {
>>> +   if (pos1 < pos2
>>> +       && INSN_PRIORITY (ready[pos1]) + 4 >= INSN_PRIORITY (ready[pos2]))
>>> +     {
>>> +       rtx temp;
>>> +       INSN_PRIORITY (ready[pos1]) = INSN_PRIORITY (ready[pos2]);
>>> +       temp = ready[pos1];
>>> +       ready[pos1] = ready[pos2];
>>> +       ready[pos2] = temp;
>>> +     }
>>> + }
>> 
>> To be a general function rather than a 74k function, the magic value
>> 4 should be a parameter too.  The comment seems a bit vague: how about
>> "Make sure the instruction at POS1 in ready queue READY is ahead of
>> the instruction at POS2, but only if its priority is no less than
>> LIMIT units of the other instruction's priority.  Assume that
>> only one of the instructions may issue this cycle."  Copy-edit
>> as necessary.
>> 
>> It isn't obvious without the last bit why you're only swapping,
>> rather than inserting POS1 directly ahead of POS2.  With the
>> comment there, you can remove:
>> 
>> +   /* At this point the ready queue may no longer be sorted, but that's
>> +      OK since 74k can't schedule concurrent load/store on the same
>> +      cycle.  */
>> 
>> I'm uncertain whether setting INSN_PRIORITY is really the
>> right thing to do here.  David, why isn't the sorting done by
>> mips_sched_reorder enough?
>> 
>
> This is the multi-issue problem.  ready_sort is called more than once
> during each cycle.  The 1st instruction chosen during that cycle may
> not be an AGEN one (eg a floating point insn or an ALU insn), which
> means mips_sched_reorder2 needs to be implemented.  So instead of
> doing the same thing in mips_sched_reorder2, just changing the
> INSN_PRIORITY gets the same effect done automatically by ready_sort.

But the problem with that is that the priority sticks.  If we change
the priority of a load L1 (say) from A to B, the scheduler may later
insert a newly-ready load L2 with a priority between A and B.
We would then wrongly treat L1 as having a higher priority than L2.

I think defining TARGET_SCHED_REORDER2 is the right thing.  Let's
move the "cycle == 0" stuff in mips_sched_reorg into Sandra's new
mips_sched_init function.  I think mips_sched_reorder will then be
suitable for both TARGET_SCHED_REORDER and TARGET_SCHED_REORDER2.

Richard