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

[David Ung <davidu@mips.com>]

Richard Sandiford wrote:
> 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.

The priority sticking is not really a problem, because if there are insn in the 
ready queue, it mean that they are good to go at the current cycle.  A 
newly-ready load L2 you talked about is not going to happen during the current 
cycle, and the now-modified load/store would have been issued.

> 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.

I have no objection to doing it again in TARGET_SCHED_REORDER2.
The only worry is that TARGET_SCHED_REORDER2 will get called multiple times in 
the same cycle (upto 3 for 74k?)!!

David